Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't let .debug_gdb_scripts become loadable into memory. #41627

Closed
wants to merge 2 commits into from

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Apr 29, 2017

Fixes #41626 by applying what I did in #35409 for metadata sections, to this section.

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@japaric
Copy link
Member

japaric commented Apr 29, 2017

Thanks for the quick fix @eddyb!

@bors r+

cc @whitequark

@bors
Copy link
Contributor

bors commented Apr 29, 2017

📌 Commit bc6f725 has been approved by japaric

@japaric japaric assigned japaric and unassigned nikomatsakis Apr 29, 2017
@frewsxcv
Copy link
Member

@bors r-

i think this pr caused this fail in this rollup

@eddyb
Copy link
Member Author

eddyb commented Apr 30, 2017

Hah, although I have no idea why it succeeds here.

@whitequark
Copy link
Member

"side-effect free" stares

@japaric
Copy link
Member

japaric commented Apr 30, 2017

@eddyb Travis error

[00:41:03] /checkout/src/test/codegen/gdb_debug_script_load.rs:20:11: error: expected string not found in input
[00:41:03] // CHECK: load volatile i8, i8* getelementptr inbounds ([[B:[[0-9]* x i8]]], [[B]]* @rustc_debug_gdb_scripts_section, i32 0, i32 0), align 1

Looks like this test is looking for the volatile load you just swapped with llvm.used.

/// .debug_gdb_scripts global is referenced, so it isn't removed by the linker.
pub fn insert_reference_to_gdb_debug_scripts_section_global(ccx: &CrateContext, builder: &Builder) {
/// Inserts the .debug_gdb_scripts global into the set of symbols
/// to be placed in `llvm.used`, so it isn't removed by the linker.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"by the linker" - you mean the compiler, right? The linker doesn't know or care about llvm.used stuff but likely it has some special case to preserve the .debug_gdb_scripts section.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, by the linker. I thought that too, and started writing a comment, but turns out that it's not true; LLVM has a separate !llvm.compiler.used for what you say, and !llvm.used is defined to keep the symbol through the linking process as well.

Copy link
Member

@whitequark whitequark Apr 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upon further investigation, on ELF, !llvm.used doesn't actually change anything in the object file, so LangRef is blatantly lying. On MachO, it uses a .no_dead_strip directive. On ELF...

  case MCSA_NoDeadStrip:
    // Ignore for now.
    break;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To add to this, the default linker script on x86_64 Linux has no mention of .debug_gdb_scripts (or a wildcard over all .debug* sections), and the logic of --gc-sections does not appear to special-case any of that either (it only really special-cases .eh_frame, in general; the rest is handled by traversing the graph). So this is interesting.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW I only modified the part of the comment that was obviously wrong to me. Is it possible --gc-sections will strip this section, making it useless?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have confirmed that the linker, called with --gc-sections, will, in fact, remove .debug_gdb_scripts (whether or not !llvm.used is used, of course). Not only that, but it will also gladly remove this section even when used exactly as directed by the gcc manual, and building with gcc and binutils ld!

My conclusion is that, I suppose not completely unexpectedly, this feature is poorly thought out and is not widely supported by toolchains, and Rust's original solution is gross but undeniably effective. Ideally, linker scripts (on ELF platforms) would be updated to do something like...

SECTIONS {
  .debug_gdb_scripts 0 (NOLOAD) : {
    KEEP(*(.debug_gdb_scripts))
  }
}

... but unfortunately, this doesn't work! Without the directive above, the section shows in objdump as:

  1 .debug_gdb_scripts 000000aa  ffff0000  ffff0000  00020000  2**0
                  CONTENTS, ALLOC, LOAD, READONLY, DATA

which is of course not what we want because it's LOAD. With (NOLOAD), however, it's another story entirely:

  1 .debug_gdb_scripts 000000aa  ffff0000  ffff0000  00020000  2**0
                  ALLOC, READONLY

So in a direct contradiction with the ld manual, which claims that:

The linker will process the section normally, but will mark it so that a program loader will not load it into memory. [...] The contents of the [...] section will appear in the linker output file as usual.

the contents of the section does not, in fact, appear in the output file at all.

I'd like to be proven wrong but I am afraid it's not possible to implement this feature.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I have proven myself wrong. If I use the extremely obscure output section type of INFO in the linker script, then exactly the thing that I expect happens!

.debug_gdb_scripts 0 (INFO) : {
  KEEP(*(.debug_gdb_scripts))
}
  1 .debug_gdb_scripts 000000aa  00000000  00000000  00011848  2**0
                  CONTENTS, READONLY, DEBUGGING

I do not believe it is viable to integrate this into upstream rustc and so this PR and its parent issue may be closed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So incidentally I was curious where did the DEBUGGING bit appear, since that's not something gcc (or gas) can do, and I looked at the binutils source code. It looks like all .debug* sections should have their SEC_ALLOC bit removed (and SEC_DEBUGGING bit added)...

  if ((flags & SEC_ALLOC) == 0)
    {
      /* The debugging sections appear to be recognized only by name,
	 not any sort of flag.  Their SEC_ALLOC bits are cleared.  */
      if (name [0] == '.')
	{
	  const char *p;
	  int n;
	  if (name[1] == 'd')
	    p = ".debug", n = 6;
	  // [snip]
	  else
	    p = NULL, n = 0;
	  if (p != NULL && strncmp (name, p, n) == 0)
	    flags |= SEC_DEBUGGING;
	}

... but this is evidently not happening for .debug_gdb_scripts for some reason that I have no doubt is deeply unsettling in more than one way. There's really a large amount of logic in binutils that touches SEC_DEBUGGING, most of which is almost but not exactly identical platform code, and the more I look into it, the less I want to be aware of it. Or be conscious.

@eddyb eddyb closed this Apr 30, 2017
adamgreig pushed a commit to rust-embedded/cortex-m that referenced this pull request Jan 12, 2022
See the discussion in rust-lang/rust#41627
for an explanation of why this is necessary.
reitermarkus pushed a commit to reitermarkus/cortex-m that referenced this pull request May 4, 2022
See the discussion in rust-lang/rust#41627
for an explanation of why this is necessary.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants