[ELF] LinkerScript: Don't assign zero to all regular symbols

[arichardson]

Summary

This fixes an assertion `Align != 0u && "Align can't be 0."'
in llvm::alignTo() when a linker script references a globally
defined variable in an ALIGN() context.

[ruiu]

The BFD linker doesn't accept variables in alignment expressions. I got this error.

nonconstant expression for section alignment

The assertion failure needs fixing, but we could accept it or reject it, so I wonder why you noticed it. Are you actually using this?

[arichardson]

This happens when using the following linker scripts: https://github.com/CTSRD-CHERI/cheribsd/blob/master/lib/libc_cheri/sandbox.ld and https://github.com/CTSRD-CHERI/cherios/blob/master/ldscripts/sandbox.ld and these to work fine with the ancient (2007) FreeBSD BFD version.
The crash was discovered when we tried to link files using sandbox.ld with LLD instead of BFD. I just tried running the testcase with the FreeBSD BFD and you are right that it rejects it. I am not entirely sure what is different in the test case compared to sandbox.ld that would cause it to be rejected.

The reason we use the variable in our linker scripts is that we need either 16 or 32 byte alignment depending on which CPU configuration we run the binary on. To workaround this we could just hardcode it to 32 as that will work for both.
However, being able to use a variable makes it clear that we want to align the section to the size of a capability register and not just some random constant 0x20.

[ruiu]

Thank you for the information. I don't know why the BFD linker rejects this test case, but I don't think we need to understand it. This expression itself makes sense, so we should allow it. You code looks fine. There are only a few nits.

ELF/LinkerScript.cpp

71–73 ↗

(On Diff #95001)

Please add a comment why we set symbol values early.

// We want to set symbol values early if we can. This allows us to use symbols 
// as variables in linker scripts. E.g. doing this allows us to write expressions
// like this: `alignment = 16; . = ALIGN(., alignment)`

I think I prefer SymValue over SymbolValue.

[ruiu]

LGTM

ELF/LinkerScript.cpp

71 ↗

(On Diff #95039)

Add a blank space before comment.

[arichardson]

Could you commit for me please? I don't have commit access.

lld/trunk/ELF/LinkerScript.cpp

template <class ELFT> static SymbolBody *addRegular(SymbolAssignment *Cmd) {
   Symbol *Sym;
   uint8_t Visibility = Cmd->Hidden ? STV_HIDDEN : STV_DEFAULT;
   std::tie(Sym, std::ignore) = Symtab<ELFT>::X->insert(
       Cmd->Name, /*Type*/ 0, Visibility, /*CanOmitFromDynSym*/ false,
       /*File*/ nullptr);
   Sym->Binding = STB_GLOBAL;
   ExprValue Value = Cmd->Expression();
   SectionBase *Sec = Value.isAbsolute() ? nullptr : Value.Sec;
+
+  // We want to set symbol values early if we can. This allows us to use symbols
+  // as variables in linker scripts. Doing so allows us to write expressions
+  // like this: `alignment = 16; . = ALIGN(., alignment)`
+  uint64_t SymValue = Value.isAbsolute() ? Value.getValue() : 0;
   replaceBody<DefinedRegular>(Sym, Cmd->Name, /*IsLocal=*/false, Visibility,
-                              STT_NOTYPE, 0, 0, Sec, nullptr);
+                              STT_NOTYPE, SymValue, 0, Sec, nullptr);
   return Sym->body();
 }
 
 OutputSection *LinkerScript::getOutputSection(const Twine &Loc,
                                               StringRef Name) {
   for (OutputSection *Sec : *OutputSections)
     if (Sec->Name == Name)
       return Sec;

lld/trunk/test/ELF/linkerscript/align.s

 # RUN:   *(.bbb) \
 # RUN:  } \
 # RUN:  .ccc : ALIGN(4096 * 4) \
 # RUN:  { \
 # RUN:   *(.ccc) \
 # RUN:  } \
 # RUN: }" > %t2.script
 # RUN: ld.lld -o %t2 --script %t2.script %t
-# RUN: llvm-objdump -section-headers %t1 | FileCheck %s
+# RUN: llvm-objdump -section-headers %t2 | FileCheck %s
+
+## Check use of variables in align expressions:
+# RUN: echo " VAR = 0x1000; \
+# RUN: __code_base__ = 0x10000; \
+# RUN: SECTIONS { \
+# RUN:  . = __code_base__; \
+# RUN:  .aaa : \
+# RUN:  { \
+# RUN:   *(.aaa) \
+# RUN:  } \
+# RUN:  .bbb : ALIGN(VAR) \
+# RUN:  { \
+# RUN:   *(.bbb) \
+# RUN:  } \
+# RUN:  . = ALIGN(., VAR * 4); \
+# RUN:  .ccc : \
+# RUN:  { \
+# RUN:   *(.ccc) \
+# RUN:  } \
+# RUN:  __start_bbb = ADDR(.bbb); \
+# RUN:  __end_bbb = ALIGN(__start_bbb + SIZEOF(.bbb), VAR); \
+# RUN: }" > %t3.script
+# RUN: ld.lld -o %t3 --script %t3.script %t
+# RUN: llvm-objdump -section-headers %t3 | FileCheck %s
+# RUN: llvm-objdump -t %t3 | FileCheck -check-prefix SYMBOLS %s
+
+# SYMBOLS-LABEL: SYMBOL TABLE:
+# SYMBOLS-NEXT: 0000000000000000         *UND*           00000000
+# SYMBOLS-NEXT: 0000000000014008         .text           00000000 _start
+# SYMBOLS-NEXT: 0000000000010000         *ABS*           00000000 __code_base__
+# SYMBOLS-NEXT: 0000000000001000         *ABS*           00000000 VAR
+# SYMBOLS-NEXT: 0000000000011000         .bbb            00000000 __start_bbb
+# SYMBOLS-NEXT: 0000000000012000         *ABS*           00000000 __end_bbb
 
 .global _start
 _start:
  nop
 
 .section .aaa, "a"
 .quad 0
 
 .section .bbb, "a"
 .quad 0
 
 .section .ccc, "a"
 .quad 0