Emit DW_AT_main_subprogram #38109

tromey · 2016-12-01T15:31:35Z

This changes rustc to emit DW_AT_main_subprogram on the "main" program.
This lets gdb suitably stop at the user's main in response to
"start" (rather than the library's main, which is what happens
currently).

Fixes #32620
r? michaelwoerister

alexcrichton · 2016-12-01T16:51:47Z

It looks like llvm 3.7 may break the tests added here (on travis), but I think that can be fixed with // min-llvm-version 3.7

tromey · 2016-12-01T22:41:02Z

I think that can be fixed with // min-llvm-version 3.7

Thanks - I will update the patch soon. I think for this I'll introduce a new test, so that the existing test keeps running on travis.

michaelwoerister

Looks good to me. Thanks @tromey! A test case for the common case would be good (can be very similar to the #[start] test case).

michaelwoerister · 2016-12-02T14:46:19Z

src/librustc_trans/debuginfo/mod.rs

+    let mut flags = FlagPrototyped as c_uint;
+    match *cx.sess().entry_fn.borrow() {
+        Some((id, _)) => {
+            if instance == Instance::mono(cx.shared(), cx.tcx().map.local_def_id(id)) {


This could be simplified to if id == local_id {.

I needed something a bit wordier:

if let Some(local_id_unwrapped) = local_id { if id == local_id_unwrapped { flags |= FlagMainSubprogram as c_uint; } }

Oh, that makes sense. Though you could probably make it shorter be wrapping id in an Option and then comparing Option against Option. Or do something like this:

let entry_fn_id = *cx.sess().entry_fn.borrow(); if entry_fn_id.is_some() && local_id == entry_fn_id { flags |= ...; }

The standard style is is if entry_fn_id == Some(local_id).

michaelwoerister · 2016-12-02T14:47:54Z

src/librustc_trans/debuginfo/mod.rs

@@ -286,7 +296,7 @@ pub fn create_function_debug_context<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>,
            is_local_to_unit,
            true,
            scope_line as c_uint,
-            FlagPrototyped as c_uint,
+            flags,


You removed the FlagPrototyped (which might very well make sense). Can you comment on your reasoning there?

flags is initialized to include it:

+ let mut flags = FlagPrototyped as c_uint;

I'm happy to write this some other way if you think it would be clearer.

Oh, that's right, I just missed it. Sorry.

michaelwoerister · 2016-12-02T14:51:11Z

src/test/codegen/gdb_debug_script_load.rs

@@ -18,6 +18,7 @@

 // CHECK-LABEL: @main
 // CHECK: load volatile i8, i8* getelementptr inbounds ([[B:\[[0-9]* x i8\]]], [[B]]* @__rustc_debug_gdb_scripts_section__, i32 0, i32 0), align 1
+// CHECK: {{.*}}DISubprogram{{.*}}name: "start",{{.*}}DIFlagMainSubprogram{{.*}}

 #[start]


Having a #[start] function is a rather special case, I think. Can we have another test case for a regular main function?

michaelwoerister · 2016-12-02T15:27:40Z

src/librustc_llvm/ffi.rs

@@ -451,6 +451,7 @@ pub mod debuginfo {
        FlagIndirectVariable = 1 << 13,
        FlagLValueReference = 1 << 14,
        FlagRValueReference = 1 << 15,
+        FlagMainSubprogram = 1 << 21,


I looks like older LLVM versions will just silently ignore this. At least 3.7 on Travis. Which is good.

tromey · 2016-12-02T18:27:08Z

It looks like llvm 3.7 may break the tests added here (on travis), but I think that can be fixed with // min-llvm-version 3.7

I'm wondering today if that should be "min-llvm-version 3.8"? Since IIUC 3.7 is what is in travis, and will fail. Even 3.8 is technically wrong in a sense, because if there's a real 3.8 release, it won't contain the needed patch.

michaelwoerister · 2016-12-02T19:49:00Z

I'm wondering today if that should be "min-llvm-version 3.8"?

I guess that no LLVM release in existence today contains the patch, so this test can only pass with our own version of LLVM. Do we have a rust-llvm-only directive for tests?

bors · 2016-12-04T06:16:34Z

☔ The latest upstream changes (presumably #37857) made this pull request unmergeable. Please resolve the merge conflicts.

tromey · 2016-12-20T05:32:35Z

Just FYI - I've rebased this and fixed it up, and it works in manual testing. However the test cases fail now. I don't know the test suite setup very well; my suspicion is that the tests aren't built in isolation, leading rustc not to emit DW_AT_main_subprogram for the test main functions. I'll try to ping someone on irc tomorrow to see what can be done; or else if you are reading this and happen to know, please comment. Thanks :)

tromey · 2016-12-22T18:54:15Z

However the test cases fail now.

Of course this turned out to be operator error; somehow I had a stale rustc in my tree that I was running manually.

michaelwoerister · 2016-12-22T19:18:08Z

Let me know when it's ready for review.

tromey · 2016-12-22T23:10:20Z

@michaelwoerister - hopefully it's ready now.

tromey · 2016-12-23T04:33:00Z

Well, I guess not. In the new scheme for DIFlags I suppose some kind of version or other configury is needed. Maybe I should have listened to that warning in RustWrapper.cpp.

bors · 2016-12-31T00:59:01Z

☔ The latest upstream changes (presumably #38701) made this pull request unmergeable. Please resolve the merge conflicts.

alexcrichton · 2017-02-04T21:53:19Z

ping @tromey, it looks like this may need a rebase now?

tromey · 2017-02-05T04:24:54Z

it looks like this may need a rebase now?

Yeah. Sorry about my silence on this. I still intend to work on it. I think there are two issues with the patch currently.

One is that the refactor in this area means that the old approach of passing in the constant, regardless of the underlying LLVM version, will no longer work, because the enumerator in question just doesn't exist in earlier versions of LLVM. The Rust fork of LLVM does have this, and so does 4.0, but I don't know of a way to distinguish the Rust LLVM from the ordinary upstream LLVM. So, maybe adding a way to distinguish this case would be good.

Alternatively a simpler solution would be to only enable this feature for LLVM 4.0. I think this is less nice because in the future I hope to add more Rust-specific features to LLVM's debuginfo generation, and backport these to the Rust LLVM fork; and if this all comes to pass then it will be handy to have the necessary infrastructure already in place.

The second problem with this patch is related: the tests also have no way to check whether they're being run against the Rust fork of LLVM.

cuviper · 2017-02-05T05:35:55Z

I don't know of a way to distinguish the Rust LLVM from the ordinary upstream LLVM. So, maybe adding a way to distinguish this case would be good.

There is a -DLLVM_RUSTLLVM for this sort of thing. It's used in PassWrapper.cpp to stub a couple functions that only exist in the Rust fork of LLVM.

tromey · 2017-02-05T06:02:52Z

There is a -DLLVM_RUSTLLVM for this sort of thing

Thanks! That really simplifies things.

This changes rustc to emit DW_AT_main_subprogram on the "main" program. This lets gdb suitably stop at the user's main in response to "start" (rather than the library's main, which is what happens currently). Fixes rust-lang#32620 r? michaelwoerister

alexcrichton · 2017-02-05T21:31:40Z

re-r? @michaelwoerister

michaelwoerister · 2017-02-09T14:46:28Z

@bors r+

Thanks @tromey!

bors · 2017-02-09T14:46:29Z

📌 Commit b037c52 has been approved by michaelwoerister

bors · 2017-02-09T17:09:54Z

⌛ Testing commit b037c52 with merge 4053276...

Emit DW_AT_main_subprogram This changes rustc to emit DW_AT_main_subprogram on the "main" program. This lets gdb suitably stop at the user's main in response to "start" (rather than the library's main, which is what happens currently). Fixes #32620 r? michaelwoerister

bors · 2017-02-09T21:58:05Z

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing 4053276 to master...

alexcrichton assigned michaelwoerister Dec 1, 2016

michaelwoerister reviewed Dec 2, 2016

View reviewed changes

tromey force-pushed the main-subprogram branch from 815e039 to 9401dc9 Compare December 22, 2016 23:09

Emit DW_AT_main_subprogram

b037c52

This changes rustc to emit DW_AT_main_subprogram on the "main" program. This lets gdb suitably stop at the user's main in response to "start" (rather than the library's main, which is what happens currently). Fixes rust-lang#32620 r? michaelwoerister

tromey force-pushed the main-subprogram branch from 9401dc9 to b037c52 Compare February 5, 2017 18:04

bors merged commit b037c52 into rust-lang:master Feb 9, 2017

tromey deleted the main-subprogram branch August 1, 2018 16:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emit DW_AT_main_subprogram #38109

Emit DW_AT_main_subprogram #38109

tromey commented Dec 1, 2016

alexcrichton commented Dec 1, 2016

tromey commented Dec 1, 2016

michaelwoerister left a comment

michaelwoerister Dec 2, 2016

tromey Dec 2, 2016

michaelwoerister Dec 2, 2016

arielb1 Dec 2, 2016

michaelwoerister Dec 2, 2016

tromey Dec 2, 2016

michaelwoerister Dec 2, 2016

michaelwoerister Dec 2, 2016

michaelwoerister Dec 2, 2016

tromey commented Dec 2, 2016

michaelwoerister commented Dec 2, 2016

bors commented Dec 4, 2016

tromey commented Dec 20, 2016

tromey commented Dec 22, 2016

michaelwoerister commented Dec 22, 2016

tromey commented Dec 22, 2016

tromey commented Dec 23, 2016

bors commented Dec 31, 2016

alexcrichton commented Feb 4, 2017

tromey commented Feb 5, 2017

cuviper commented Feb 5, 2017

tromey commented Feb 5, 2017

alexcrichton commented Feb 5, 2017

michaelwoerister commented Feb 9, 2017

bors commented Feb 9, 2017

bors commented Feb 9, 2017

bors commented Feb 9, 2017

Emit DW_AT_main_subprogram #38109

Emit DW_AT_main_subprogram #38109

Conversation

tromey commented Dec 1, 2016

alexcrichton commented Dec 1, 2016

tromey commented Dec 1, 2016

michaelwoerister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tromey commented Dec 2, 2016

michaelwoerister commented Dec 2, 2016

bors commented Dec 4, 2016

tromey commented Dec 20, 2016

tromey commented Dec 22, 2016

michaelwoerister commented Dec 22, 2016

tromey commented Dec 22, 2016

tromey commented Dec 23, 2016

bors commented Dec 31, 2016

alexcrichton commented Feb 4, 2017

tromey commented Feb 5, 2017

cuviper commented Feb 5, 2017

tromey commented Feb 5, 2017

alexcrichton commented Feb 5, 2017

michaelwoerister commented Feb 9, 2017

bors commented Feb 9, 2017

bors commented Feb 9, 2017

bors commented Feb 9, 2017