[ELF] Refine warning condition for memory region assignment for non-allocatable section

[andcarminati]

Summary

The warning "ignoring memory region assignment for non-allocatable section" should be generated under the following conditions:

• sections without SHF_ALLOC attribute and,
• presence of input sections or data commands (ByteCommand)

The goal of the change is to reduce spurious warnings that are generated for some output sections that have no input section.

[peter.smith]

Just so I understand I think the main use case for this is where there is a linker script with OutputSections assigned to memory regions that may or may not contain input sections, it will depend on the input sections. As they won't get input sections they won't get the SHF_ALLOC flag and will get a spurious warning?

I agree that we shoudn't warn in that case, although I think the proposed change might suppress too many warnings at the moment.

I note that GNU ld permits the assignment of non SHF_ALLOC sections to MEM regions, assigning them an address if SHF_ALLOC, (although it ignores the empty ones). Although that is a separate issue.

lld/ELF/LinkerScript.cpp

914–919

I think this will also suppress the warning for something like:

.dat : { LONG(0); } > MEM1

Which I note that GNU ld makes SHF_ALLOC but LLD does not, which is a separate issue, but I think we should keep the warning for this case until that can be changed. Perhaps worth warning when the size is non zero or whether the sec contains data generating commands.

lld/test/ELF/linkerscript/memory-nonalloc-no-warn.test

62

Was this meant to be MEM1?

[peter.smith]

Just done a bit more experiments on the output section just containing LONG. It appears that LLD will give it flags based on the preceding OutputSection. I.e. if it follows .text it will get SHF_ALLOC and SHF_EXECINSTR however if it follows .nonalloc it won't get either. In GNU ld it gets SHF_ALLOC and SHF_WRITE.

[andcarminati]

In D151802#4384444, @peter.smith wrote:

Just so I understand I think the main use case for this is where there is a linker script with OutputSections assigned to memory regions that may or may not contain input sections, it will depend on the input sections. As they won't get input sections they won't get the SHF_ALLOC flag and will get a spurious warning?

I agree that we shoudn't warn in that case, although I think the proposed change might suppress too many warnings at the moment.

I note that GNU ld permits the assignment of non SHF_ALLOC sections to MEM regions, assigning them an address if SHF_ALLOC, (although it ignores the empty ones). Although that is a separate issue.

Hi Peter, thank you for the comments. That is the exact point, we are getting spurious warnings for some cases.

lld/test/ELF/linkerscript/memory-nonalloc-no-warn.test

62

Indeed, thank you!

[andcarminati]

This comment was removed by andcarminati.

lld/ELF/LinkerScript.cpp

914–919

The warning for this case (without the patch) depends on the ordering of the commands. For example:

.nonalloc : { *(.nonalloc) } > MEM1
.dat : { LONG(0); } > MEM1
.intvec0_out (VEC_START + 0x0000) : {KEEP (*(.intvec1)) } > MEM2
.intvec1_out (VEC_START + 0x0020) : {KEEP (*(.intvec1)) } > MEM2
.intvec2_out (VEC_START + 0x0040) : {KEEP (*(.intvec2)) } > MEM2
.intvec3_out (VEC_START + 0x0060) : {KEEP (*(.intvec3)) } > MEM2

Gives:

ld.lld: warning: ignoring memory region assignment for non-allocatable section '.nonalloc'
ld.lld: warning: ignoring memory region assignment for non-allocatable section '.dat'
ld.lld: warning: ignoring memory region assignment for non-allocatable section '.intvec0_out'
ld.lld: warning: ignoring memory region assignment for non-allocatable section '.intvec1_out'
ld.lld: warning: ignoring memory region assignment for non-allocatable section '.intvec2_out'

But, if we change the order:

.nonalloc : { *(.nonalloc) } > MEM1
.intvec0_out (VEC_START + 0x0000) : {KEEP (*(.intvec1)) } > MEM2
.intvec1_out (VEC_START + 0x0020) : {KEEP (*(.intvec1)) } > MEM2
.intvec2_out (VEC_START + 0x0040) : {KEEP (*(.intvec2)) } > MEM2
.intvec3_out (VEC_START + 0x0060) : {KEEP (*(.intvec3)) } > MEM2
.dat : { LONG(0); } > MEM1

We get:

ld.lld: warning: ignoring memory region assignment for non-allocatable section '.nonalloc'
ld.lld: warning: ignoring memory region assignment for non-allocatable section '.intvec0_out'
ld.lld: warning: ignoring memory region assignment for non-allocatable section '.intvec1_out'
ld.lld: warning: ignoring memory region assignment for non-allocatable section '.intvec2_out'

With the change, we have a more deterministic output, just for the .nonalloc

What do you think? We can change to keep the warning for the mentioned case, but it will depend on the ordering to be triggered.

[andcarminati]

All current suggestions addressed:

• Warnings kept for data generation commands (ByteCommand)
• Test update (fix MEM -> MEM1)

[peter.smith]

Thanks for the update this looks good to me. If in a later change we can make the ByteCommands set the SHF_ALLOC flag on the OutputSection we can update the test.

Please leave a few days for MaskRay (Code Owner) to comment to see if he has any suggestions.

[andcarminati]

In D151802#4390984, @peter.smith wrote:

Thanks for the update this looks good to me. If in a later change we can make the ByteCommands set the SHF_ALLOC flag on the OutputSection we can update the test.

Please leave a few days for MaskRay (Code Owner) to comment to see if he has any suggestions.

Thank you very much for the review.

[MaskRay]

Thanks for the patch. The code looks good, there are a few needed changes so I clicked "Request Changes".

lld/ELF/LinkerScript.cpp

916

isa<ByteCommand>

Actually, you can rewrite this scope with llvm::any_of

lld/test/ELF/linkerscript/memory-nonalloc-no-warn.test

22

Any reason to test -Map=? To test all sections are present, use llvm-readelf -S

50

Don't indent .section. The .section below is not indented.

72

delete trailing blank lines

[MaskRay]

[ELF] Fix linker warning condition for empty sections

I don't think there is a bug. The new patch makes the behavior closer to GNU ld which may be more desired. "Refine" may be more appropriate.

[ELF] Refine warning condition for memory region assignment for non-allocatable section

lld/test/ELF/linkerscript/memory-nonalloc-no-warn.test

12

Otherwise, this doesn't really test that there is no other warning.

[andcarminati]

All comments from MaskRay were addressed.

[MaskRay]

Some nits

lld/ELF/LinkerScript.cpp

916

Ping for this unaddressed comment. isa<ByteCommand>

lld/test/ELF/linkerscript/memory-nonalloc-no-warn.test

27

File offsets will make test updates difficult if .symtab/.strtab changes. https://maskray.me/blog/2021-08-08-toolchain-testing#the-test-checks-too-much

Perhaps the "Off" column should all use {{.*}} to ignore values, as well "Size" for .comment/.symtab/.shstrtab

[andcarminati]

MaskRay' s comment about the test was addressed.

[andcarminati]

If there are no more comments, can you kindly commit this change, when you have time @MaskRay ?

[MaskRay]

simplify

[MaskRay]

simplify and better test

[MaskRay]

Since you do not use arc diff 'HEAD^' to update the differential, arc patch D151802 doesn't give author name/email information. You need to provide a git commit --amend --author='...' command for me to give credits to you!

[andcarminati]

In D151802#4419200, @MaskRay wrote:

Since you do not use arc diff 'HEAD^' to update the differential, arc patch D151802 doesn't give author name/email information. You need to provide a git commit --amend --author='...' command for me to give credits to you!

Sorry for this missing detail:

git commit --amend --author='Andreu Carminati <[e-mail]>'

Thank you!

lld/ELF/LinkerScript.cpp

 // section in. If found, a pointer to the appropriate memory region is
 // returned in the first member of the pair. Otherwise, a nullptr is returned.
 // The second member of the pair is a hint that should be passed to the
 // subsequent call of this method.
 std::pair<MemoryRegion *, MemoryRegion *>
 LinkerScript::findMemoryRegion(OutputSection *sec, MemoryRegion *hint) {
   // Non-allocatable sections are not part of the process image.
   if (!(sec->flags & SHF_ALLOC)) {
-    if (!sec->memoryRegionName.empty())
+    bool hasInputOrByteCommand =
+        sec->hasInputSections ||
+        llvm::any_of(sec->commands, [](SectionCommand *comm) {
+          return ByteCommand::classof(comm);
+        });
+    if (!sec->memoryRegionName.empty() && hasInputOrByteCommand)
       warn("ignoring memory region assignment for non-allocatable section '" +
            sec->name + "'");
     return {nullptr, nullptr};
   }
 
   // If a memory region name was specified in the output section command,
   // then try to find that region first.
   if (!sec->memoryRegionName.empty()) {

lld/test/ELF/linkerscript/memory-nonalloc-no-warn.test

+# REQUIRES: x86
+
+# RUN: split-file %s %t
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %t/a.s -o %t/a.o
+
+## Check if the linker can generate the final file, containing all non-empty sections,
+## without warnings for .intvec0_out, .intvec1_out1, .intvec2_out, because their
+## input sections are empty. Althoung, the linker must generate a warning for the
+## first ".nonalloc" section and the second .dat, as expected.
+
+# RUN: ld.lld %t/a.o -T %t/a.lds -o %t/a.elf 2>&1 | FileCheck %s --check-prefix=WARN --implicit-check-not=warning:
+
+# WARN: warning: ignoring memory region assignment for non-allocatable section '.nonalloc'
+# WARN: warning: ignoring memory region assignment for non-allocatable section '.dat'
+
+## The output file must include all sections.
+# RUN: llvm-readelf -S %t/a.elf | FileCheck %s
+
+# CHECK:      There are 12 section headers, starting at offset 0x2140:
+# CHECK:      [Nr] Name         Type      Address          Off    Size   ES Flg Lk Inf Al
+# CHECK-NEXT: [ 0]              NULL      0000000000000000 000000 000000 00     0  0   0
+# CHECK-NEXT: [ 1] .nonalloc    PROGBITS  0000000000000000 001064 001000 00 W   0  0   1
+# CHECK-NEXT: [ 2] .comment     PROGBITS  0000000000000000 {{.*}} {{.*}} 01 MS  0  0   1
+# CHECK-NEXT: [ 3] .symtab      SYMTAB    0000000000000000 {{.*}} {{.*}} 18     5  1   8
+# CHECK-NEXT: [ 4] .shstrtab    STRTAB    0000000000000000 {{.*}} {{.*}} 00     0  0   1
+# CHECK-NEXT: [ 5] .strtab      STRTAB    0000000000000000 {{.*}} {{.*}} 00     0  0   1
+# CHECK-NEXT: [ 6] .dat         PROGBITS  0000000000000000 002137 000004 00 W   0  0   1
+# CHECK-NEXT: [ 7] .intvec0_out PROGBITS  0000000000000000 00213b 000000 00 W   0  0   1
+# CHECK-NEXT: [ 8] .intvec1_out PROGBITS  0000000000000000 00213b 000000 00 W   0  0   1
+# CHECK-NEXT: [ 9] .intvec2_out PROGBITS  0000000000000000 00213b 000000 00 W   0  0   1
+# CHECK-NEXT: [10] .intvec3_out PROGBITS  00000000803fe060 001060 000004 00 AX  0  0   1
+# CHECK-NEXT: [11] .text        PROGBITS  00000000803fe064 001064 000000 00 AX  0  0   4
+
+
+#--- a.s
+.global _start
+.text
+_start:
+.section .nonalloc,"w"
+.zero 0x1000
+.section .intvec3,"ax",@progbits
+.long 1
+
+#--- a.lds
+MEMORY {
+  MEM1 : ORIGIN = 0x10000000, LENGTH = 1M
+  MEM2 (rx) : ORIGIN = 0x80000000, LENGTH = 4M
+}
+
+VEC_START = 0x803FE000;
+
+SECTIONS {
+ .nonalloc : { *(.nonalloc) } > MEM
+ .dat : { LONG(0); } > MEM1
+ .intvec0_out (VEC_START + 0x0000) : {KEEP (*(.intvec1)) } > MEM2
+ .intvec1_out (VEC_START + 0x0020) : {KEEP (*(.intvec1)) } > MEM2
+ .intvec2_out (VEC_START + 0x0040) : {KEEP (*(.intvec2)) } > MEM2
+ .intvec3_out (VEC_START + 0x0060) : {KEEP (*(.intvec3)) } > MEM2
+}