[ELF] - Do not remove empty sections referenced in LOADADDR/ADDR commands.

[grimar]

Summary

This is https://bugs.llvm.org//show_bug.cgi?id=38750.

If script references empty sections in LOADADDR/ADDR commands

.empty  : { *(.empty ) }
.text   : AT(LOADADDR (.empty) + SIZEOF (.empty)) { *(.text) }

then an empty section will be removed and LOADADDR/ADDR will evaluate to null.
It is not that user may expect from using of the generic script, what is a common case.

There is a trivial fix for that, so I would do that.

[grimar]

Ping.

[grimar]

Ping.

[ruiu]

I really don't like to add more code to the linker script support to make it more and more compatible with GNU, as there is not official language spec and there will always be an incompatibility. How much important is this patch? I think you should discuss that more before start trying to fix. We should not try to "fix" all the reported "issues".

[grimar]

+Peter, interesting to know his opinion about how much useful this one might be (or not) for embedded soft.

We already don't remove an empty section if its expression contains symbols. It seems consistent step to keep the referenced sections as well ten.
We do produce a broken output here now. Personally, I am not a user of tricky linker script features, but I think I am observing such constructions like
AT(ADDR (.section_name) + 0xABC) often in scripts in the wild. There is a difference in how do we handle the empty sections in LLD with what bfd do.
Some differences probably will never be fixed, but some can be fixed easily. I do not understand why we would want to leave the situation (fixed by this patch) "as is" when we:

1. Already spent some time on fixing the others different cases relative to empty sections to improve consistency with the bfd.
2. Silently produce a broken output now and reporting the error would be more complicated than actual fix perhaps.
3. Have a user who faced and reported the issue (and an unknown number of people who did not yet) and a patch that is trivial and consistent with the overall LLD story about empty sections.

[peter.smith]

I think that this type of script is common in embedded systems. A company will have a product family represented by a common core + variable feature sets, they often have a single linker script that is shared between the projects. This often results in some Output Sections being empty for some of the products. On larger projects it can be difficult to know when this will occur, for example all it takes is one developer to remove the use of a library to silently break a working program.

For this particular example, I don't think it is necessary to keep the .empty section in the ELF file; however I think that it is important for the calculation of the addresses of .text and .data to be either correct or we give an error message. It may be that forcibly keeping .empty is the easiest way of achieving that. An error message is not particularly friendly but it would at least alert the user that they would need to add zero sized sections or symbols to make sure the section was kept.

[grimar]

In D54621#1314195, @peter.smith wrote:

For this particular example, I don't think it is necessary to keep the .empty section in the ELF file; however I think that it is important for the calculation of the addresses of .text and .data to be either correct or we give an error message. It may be that forcibly keeping .empty is the easiest way of achieving that. An error message is not particularly friendly but it would at least alert the user that they would need to add zero sized sections or symbols to make sure the section was kept.

Thanks, Peter! I suddenly noticed that seems it would be not hard to report an error instead. I'll try to prepare the alternative patch doing that to check what is needed.

[grimar]

A version that simply reports an error is D55118.

[peter.smith]

This could also occur with ALIGNOF and SIZEOF. It looks like Script->getOrCreateSectionName() is only called when a part of a script references another, whereas SECTIONS uses Script->createSectionName(). Might it be better to set ForceKeepIfEmpty in that function.

[grimar]

In D54621#1314451, @peter.smith wrote:

This could also occur with ALIGNOF and SIZEOF. It looks like Script->getOrCreateSectionName() is only called when a part of a script references another, whereas SECTIONS uses Script->createSectionName(). Might it be better to set ForceKeepIfEmpty in that function.

Note, I have SIZEOF in the test.

I tried to isolate and minimize the effect, so to keep the empty section only when/if it is really needed.
ALIGNOF and SIZEOF would return 0 for an empty removed section, that seems to be OK just to ignore them.

[grimar]

Ping.

[MaskRay]

Ping :)

[peter.smith]

I think that this change is worth making as it is a reasonable to expect ADDR and LOADADDR to return a valid result even when empty. I note that with wildcards the author may not know ahead of time whether the OutputSection will be empty or not so this doesn't just affect people intentionally using empty OutputSections.

I've take another look and have made some suggestions to make it easier to understand.

ELF/LinkerScript.cpp

827–828

I think it might be best to keep Sec.ExpressionsUseSymbols and introduce a new bool variable called something like UsedInExpression and set that whenever ADDR and LOADADDR uses the name. Even though the effect (keep section) is the same, I think the reason to keep it is different and it gets lost in the comment. Perhaps something like:

// We do not want to remove OutputSections with expressions that reference symbols even if the OutputSection is empty. We want to ensure that the expressions can be evaluated and report an error if they cannot.
if (Sec.ExpressionsUseSymbols)
  return false;

// OutputSections may be referenced by name in ADDR and LOADADDR expressions, as an empty Section can has a valid VMA and LMA we keep the OutputSection to maintain the integrity of the other Expression.
if (Sec.UsedInExpression)
  return false;

[grimar]

Thanks, Peter! I applied your suggestion and improved the test case.

[ruiu]

LGTM

ELF/LinkerScript.cpp

void LinkerScript::assignOffsets(OutputSection *Sec) {
   }
 }
 
 static bool isDiscardable(OutputSection &Sec) {
   // We do not remove empty sections that are explicitly
   // assigned to any segment.
   if (!Sec.Phdrs.empty())
     return false;
 
-  // We do not want to remove sections that reference symbols in address and
-  // other expressions. We add script symbols as undefined, and want to ensure
-  // all of them are defined in the output, hence have to keep them.
+  // We do not want to remove OutputSections with expressions that reference
+  // symbols even if the OutputSection is empty. We want to ensure that the
+  // expressions can be evaluated and report an error if they cannot.
   if (Sec.ExpressionsUseSymbols)
     return false;
 
+  // OutputSections may be referenced by name in ADDR and LOADADDR expressions,
+  // as an empty Section can has a valid VMA and LMA we keep the OutputSection
+  // to maintain the integrity of the other Expression.
+  if (Sec.UsedInExpression)
+    return false;
+
   for (BaseCommand *Base : Sec.SectionCommands) {
     if (auto Cmd = dyn_cast<SymbolAssignment>(Base))
       // Don't create empty output sections just for unreferenced PROVIDE
       // symbols.
       if (Cmd->Name != "." && !Cmd->Sym)
         continue;
 
     if (!isa<InputSectionDescription>(*Base))

ELF/OutputSections.h

public:
   llvm::Optional<std::array<uint8_t, 4>> Filler;
   ConstraintKind Constraint = ConstraintKind::NoConstraint;
   std::string Location;
   std::string MemoryRegionName;
   std::string LMARegionName;
   bool NonAlloc = false;
   bool Noload = false;
   bool ExpressionsUseSymbols = false;
+  bool UsedInExpression = false;
   bool InOverlay = false;
 
   void finalize();
   template <class ELFT> void writeTo(uint8_t *Buf);
   template <class ELFT> void maybeCompress();
 
   void sort(llvm::function_ref<int(InputSectionBase *S)> Order);
   void sortInitFini();

ELF/ScriptParser.cpp

return [=] {
       ExprValue I = Inner();
       I.ForceAbsolute = true;
       return I;
     };
   }
   if (Tok == "ADDR") {
     StringRef Name = readParenLiteral();
     OutputSection *Sec = Script->getOrCreateOutputSection(Name);
+    Sec->UsedInExpression = true;
     return [=]() -> ExprValue {
       checkIfExists(Sec, Location);
       return {Sec, false, 0, Location};
     };
   }
   if (Tok == "ALIGN") {
     expect("(");
     Expr E = readExpr();
if (Script->MemoryRegions.count(Name) == 0) {
       setError("memory region not defined: " + Name);
       return [] { return 0; };
     }
     return [=] { return Script->MemoryRegions[Name]->Length; };
   }
   if (Tok == "LOADADDR") {
     StringRef Name = readParenLiteral();
     OutputSection *Cmd = Script->getOrCreateOutputSection(Name);
+    Cmd->UsedInExpression = true;
     return [=] {
       checkIfExists(Cmd, Location);
       return Cmd->getLMA();
     };
   }
   if (Tok == "MAX" || Tok == "MIN") {
     expect("(");
     Expr A = readExpr();

test/ELF/linkerscript/empty-sections-expressions.test

+# REQUIRES: x86
+# RUN: echo ".text; nop; .data; .byte 0" \
+# RUN:   | llvm-mc -filetype=obj -triple=x86_64-pc-linux - -o %t.o
+# RUN: ld.lld -o %t --script %s %t.o
+# RUN: llvm-readelf -program-headers %t | FileCheck %s
+
+## Check we do not remove the empty output sections used in LOADADDR/ADDR
+## expressions and hence can evaluate the correct addresses.
+
+# CHECK:      Program Headers:
+# CHECK-NEXT:  Type Offset   VirtAddr           PhysAddr
+# CHECK-NEXT:  LOAD 0x001000 0x0000000000080000 0x0000000000080000
+# CHECK-NEXT:  LOAD 0x001001 0x0000000000080001 0x0000000000082000
+
+# CHECK:      Section to Segment mapping:
+# CHECK:       00 .empty .text
+# CHECK-NEXT:  01 .data
+
+SECTIONS {
+ . = 0x00080000;
+ .empty  : { *(.empty ) }
+ .text   : AT(LOADADDR(.empty) + SIZEOF(.empty)) { *(.text) }
+ .data   : AT(ADDR(.empty) + 0x2000) { *(.data) }
+}