[lld] Wrong section VMA/LMA in case of using linker scripts

[dstepanov]

Summary

These changes are introduced to fix the problem described in:
https://llvm.org/bugs/show_bug.cgi?id=27805
The idea is to add the cycle in the ELF/LinkerScript.cpp:assignAddresses() routine to go through all the sections and set all the addresses correctly.

Submitted as rev.r270090.

[ruiu]

I think this change will make findSection dead function. Please remove.

Can you add a test to demonstrate the issue?

[dstepanov]

That is true the findSection() routine will be dead after this change and
can be removed. I'll update it.

The test case to demonstrate this issue is attached to
https://llvm.org/bugs/show_bug.cgi?id=27805. Or do you mean the test/ELF/
directory should be updated with new issue for this particular case?

[dstepanov]

The findSection() routine is removed.
New linkerscript-repsection-va.s test case is added to check an issue.

[grimar]

Is not that a bug that 2 sections are created in such conditions ? gnu linker behavior creates one .foo output section and that looks correct for me having this matching pattern and section names.

[ruiu]

We cannot say if this is a bug because the linker script has no spec, but it may be considered as a bug. So the thing here is that if the user instructs the linker to create an output section from input sections with various types of attributes (executable, writable, etc.), what the linker should do. GNU linker seems to create a single section anyways. We are currently creating multiple sections, binned by section attributes. It seems that GNU's behavior makes more sense.

I'm okay with this patch since the previous behavior was more buggy than this, though.

[grimar]

In D20378#434465, @ruiu wrote:

We cannot say if this is a bug because the linker script has no spec, but it may be considered as a bug. So the thing here is that if the user instructs the linker to create an output section from input sections with various types of attributes (executable, writable, etc.), what the linker should do. GNU linker seems to create a single section anyways. We are currently creating multiple sections, binned by section attributes. It seems that GNU's behavior makes more sense.

I'm okay with this patch since the previous behavior was more buggy than this, though.

Yes, my comment was in general, have nothing against this patch.
But returning to a bug: linker has ONLY_IF_RO/ONLY_IF_RW commands, so (I did not check, but it seems)
that it is possible to achieve the behavior this patch shows if using them, but probably not the opposite (gnu) one.
That is why I think gnu default behavior is correct as default at in that way we are really should be able to layout in all possible ways.

[ruiu]

LGTM

I'm singing off since this patch itself doesn't at least make things worse, and and the new test make it explicit what we are doing.

[dstepanov]

Just want to add some more clarification here. It looks like for the
current lld code it is an expected behaviour to create two sections. The
main function to create the section key is ELF/Writer.cpp:createKey():
return SectionKey<ELFT::Is64Bits>{OutsecName, Type, Flags, Alignment};
So there are 4 arguments to generate the key. And even more, there is a
small comment about it:

// For SHF_MERGE we create different output sections for each alignment.
// This makes each output section simple and keeps a single level mapping

from

// input to output.

I can't say if it is good in general, but looks like an expected behaviour.

Also two sections can be created even without a linker script. I tried to
describe this case in https://llvm.org/bugs/show_bug.cgi?id=27805. In this
case there are two .rodata sections with different alignment values. The
good thing here is that without linker script another procedure is used to
set the VMA/LMA fields (it just go through all the sections). This approach
can't be used as is in case of the linker script.

[grimar]

Thanks !

Few hints:

1. If you would add "Differential revision: <URL for this page>" (see some other commits for reference), this review would changed it status to "closed" automatically. And history of commits would also be updated for this page. Otherwise you probably want to close this manually now, as current active status is confusing.
2. It is preferable to include commit header, separated from commit summary message when you do a commit.

[dstepanov]

Submitted as rev.r270090.

[emaste]

You can also mark up a revision with rL to have Phabricator link to it: rL270090

ELF/LinkerScript.cpp

 }
 
 template <class ELFT>
-static OutputSectionBase<ELFT> *
-findSection(ArrayRef<OutputSectionBase<ELFT> *> V, StringRef Name) {
-  for (OutputSectionBase<ELFT> *Sec : V)
-    if (Sec->getName() == Name)
-      return Sec;
-  return nullptr;
-}
-
-template <class ELFT>
 void LinkerScript<ELFT>::assignAddresses(
     ArrayRef<OutputSectionBase<ELFT> *> Sections) {
   // Orphan sections are sections present in the input files which

       continue;
     }
 
-    OutputSectionBase<ELFT> *Sec = findSection<ELFT>(Sections, Cmd.SectionName);
-    if (!Sec)
-      continue;
+    // Find all the sections with required name. There can be more than
+    // ont section with such name, if the alignment, flags or type
+    // attribute differs.
+    for (OutputSectionBase<ELFT> *Sec : Sections) {
+      if (Sec->getName() != Cmd.SectionName)
+        continue;
 
       if ((Sec->getFlags() & SHF_TLS) && Sec->getType() == SHT_NOBITS) {
         uintX_t TVA = Dot + ThreadBssOffset;
         TVA = alignTo(TVA, Sec->getAlign());
         Sec->setVA(TVA);
         ThreadBssOffset = TVA - Dot + Sec->getSize();
         continue;
       }
 
       if (Sec->getFlags() & SHF_ALLOC) {
         Dot = alignTo(Dot, Sec->getAlign());
         Sec->setVA(Dot);
         Dot += Sec->getSize();
         continue;
+      }
     }
   }
 }

test/ELF/linkerscript-repsection-va.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
+
+# RUN: echo "SECTIONS {.foo : {*(.foo.*)} }" > %t.script
+# RUN: ld.lld -o %t1 --script %t.script %t
+# RUN: llvm-objdump -section-headers %t1 | FileCheck %s
+# CHECK:      Sections:
+# CHECK-NEXT: Idx Name          Size      Address          Type
+# CHECK-NEXT:   0               00000000 0000000000000000
+# CHECK-NEXT:   1 .foo          00000004 0000000000000158 DATA
+# CHECK-NEXT:   2 .foo          00000004 000000000000015c DATA
+# CHECK-NEXT:   3 .text         00000001 0000000000000160 TEXT DATA
+
+.global _start
+_start:
+ nop
+
+.section .foo.1,"a"
+foo1:
+ .long 0
+
+.section .foo.2,"aw"
+foo2:
+ .long 0