Don't set the linkage_name for static variables

[m4b]

For no_mangle statics:

1. Linkage_name no longer set
2. The static variable also no longer has a dwarf namespace scope

This matches C++ output, which does not set the linkage_name and is not scoped:

e.g. c++:

0x000000b6:   DW_TAG_base_type [8]  
                DW_AT_name [DW_FORM_strp]       ( .debug_str[0x00000077] = "long int")
                DW_AT_encoding [DW_FORM_data1]  (DW_ATE_signed)
                DW_AT_byte_size [DW_FORM_data1] (0x08)

0x000000bd:   DW_TAG_variable [9]  
                DW_AT_name [DW_FORM_strp]       ( .debug_str[0x00000053] = "TEST")
                DW_AT_type [DW_FORM_ref4]       (cu + 0x0048 => {0x00000048})
                DW_AT_external [DW_FORM_flag_present]   (true)
                DW_AT_decl_file [DW_FORM_data1] ("/home/m4b/tmp/bad_debug/test.cpp")
                DW_AT_decl_line [DW_FORM_data1] (14)
                DW_AT_location [DW_FORM_exprloc]        (<0x9> 03 40 10 20 00 00 00 00 00 )

0x000000d2:   DW_TAG_namespace [2] *
                DW_AT_name [DW_FORM_strp]       ( .debug_str[0x0000009d] = "std")

and (now) Rust:

0x0000002a:   DW_TAG_variable [2]  
                DW_AT_name [DW_FORM_strp]       ( .debug_str[0x00000046] = "TEST")
                DW_AT_type [DW_FORM_ref4]       (cu + 0x0045 => {0x00000045})
                DW_AT_external [DW_FORM_flag_present]   (true)
                DW_AT_decl_file [DW_FORM_data1] ("/tmp/test.rs")
                DW_AT_decl_line [DW_FORM_data1] (8)
                DW_AT_alignment [DW_FORM_udata] (1)
                DW_AT_location [DW_FORM_exprloc]        (<0x9> 03 c0 4d 06 00 00 00 00 00 )

0x00000040:   DW_TAG_namespace [3] *
                DW_AT_name [DW_FORM_strp]       ( .debug_str[0x0000004b] = "test")

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[shepmaster]

Ping from triage! It's been over 7 days since we heard from reviewer @pnkfelix. Please assign a new reviewer @rust-lang/compiler.

[Zoxc]

@m4b Can you squash your commits into one? I'd also like to see some capitalization on the comments, as the is apparently the style rustc goes for.

[estebank]

Not my area of expertise, but it looks good to merge after @Zoxc comments are addressed.

[m4b]

@Zoxc sure - but just curious doesn’t git allow to squash automatically ? Or does that not integrate with bors? (I know next to nothing about rust + bors merging, review commands, etc)

[Zoxc]

@m4b Squashing commits is a destructive operation (it destroys history), so it would be terrible if it was done automatically.

[m4b]

No I mean when I merge PRs in my repos it allows me to squash their PR into a single commit (essentially what you’re asking me to do manually). Hence my question is why can’t the PR merger just do it. It’s not a big deal was just wondering if there’s some other reason I’m not seeing

[Ixrec]

I think @m4b is referring to the Settings > Options > Merge Button > "Allow squash merging" option that Github has for each repository. We use it all the time where I work, but I think it wasn't on by default and we had to go turn it on (and eventually we even disabled "Allow merge commits" so that we can't ever forget to squash a PR). I have no idea if the Rust team has considered enabling that option, but if contributors are expected to manually squash their PRs today then it'd probably be an improvement.

[shepmaster]

if contributors are expected to manually squash their PRs today

There is no project-wide blanket rule for squashing. I believe the general idea is to have commits that "tell a cohesive story". A series of "oh, fixed that test, addressed review" isn't that useful long term, but neither is a single 10KLOC commit that is "I changed a bunch of stuff". I don't know what was here before, but at first approximation a commit that is "+18 −7" seems unlikely to need multiple commits.

[shepmaster]

why can’t the PR merger just do it

Rust doesn't use the standard GitHub merge process, so any such functionality would need to be added to bors, yes.

[estebank]

As @shepmaster is alluding to sometimes you do want to keep the commit history for a given PR, so making bors do it on its own would be detrimental.

@alexcrichton could you take a look at this PR? It seems reasonable to me.

[Zoxc]

We should also check that this behaves correctly for CodeView debug information.

[m4b]

We should probably just add tests in general for this stuff ;)

[Zoxc]

@m4b Yeah. You can add a gdb test for this at least (see https://github.com/rust-lang/rust/tree/master/src/test/debuginfo), assuming there's some observable difference in gdb.

[alexcrichton]

r? @michaelwoerister

[michaelwoerister]

@m4b, thanks for the PR! The code looks good to me. If you could find a way to test in a src/test/debuginfo that would be good.

[m4b]

I added a file based off of other files in the suggested directory, but I'm going to need someone to explain to me how debuginfo tests work.

Specifically, I changed: https://github.com//m4b/rust/blob/fbac12b980a06c4f2e8efb4eee6974507b38e979/src/test/debuginfo/gdb-pretty-struct-and-enums-pre-gdb-7-7.rs#L23

To not the expected value and running ./x.py test src/test/debuginfo yields a passed test:

test [debuginfo-gdb] debuginfo/vec-slices.rs ... ignored
test [debuginfo-gdb] debuginfo/vec.rs ... ignored
test [debuginfo-gdb] debuginfo/gdb-pretty-struct-and-enums-pre-gdb-7-7.rs ... ok

test result: ok. 1 passed; 0 failed; 108 ignored; 0 measured; 0 filtered out

	finished in 0.385
Build completed successfully in 0:00:05

which makes me suspicious that my test is doing anything in the first place.

[m4b]

@tromey does the rust gdb language setting hardcode namespace assumptions?

I'm seeing really weird behavior by the rust language setting, e.g.:

For a simple rust program:

#[no_mangle]
pub static TEST: u64 = 0xdeadbeef;

pub fn main() {
    println!("{}", TEST);
}

we see this (correct) debuginfo:

.debug_info contents:
0x00000000: Compile Unit: length = 0x0000008a version = 0x0004 abbr_offset = 0x0000 addr_size = 0x08 (next unit at 0x0000008e)

0x0000000b: DW_TAG_compile_unit [1] *
              DW_AT_producer [DW_FORM_strp]     ( .debug_str[0x00000000] = "clang LLVM (rustc version 1.24.0-dev)")
              DW_AT_language [DW_FORM_data2]    (0x001c)
              DW_AT_name [DW_FORM_strp] ( .debug_str[0x00000026] = "/home/m4b/tmp/bad_debug/nomangle.rs")
              DW_AT_stmt_list [DW_FORM_sec_offset]      (0x00000000)
              DW_AT_comp_dir [DW_FORM_strp]     ( .debug_str[0x0000004a] = "/tmp")
              DW_AT_low_pc [DW_FORM_addr]       (0x00000000000063b0)
              DW_AT_high_pc [DW_FORM_data4]     (0x00000092)

0x0000002a:   DW_TAG_variable [2]  
                DW_AT_name [DW_FORM_strp]       ( .debug_str[0x0000004f] = "TEST")
                DW_AT_type [DW_FORM_ref4]       (cu + 0x0040 => {0x00000040})
                DW_AT_external [DW_FORM_flag_present]   (true)
                DW_AT_decl_file [DW_FORM_data1] (0x01)
                DW_AT_decl_line [DW_FORM_data1] (0x02)
                DW_AT_Unknown_88 [DW_FORM_udata]        (1)
                DW_AT_location [DW_FORM_exprloc]        (<0x9> 03 40 49 06 00 00 00 00 00 )

0x00000040:   DW_TAG_base_type [3]  
                DW_AT_name [DW_FORM_strp]       ( .debug_str[0x0000007d] = "u64")
                DW_AT_encoding [DW_FORM_data1]  (0x07)
                DW_AT_byte_size [DW_FORM_data1] (0x08)

0x00000047:   DW_TAG_namespace [4] *
                DW_AT_name [DW_FORM_strp]       ( .debug_str[0x00000054] = "nomangle")

which matches the c++ output for no-mangled static variables.

However, consider this gdb transcript:

(gdb) ptype TEST
No symbol 'TEST' in current context
(gdb) set lang c
(gdb) ptype TEST
Reading in symbols for /home/m4b/tmp/bad_debug/nomangle.rs...done.
type = u64
(gdb) whatis TEST
type = u64
(gdb) info addr TEST
Symbol "TEST" is static storage at address 0x64940.
(gdb) set lang c++
(gdb) info addr TEST
Symbol "TEST" is static storage at address 0x64940.
(gdb) ptype TEST
type = u64
(gdb) break nomangle::main
Breakpoint 1 at 0x63c6: file /home/m4b/tmp/bad_debug/nomangle.rs, line 5.
(gdb) r
Starting program: /tmp/nomangle 
Using PIE (Position Independent Executable) displacement 0x555555554000 for "/tmp/nomangle".
..
Breakpoint 1, nomangle::main () at /home/m4b/tmp/bad_debug/nomangle.rs:5
5	    println!("{}", TEST);
Warning: the current language does not match this frame.
(gdb) ptype TEST
type = u64
(gdb) info addr TEST
Symbol "TEST" is static storage at address 0x5555555b8940.
(gdb) p TEST
$1 = 3735928559
(gdb) set lang rust
(gdb) p TEST
No symbol 'TEST' in current context
(gdb) info addr TEST
Symbol "TEST" is at 0x5555555b8940 in a file compiled without debugging.
(gdb) 

It seems like there is something wrong with the rust language setting in gdb, since even the c and c++ language settings allow the symbol to be properly printed...

[tromey]

This should probably say u64

[m4b]

Ha yup sorry was trying to get tests to fail on my machine as per above comment, will fix

[estebank]

@m4b it did fail in travis, it seems

[m4b]

Yes but it doesn’t on my local machine, using above instructions. I do not know why...

[tromey]

@tromey does the rust gdb language setting hardcode namespace assumptions?

It shouldn't but I think the best thing would be if you could email me the executable and then I'll just debug gdb and see what is going on.

[m4b]

If someone could help explain the CI failure I’m at a total loss. I don’t see any expected output, it says the test panicked (I don’t even know what that means in this context) and as I wrote above I’m unable to even get tests running on my local machine. :headache:

[bors]

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

[tromey]

I debugged this a bit in gdb and I now wonder if a different approach would be preferable for this patch (plus maybe a gdb extension as well).

gdb's expression parser tries to implement rust-like semantics. And, while "no mangle" affects the linkage name, I don't think it affects the name of the item as known to Rust. That is, I think the linkage name is TEST but the Rust name remains module::TEST for other purposes.

If that's so, then I think it would be better to keep TEST in the namespace, but emit a DWARF linkage name to explain the name discrepancy to debuggers. This is in keeping with the general DWARF idea that the debugging info represents a source-ish view.

The gdb extension then would be to allow referring to items by linkage name in the Rust expression parser.

In my test with g++ this is somewhat consistent with what C++ does. (g++ doesn't seem to emit the linkage name, but surely that's a g++ bug)

namespace x {
  extern "C" int zzq;
}

int x::zzq;

pokyo. nm r.o
0000000000000000 B zzq

[m4b]

And, while "no mangle" affects the linkage name, I don't think it affects the name of the item as known to Rust. That is, I think the linkage name is TEST but the Rust name remains module::TEST for other purposes.

This is a very interesting point actually. To extrapolate a bit, iiuc, there's tension here between rusts linkage/crate module and binary linkage models.

E.g., if I define #[no_mangle] pub static FOO: u64 = 0xdeadbeef and you use my crate, to use it you'd have to do mycrate::FOO.

This closely mirrors the usage you gave with the cpp code, so I think I agree that the namespace should be included.

However, for cases like when you have the following in your crate:

extern "C" {
  static FOO: u64;
}

I think this matches the C++ case of an unnamespaced static variable, and this should definitely not have a namespace (it literally gets defined at link time, with that exact name, with no module prefixes and explodes when there are multiple symbols).

So the problem still remains that the rust gdb extension is not capable of printing unnamespaced statics for some reason (which I'll note was the original impetus of the issue).

Since the c and c++ gdb extensions have no issue, and report it as a static variable with debugging info, but when switching to the rust language setting the variable either disappears or reports it has no debugging info, this strongly suggests to me the extension is failing in these cases and should be fixed (and not worked around).

I would be happy to take a look at this if you're busy and could give me an initial pointer to take a look at.

(g++ doesn't seem to emit the linkage name, but surely that's a g++ bug)

Why ? This seems to exactly match expectations and the dwarf spec - its linkage name would have to be just its name, in which case it is omitted. clang also agrees, and does not emit a linkage name.

[m4b]

This is with the latest changes:

(gdb) ptype nomangle::TEST
No symbol 'nomangle::TEST' in current context
(gdb) set lang c++
(gdb) ptype nomangle::TEST
type = u64
(gdb) info addr nomangle::TEST
Symbol "nomangle::TEST" is static storage at address 0x65ec0.
(gdb) set lang rust
(gdb) info addr nomangle::TEST
No symbol "nomangle::TEST" in current context.
(gdb) ptype TEST
No symbol 'TEST' in current context
(gdb) info addr TEST
Symbol "TEST" is at 0x65ec0 in a file compiled without debugging.

Which is the same situation as before w.r.t. gdb not working only in rust language mode.

The dwarf info is correct:

.debug_info contents:
0x00000000: Compile Unit: length = 0x0000008a version = 0x0004 abbr_offset = 0x0000 addr_size = 0x08 (next unit at 0x0000008e)

0x0000000b: DW_TAG_compile_unit [1] *
              DW_AT_producer [DW_FORM_strp]     ( .debug_str[0x00000000] = "clang LLVM (rustc version 1.24.0-dev)")
              DW_AT_language [DW_FORM_data2]    (0x001c)
              DW_AT_name [DW_FORM_strp] ( .debug_str[0x00000026] = "/home/m4b/tmp/bad_debug/nomangle.rs")
              DW_AT_stmt_list [DW_FORM_sec_offset]      (0x00000000)
              DW_AT_comp_dir [DW_FORM_strp]     ( .debug_str[0x0000004a] = "/tmp")
              DW_AT_low_pc [DW_FORM_addr]       (0x0000000000006440)
              DW_AT_high_pc [DW_FORM_data4]     (0x00000092)

0x0000002a:   DW_TAG_namespace [2] *
                DW_AT_name [DW_FORM_strp]       ( .debug_str[0x0000004f] = "nomangle")

0x0000002f:     DW_TAG_variable [3]  
                  DW_AT_name [DW_FORM_strp]     ( .debug_str[0x00000058] = "TEST")
                  DW_AT_type [DW_FORM_ref4]     (cu + 0x007d => {0x0000007d})
                  DW_AT_external [DW_FORM_flag_present] (true)
                  DW_AT_decl_file [DW_FORM_data1]       (0x01)
                  DW_AT_decl_line [DW_FORM_data1]       (0x02)
                  DW_AT_Unknown_88 [DW_FORM_udata]      (1)
                  DW_AT_location [DW_FORM_exprloc]      (<0x9> 03 c0 5e 06 00 00 00 00 00 )

@tromey its clear to me this is entirely a gdb issue that needs to be resolved over there and theres nothing more to be done on rustc's or this PRs end.

I propose to merge this once (I ask again) someone can help to explain the build failure to me.

EDIT:

For completeness, here is the g++ debug info for the c++ file above:

.debug_info contents:
0x00000000: Compile Unit: length = 0x00000679 version = 0x0004 abbr_offset = 0x0000 addr_size = 0x08 (next unit at 0x0000067d)

0x0000000b: DW_TAG_compile_unit [1] *
              DW_AT_producer [DW_FORM_strp]     ( .debug_str[0x00000000] = "clang version 3.9.1 (tags/RELEASE_391/final)")
              DW_AT_language [DW_FORM_data2]    (0x0004)
              DW_AT_name [DW_FORM_strp] ( .debug_str[0x0000002d] = "r.cpp")
              DW_AT_stmt_list [DW_FORM_sec_offset]      (0x00000000)
              DW_AT_comp_dir [DW_FORM_strp]     ( .debug_str[0x00000033] = "/home/m4b/tmp/bad_debug")
              DW_AT_low_pc [DW_FORM_addr]       (0x0000000000000670)
              DW_AT_high_pc [DW_FORM_data4]     (0x00000030)

0x0000002a:   DW_TAG_namespace [2] *
                DW_AT_name [DW_FORM_strp]       ( .debug_str[0x0000017b] = "x")
                DW_AT_decl_file [DW_FORM_data1] (0x01)
                DW_AT_decl_line [DW_FORM_data1] (0x03)

0x00000031:     DW_TAG_variable [3]  
                  DW_AT_name [DW_FORM_strp]     ( .debug_str[0x0000004b] = "ZZQ")
                  DW_AT_type [DW_FORM_ref4]     (cu + 0x0047 => {0x00000047})
                  DW_AT_external [DW_FORM_flag_present] (true)
                  DW_AT_decl_file [DW_FORM_data1]       (0x01)
                  DW_AT_decl_line [DW_FORM_data1]       (0x07)
                  DW_AT_location [DW_FORM_exprloc]      (<0x9> 03 34 10 20 00 00 00 00 00 )

I.e., it's exactly the same as our output (except we add the alignment DW_AT and g++ omits it), hence correct.

Actually, I just realized we don't emit file and line attributes for the namespace; I'm not sure why that would cause a problem rust side, but it is important to note that it is slightly different.

Nevertheless I think this should be merged

[michaelwoerister]

So what exactly is the state here? The build error looks like just the new debuginfo test failing. Do you think there is another version of the test that would pass? Setting the language in the test after "run" looks like a potential race condition. I'm trying to reproduce this locally at the moment.
Otherwise I'm fine with just not setting the linkage name for statics, if that improves things.

[michaelwoerister]

@m4b I cannot reproduce the error locally. You could try to move the set lang call up to before run and see if that helps with CI. If it doesn't, you can just remove the test.

[m4b]

@michaelwoerister ok, will try; thanks for looking into this! Alternatively, could add different print and wait until the debugging issues in gets figured out, and then update back to what it is now.

[tromey]

I think this matches the C++ case of an unnamespaced static variable, and this should definitely not have a namespace (it literally gets defined at link time, with that exact name, with no module prefixes and explodes when there are multiple symbols).

I don't actually know this area of Rust, but the question really revolves around the language semantics of extern "C". if that affects the name as known to Rust, then yes, it shouldn't be namespaced, I think; and then it's a hole in gdb as I didn't consider this situation when writing the gdb support.

(g++ doesn't seem to emit the linkage name, but surely that's a g++ bug)

Why ? This seems to exactly match expectations and the dwarf spec - its linkage name would have to be just its name, in which case it is omitted. clang also agrees, and does not emit a linkage name.

The way I see it is that C++ source refers to this as x::zzq, and the extern "C" affects only the linkage name, which is something external to the language semantics. The DWARF should reflect the language semantics, with the linkage name being a hint for debuggers that something unusual is happening with the translation.

I'd appreciate your thoughts on this. TBH I was surprised that that C++ snippet compiled at all, since my mental model didn't allow for extern C objects in namespaces. So maybe I'm further confused somewhere.

[m4b]

@michaelwoerister I removed the test file; I cannot get it working and I can't repro here so i'm at a loss.

@tromey

I'd appreciate your thoughts on this. TBH I was surprised that that C++ snippet compiled at all, since my mental model didn't allow for extern C objects in namespaces. So maybe I'm further confused somewhere.

I am not even remotely a C++ expert but afaik, extern C in C++ affects the linkage model, not the namespace; so I believe you can have two extern C for a symbol FOO in two different namespaces, (which you'd obviously access differently) but the linkage name must both be FOO (and hence will likely cause a duplicate symbol definition error).

Similar things go for rust, e.g., stuff like this happens: das-labor/panopticon#314 (comment) (tl;dr two different versions of goblin were getting pulled into the binary project, and since they both [no_mangle] the same-named function (wow_so_meta_doge - don't ask :P ), it apparently caused multiple definitions errors in the linker afaiu)

I'll email you the latest binary; as I said, I think the gdb plugin for rust language settings has a bug, and isn't displaying it properly, as the c++/c correctly access everything, as I pasted above.

[philipc]

Based on 10.5/6 of the C++17 draft, C++ allows multiple declarations with C linkage in different namespaces, but these refer to the same function, and there can still only one be definition.

I think a similar thing in rust would be:

mod foo {
    extern {
       pub fn test();
    }
}

pub mod bar {
    #[no_mangle]
    pub fn test() {
        println!("test");
    }
}

fn main() {
    unsafe { foo::test(); }
}

[michaelwoerister]

@bors r+

Thanks for your patience, @m4b!

[bors]

📌 Commit 500dc14 has been approved by michaelwoerister

[bors]

⌛ Testing commit 500dc14 with merge df8dfde...

[bors]

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

[m4b]

@philipc thanks for the example, that aligns with what my understanding for rust cases has come to.

Unfortunately, and I suspect for many others (but this is obviously speculation) when I write no mangle I go to a “c place” in my mind and would never expect the no mangle to have a namespace prefix, and hence when I pop open gdb I’d just expect to write the bare, unmangled name.

I don’t know a good solution/compromise for this at the moment. Maybe there is none and it’s just education that everything is namespaced in rust at the debugging level ?

[tromey]

I've pushed a gdb patch to address one of the issues pointed out by this PR: https://sourceware.org/ml/gdb-patches/2018-01/msg00418.html

I'm not sure I covered everything, so if you know of something more, please ping me. Thanks!