Fix common uses of INSERT AFTER with .bss and .text

[mattico]

Fixes #267
Fixes #266

This fixes two related issues.

1. Named sections are often inserted after .bss or .text in order to have them handled as if they were part of that section. Defining the start/end symbols outside of the section allows this to work.
2. Uninitialized C statics will end up as common symbols which end up in the COMMON input section. If this section is orphaned, it will likely end up placed after .bss. C code often expects these statics to be zero initialized. The first change would cause these symbols to be placed before __ebss so they will get zeroed by the reset handler. Explicitly placing the common symbols into .bss ensures this happens. Users who want uninitialized symbols should use the .uninit section.

See #287 (comment)

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @therealprof (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.

[therealprof]

I'm a bit confused why the label placement inside the section doesn't have the same effect. Is this known linker behaviour causing the label _ebss to be calculated too early?

Anyway this looks fine to me. Any objections @jonas-schievink @adamgreig ?

[adamgreig]

I'm sure this linker script is the most touched file in the repo and I'm always worried a simple PR will cause some undetected bug in six month's time, especially trying to support gcc and lld simultaneously. This looks fine but I also don't understand why it's needed exactly. @mattico, could you elaborate on the motivation for this change?

It looks like maybe the common use of INSERT AFTER to add extra sections from memory.x could cause those sections to be added after .bss and therefore also after _ebss, whereas with it defined outside that block those extra sections are added between .bss and _ebss?

[mattico]

It looks like maybe the common use of INSERT AFTER to add extra sections from memory.x could cause those sections to be added after .bss and therefore also after _ebss, whereas with it defined outside that block those extra sections are added between .bss and _ebss?

I believe this is correct. Also my GCC is doing something like INSERT AFTER for statics for some reason.

I'll try to get a more thorough understanding of the issue.

[mattico]

Alright so.

https://sourceware.org/binutils/docs/ld/Miscellaneous-Commands.html

INSERT [ AFTER | BEFORE ] output_section
This command is typically used in a script specified by '-T' to augment the default SECTIONS with, for example, overlays. It inserts all prior linker script statements after (or before) output_section, and also causes `-T' to not override the default linker script. The exact insertion point is as for orphan sections. See Location Counter. The insertion happens after the linker has mapped input sections to output sections. Prior to the insertion, since '-T' scripts are parsed before the default linker script, statements in the '-T' script occur before the default linker script statements in the internal linker representation of the script. In particular, input section assignments will be made to '-T' output sections before those in the default script. Here is an example of how a '-T' script using INSERT might look:

Not overriding the default linker script was an unexpected turn. However my reading of the rest is that it doesn't matter since our definitions come first for things we care about. As far as where things get inserted...

https://sourceware.org/binutils/docs/ld/Orphan-Sections.html

Orphan sections are sections present in the input files which are not explicitly placed into the output file by the linker script. The linker will still copy these sections into the output file, but it has to guess as to where they should be placed. The linker uses a simple heuristic to do this. It attempts to place orphan sections after non-orphan sections of the same attribute, such as code vs data, loadable vs non-loadable, etc. If there is not enough room to do this then it places at the end of the file.

Along with https://reviews.llvm.org/D42482

Seem to indicate that the linker will place INSERT AFTER somewhere after but not necessarily immediately after.

https://sourceware.org/binutils/docs/ld/Location-Counter.html

If the linker needs to place some input section, e.g. .rodata, not mentioned in the script, it might choose to place that section between .text and .data. You might think the linker should place .rodata on the blank line in the above script, but blank lines are of no particular significance to the linker. As well, the linker doesn't associate the above symbol names with their sections. Instead, it assumes that all assignments or other statements belong to the previous output section, except for the special case of an assignment to .. I.e., the linker will place the orphan .rodata section as if the script was written as follows:

SECTIONS
{
    start_of_text = . ;
    .text: { *(.text) }
    end_of_text = . ;

    start_of_data = . ;
    .rodata: { *(.rodata) }
    .data: { *(.data) }
    end_of_data = . ;
}

This may or may not be the script author's intention for the value of start_of_data. One way to influence the orphan section placement is to assign the location counter to itself, as the linker assumes that an assignment to . is setting the start address of a following output section and thus should be grouped with that section. So you could write:

SECTIONS
{
    start_of_text = . ;
    .text: { *(.text) }
    end_of_text = . ;

    . = . ;
    start_of_data = . ;
    .data: { *(.data) }
    end_of_data = . ;
}

Now, the orphan .rodata section will be placed between end_of_text and start_of_data.

This indicates that when the start/end symbols are outside of the section, the linker may place orphan sections between them, which may not be desirable.

https://sourceware.org/binutils/docs/ld/Simple-Assignments.html

The section of the symbol will be set from the section of the expression; for more information, see Expression Section.

So __ebss is actually in the .bss section, which makes sense. Anything which gets inserted after .bss either automatically or with INSERT AFTER will be inserted after __ebss. Confusingly it seems these inserted-after symbols are still assigned to the .bss section (as shown by objdump) even though they are placed outside the bounds of the section in memory.

Looking at the default linker script from arm-none-eabi-ld --verbose:

  . = .;
  __bss_start = .;
  __bss_start__ = .;
  .bss            :
  {
   *(.dynbss)
   *(.bss .bss.* .gnu.linkonce.b.*)
   *(COMMON)
   /* Align here to ensure that the .bss section occupies space up to
      _end.  Align after .bss to ensure correct alignment even if the
      .bss section disappears because there are no input sections.
      FIXME: Why do we need it? When there is no .bss section, we do not
      pad the .data section.  */
   . = ALIGN(. != 0 ? 32 / 8 : 1);
  }
  _bss_end__ = .; __bss_end__ = .;
  . = ALIGN(32 / 8);
  . = SEGMENT_START("ldata-segment", .);
  . = ALIGN(32 / 8);
  __end__ = .;
  _end = .; PROVIDE (end = .);

It uses the . = .; trick and defines the symbols outside of the section. I can only assume this is done for the benefit of INSERT BEFORE/AFTER, none of the other symbols are defined this way other than _etext.

However for my case, that *(COMMON) directive is the important bit. It turns out that C compilers will sometimes not put uninitialized statics in any input section but instead mark them as common. These common symbols end up in a pseudo-input-section which, if orphaned, may get inserted after .bss. So by moving __ebss out of the section I was accidentally moving it after the common symbols.

My conclusion is that linker behavior is rather poorly specified. I think we should follow the default linker script here because it seems to work. However, the only thing I really want now is to add *(COMMON) for the benefit of C static libraries.

[mattico]

That's a very strange CI failure

 + cargo rustc --target thumbv7m-none-eabi --example alignment --
...
  = note: rust-lld: error: 
          BUG(cortex-m-rt): the LMA of .data is not 4-byte aligned

I can't see how .data is affected at all by the changes. The error only occurs with rustc 1.39.0. Maybe a bug in the included version of lld?

[mattico]

The issue was the alignment of the sections when stored in flash. It seems that old lld had a bug when dealing with aligned sections and > RAM AT>FLASH causing the sections load addresses to overlap.

I added . = ALIGN(4); back into both sections which works round that bug and is more in line with the default linker script. It ensures the actual sections are padded out to the alignment.

We still want the align directives before the end symbols. Sections inserted before them might not have aligned ends and our zero_bss routine needs the symbols to be aligned.

[Dirbaio]

I can confirm this change works to fix the issues, I haven't noticed any bad effects from it either. (I have been using a custom linker script with this change for a while).

Also, the same issue is present with __edata and __erodata. I think all the __e* symbols should be defined after the section, not inside it.

[adamgreig]

Thanks for the thorough investigation!

bors merge

[bors]

Build succeeded:

• build-other (macOS-latest)
• build-other (windows-latest)
• ci-linux (1.39.0)
• ci-linux (stable)
• Rustfmt

[adamgreig]

I'm always worried a simple PR will cause some undetected bug in six month's time

ha ha!

We document that if you want to add new memory sections you can put them in memory.x and then use INSERT AFTER .bss. A main reason to do this is adding sections like CCMRAM, USB RAM, or other bits of RAM that are at weird places in the address map and you'd like to store things in. However as I just found out, this PR of course means __ebss will now point to after those far away sections. So we hardfault at reset! Oops.

I'm not immediately sure what the best remedy is, but if nothing else this will (when released) break anyone with INSERT AFTER .bss in their memory.x for sections that actually shouldn't go after BSS in the memory map. I feel like there's a reason to put INSERT AFTER (to stop the new section going right at the top?), but I can't remember it for sure. It doesn't seem like there's anything else we can suggest as a later "AFTER" target, either, since all our sections now have their end flags after the section body. Anyone have any ideas?

[jordens]

Leaving the __[es].* symbols outside their sections is indeed risky. It also interferes with the implicit LMA region and offset behavior of the surrounding sections (see https://releases.llvm.org/11.0.0/tools/lld/docs/ELF/linker_script.html#output-section-lma).
INSERT BEFORE .uninit or INSERT BEFORE .data works for some use cases (specifically disabled-on-reset RAMs that are NOLOAD and can't be bss and also loading into ITCM in pre_init) as long as #286 is applied. The complex behavior described and changed in https://reviews.llvm.org/D81986 may also be related.

[adamgreig]

@jordens, do you think we should move the __[es]* symbols back inside the sections, then? If we do that, there's no way for user-defined sections to be placed inside bss, though?

[jordens]

The intention behind __[es]* outside their link.x sections seems to be to allow users to inject sections in memory.x and have them handled through the bss zeroing and data loading mechanisms. This seems to specifically occur with linking external binary libs. E.g. and specifically #267

I'm unsure why one would need to do that for the other sections (.text, .rodata. etc) that don't need any loading/zeroing and can just be pushed into whatever output region the user wants (and also where the user wants, but only if the link between section and __[es]* symbols and the very implicit LMA stuff isn't easily broken by INSERT as it is now). Specifically #266.

Maybe the compromise is to have (a) all __s* inside the sections as there appears to be no use case that requires a user-defined section to be before the default ones within the bss or data mechanism, (b) __ebss and __edata outside to support the injection use cases above, and (c) all the other start end symbols inside the sections. That would remove lots of footguns and unnecessary hurdles for various DTCMs/ITCMs/CCRAMs/SRAMs and their custom pre_init configuration and bss/data handlers.
I just tried the current ram2bss example (like #281) after #286 and it would happily extend __ebss to zero everything up to and including the disabled-on-reset RAM2 regardless of its NOLOAD thus breaking.

I suspect that #266 (from the top issue) and maybe also #267 (by extension) was in fact addressed by #286.

And looking through link.x.in, I'm unsure about the need for the address restriction in

cortex-m-rt/link.x.in

Line 104 in 8c90451

.rodata __etext : ALIGN(4)

Lot's of speculation and little hard data, I know.