[ELF] - Introduce synthetic file for linker script symbols.

[grimar]

Summary

I suggest assigning a synthetic file for linker script and --defsym symbols, that would allow enabling
our toString logic to work with them properly.

That is useful for the following things:

1. We can print --defsym and linker script files in -cref. (LLD currently prints <internal> instead of a valid location)
2. Useful for error reporting.
3. Useful for debugging. (Not so important, but nice to have for free).

The result of such change is shown in the test cases.

[• espindola]

I like the general direction.

ELF/LTO.cpp

167

Can we now say what the comment says: !isa<BitcodeFile>(Sym->File)?

ELF/ScriptParser.cpp

270

Why this change?

[ruiu]

I'm not too excited about doing this. It looks like it opens a can of worms, even if the change itself is small. Adding a new file type is a huge thing that shouldn't be done lightly.

[ruiu]

I mean unless you can show that this is super useful, I'd oppose to adding a new file type. If it is just mildly useful, we should do it in a different way (in case you really need it) or just leave it alone (otherwise).

[• espindola]

In D45375#1060202, @ruiu wrote:

I mean unless you can show that this is super useful, I'd oppose to adding a new file type. If it is just mildly useful, we should do it in a different way (in case you really need it) or just leave it alone (otherwise).

Most lld symbols have files in them. There is a special "kind" that is represented null pointer, which is somewhat annoying to handle.

That is the main reason I like this change: It makes more symbols have a non-null File. In fact, I think a simple followup could use SyntheticFile for symbols like GLOBAL_OFFSET_TABLE and we would never have to handle a null File again.

[grimar]

In D45375#1060199, @ruiu wrote:

I'm not too excited about doing this. It looks like it opens a can of worms, even if the change itself is small.

I think it is opened now. See what we have with null input file:

1. We crash (see: D45440. That was a quick find, looking for more). It is hard to avoid such bugs in future

because having the null value for File is really not obvious.

2. We are not always able to report locations in errors properly (see relocation-relative-absolute.s of this patch),

it would simply report `<internal> location for script symbol.

3. Our -cref prints broken locations for linker script symbols. (cref.s test case of this patch).
4. Our -trace-symbol is broken for linker script symbols. I'll update the diff with the test case.
5. Our checks in code are more complicated (see in this patch).

We can avoid adding the new file class btw. Just a new type would be enough. I'll update the patch soon.

ELF/ScriptParser.cpp

270

Otherwise in cref.s we would output -defsym:1 for Location
(with that change it prints -defsym). Just a bit nicer.
(see ScriptLexer::getCurrentLocation() https://github.com/llvm-mirror/lld/blob/master/ELF/ScriptLexer.cpp#L69).

[grimar]

• Simplify, added one more test case.

[• espindola]

Please rebase.

ELF/InputFiles.h

73

clang-format

ELF/LTO.cpp

167

You should keep the comment about the section symbol being null for bitcode symbols, that is why we need the isa<BitcodeFile>

[grimar]

• Addressed review comments.

[• espindola]

I think I am OK with this.

Rui, what do you think? Would you be OK if it changed every case where File is nullptr to use a synthetic file?

[ruiu]

I'm still in doubt if this is overall a win. At least this patch itself doesn't demonstrate the benefit of having an extra type of file, and I doubt if it will be. Can this simplify things significantly? I think I'm not convinced that we want this one.

[• espindola]

In D45375#1065043, @ruiu wrote:

I'm still in doubt if this is overall a win. At least this patch itself doesn't demonstrate the benefit of having an extra type of file, and I doubt if it will be. Can this simplify things significantly? I think I'm not convinced that we want this one.

I think it can be changed so that we have a reference to a InputFile (or at least know that the pointer is never null).

The reality is that we already have Synthetic files, we just have a very special representation for them: a null pointer.

[ruiu]

The reality is that we already have Synthetic files, we just have a very special representation for them: a null pointer.

I see what you are saying. But it's not really that uncomfortable to me. It is matter of degree, but if we add a new type of a file, I'd like to see a larger benefit.

[• espindola]

In D45375#1065056, @ruiu wrote:

The reality is that we already have Synthetic files, we just have a very special representation for them: a null pointer.

I see what you are saying. But it's not really that uncomfortable to me. It is matter of degree, but if we add a new type of a file, I'd like to see a larger benefit.

Would never having a null InputFile be sufficient?

[ruiu]

Would never having a null InputFile be sufficient?

Like assigning a dummy ObjFile or something to synthetic symbols?

[• espindola]

In D45375#1065066, @ruiu wrote:

Would never having a null InputFile be sufficient?

Like assigning a dummy ObjFile or something to synthetic symbols?

Yes. With this patch we can have a single InternalFile created with createSyntheticFile("<internal>").

[MaskRay]

I recently came across the <internal> issue, and was just reminded of the presence of the patch. I am in favor of this change.

Currently we had 7 states: nullptr, ObjKind, SharedKind, LazyObjKind, ArchiveKind, BitcodeKind, BinaryKind.
With this change, we still have 7: SyntheticKind, ObjKind, SharedKind, LazyObjKind, ArchiveKind, BitcodeKind, BinaryKind.

A nice property is that InputSectionBase::file and Symbol::file will never be null.

[peter.smith]

I agree, I think that a synthetic file has some benefits over using nullptr. I have worked on a linker that did that in the past and it was a useful property to have if no Symbol had a nullptr File. There were cases where one slipped through and it caused a crash, so some extra asserts may be needed.

Some comments on this particular implementation:

• I suggest not creating separate files for each linkerscript defined symbol, with the location encoded in the filename. To me that isn't what I'd expect if I asked for the filename, I'd expect just the filename of the linkerscript that contained it. If we want to have extra location information such as line numbers for linker scripts, or Section + Offset etc. for symbols defined in objects we can add them on via some other means. Having a single file per linker script has some advantages in comparing symbols based on which InputFile defined them. Ideally we'd want two symbols defined in the same linkerscript to have the same file.
• I'd also create an <internal> synthetic file for the linker defined symbols like _GLOBAL_OFFSET_ table where there isn't a real file. That would permit us to assume that the file wouldn't be nullptr.

Anyway certainly room for more discussion.

[grimar]

One of the latest comment from Rui about this approach was: "I'm still in doubt if this is overall a win. At least this patch itself doesn't demonstrate the benefit of having an extra type of file, and I doubt if it will be. Can this simplify things significantly? I think I'm not convinced that we want this one."
If something have changed, I can try to ressurect this patch.

[MaskRay]

In D45375#1668979, @grimar wrote:

One of the latest comment from Rui about this approach was: "I'm still in doubt if this is overall a win. At least this patch itself doesn't demonstrate the benefit of having an extra type of file, and I doubt if it will be. Can this simplify things significantly? I think I'm not convinced that we want this one."
If something have changed, I can try to ressurect this patch.

mold does use a synthetic file for these synthetic symbols :)

ELF/InputFiles.h

 public:
   enum Kind {
     ObjKind,
     SharedKind,
     LazyObjKind,
     ArchiveKind,
     BitcodeKind,
     BinaryKind,
+    SyntheticKind,
   };
 
+  InputFile(Kind K, MemoryBufferRef M);
+
+  static InputFile *createSyntheticFile(StringRef Name);
+
   Kind kind() const { return FileKind; }
 
   bool isElf() const {
     Kind K = kind();
     return K == ObjKind || K == SharedKind;
   }
 
   StringRef getName() const { return MB.getBufferIdentifier(); }
public:
   // GroupId is used for --warn-backrefs which is an optional error
   // checking feature. All files within the same --{start,end}-group
   // get the same group ID. Otherwise, each file gets a new group
   // ID. For more info, see checkDependency() in SymbolTable.cpp.
   uint32_t GroupId;
   static bool IsInGroup;
 
 protected:
-  InputFile(Kind K, MemoryBufferRef M);
   std::vector<InputSectionBase *> Sections;
   std::vector<Symbol *> Symbols;
 
 private:
   const Kind FileKind;
 
   static uint32_t NextGroupId;
 };

ELF/InputFiles.cpp

 InputFile::InputFile(Kind K, MemoryBufferRef M)
     : MB(M), GroupId(NextGroupId), FileKind(K) {
   // All files within the same --{start,end}-group get the same group ID.
   // Otherwise, a new file will get a new group ID.
   if (!IsInGroup)
     ++NextGroupId;
 }
 
+InputFile *InputFile::createSyntheticFile(StringRef Name) {
+  return make<InputFile>(SyntheticKind, MemoryBufferRef("", Name));
+}
+
 Optional<MemoryBufferRef> elf::readFile(StringRef Path) {
   // The --chroot option changes our virtual root directory.
   // This is useful when you are dealing with files created by --reproduce.
   if (!Config->Chroot.empty() && Path.startswith("/"))
     Path = Saver.save(Config->Chroot + Path);
 
   log(Path);
 

ELF/LTO.cpp

for (const lto::InputFile::Symbol &ObjSym : Obj.symbols()) {
     // 4) Symbols that are defined in bitcode files and used for dynamic linking.
     R.VisibleToRegularObj = Config->Relocatable || Sym->IsUsedInRegularObj ||
                             (R.Prevailing && Sym->includeInDynsym()) ||
                             UsedStartStop.count(ObjSym.getSectionName());
     const auto *DR = dyn_cast<Defined>(Sym);
     R.FinalDefinitionInLinkageUnit =
         (IsExecutable || Sym->Visibility != STV_DEFAULT) && DR &&
         // Skip absolute symbols from ELF objects, otherwise PC-rel relocations
-        // will be generated by for them, triggering linker errors.
-        // Symbol section is always null for bitcode symbols, hence the check
-        // for isElf(). Skip linker script defined symbols as well: they have
-        // no File defined.
-        !(DR->Section == nullptr && (!Sym->File || Sym->File->isElf()));
+        // will be generated by for them, triggering linker errors. Symbol
+        // section is always null for bitcode symbols, hence the check.
+        !(DR->Section == nullptr && !isa<BitcodeFile>(Sym->File));
 
     if (R.Prevailing)
       undefine(Sym);
 
     // We tell LTO to not apply interprocedural optimization for wrapped
     // (with --wrap) symbols because otherwise LTO would inline them while
     // their values are still not final.
     R.LinkerRedefined = !Sym->CanInline;

ELF/LinkerScript.cpp

void LinkerScript::addSymbol(SymbolAssignment *Cmd) {
   // However, if an expression is `x = .`, there's no way to know its
   // value at the moment.
   //
   // 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.Sec ? 0 : Value.getValue();
 
-  replaceSymbol<Defined>(Sym, nullptr, Cmd->Name, STB_GLOBAL, Visibility,
-                         STT_NOTYPE, SymValue, 0, Sec);
+  replaceSymbol<Defined>(Sym, InputFile::createSyntheticFile(Cmd->Location),
+                         Cmd->Name, STB_GLOBAL, Visibility, STT_NOTYPE,
+                         SymValue, 0, Sec);
   Cmd->Sym = cast<Defined>(Sym);
 }
 
 // This function is called from LinkerScript::declareSymbols.
 // It creates a placeholder symbol if needed.
 static void declareSymbol(SymbolAssignment *Cmd) {
   if (!shouldDefineSym(Cmd))
     return;
 
   // We can't calculate final value right now.
   Symbol *Sym;
   uint8_t Visibility = Cmd->Hidden ? STV_HIDDEN : STV_DEFAULT;
   std::tie(Sym, std::ignore) = Symtab->insert(Cmd->Name, /*Type*/ 0, Visibility,
                                               /*CanOmitFromDynSym*/ false,
                                               /*File*/ nullptr);
-  replaceSymbol<Defined>(Sym, nullptr, Cmd->Name, STB_GLOBAL, Visibility,
-                         STT_NOTYPE, 0, 0, nullptr);
+  replaceSymbol<Defined>(Sym, InputFile::createSyntheticFile(Cmd->Location),
+                         Cmd->Name, STB_GLOBAL, Visibility, STT_NOTYPE, 0, 0,
+                         nullptr);
   Cmd->Sym = cast<Defined>(Sym);
   Cmd->Provide = false;
 }
 
 // This method is used to handle INSERT AFTER statement. Here we rebuild
 // the list of script commands to mix sections inserted into.
 void LinkerScript::processInsertCommands() {
   std::vector<BaseCommand *> V;

ELF/MapFile.cpp

for (Symbol *Sym : File->getSymbols()) {
       if (auto *D = dyn_cast<Defined>(Sym))
         if (!D->isLocal() && (!D->Section || D->Section->Live))
           Map[D].insert(File);
     }
   }
 
   // Print out a header.
   outs() << "Cross Reference Table\n\n";
-  print("Symbol", "File");
+  print("Symbol", "Location");
 
   // Print out a table.
   for (auto KV : Map) {
     Symbol *Sym = KV.first;
     SetVector<InputFile *> &Files = KV.second;
 
     print(toString(*Sym), toString(Sym->File));
     for (InputFile *File : Files)
       if (File != Sym->File)
         print("", toString(File));
   }
 }

ELF/ScriptParser.cpp

if (Tok == "ASSERT") {
       Script->SectionCommands.push_back(Cmd);
     } else {
       setError("unknown directive: " + Tok);
     }
   }
 }
 
 void ScriptParser::readDefsym(StringRef Name) {
+  std::string Loc = getCurrentLocation();
   Expr E = readExpr();
   if (!atEOF())
     setError("EOF expected, but got " + next());
-  SymbolAssignment *Cmd = make<SymbolAssignment>(Name, E, getCurrentLocation());
+  SymbolAssignment *Cmd = make<SymbolAssignment>(Name, E, Loc);
   Script->SectionCommands.push_back(Cmd);
 }
 
 void ScriptParser::addFile(StringRef S) {
   if (IsUnderSysroot && S.startswith("/")) {
     SmallString<128> PathData;
     StringRef Path = (Config->Sysroot + S).toStringRef(PathData);
     if (sys::fs::exists(Path)) {

test/ELF/cref.s

 // REQUIRES: x86
 
 // RUN: echo '.global foo; foo:' | llvm-mc -filetype=obj -triple=x86_64-pc-linux - -o %t1.o
 // RUN: echo '.global foo, bar; bar:' | llvm-mc -filetype=obj -triple=x86_64-pc-linux - -o %t2.o
 // RUN: echo '.global zed; zed:' | llvm-mc -filetype=obj -triple=x86_64-pc-linux - -o %ta.o
 // RUN: rm -f %t.a
 // RUN: llvm-ar rcs %t.a %ta.o
 // RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t3.o
 // RUN: ld.lld -shared -o %t1.so %t1.o
 // RUN: ld.lld -o /dev/null %t1.so %t2.o %t3.o %t.a -gc-sections -cref | FileCheck -strict-whitespace %s
 
-//      CHECK: Symbol                                            File
+//      CHECK: Symbol                                            Location
 // CHECK-NEXT: bar                                               {{.*}}2.o
 // CHECK-NEXT:                                                   {{.*}}3.o
 // CHECK-NEXT: foo                                               {{.*}}1.so
 // CHECK-NEXT:                                                   {{.*}}2.o
 // CHECK-NEXT:                                                   {{.*}}3.o
 // CHECK-NEXT: _start                                            {{.*}}3.o
 // CHECK-NEXT: baz                                               {{.*}}3.o
 // CHECK-NEXT: zed                                               {{.*}}.a({{.*}}a.o)

test/ELF/just-symbols-cref.s

 # REQUIRES: x86
 
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
 # RUN: ld.lld %t.o -o %t1.exe -Ttext=0x10000
 
 # RUN: ld.lld -just-symbols=%t1.exe -o %t2.exe -cref | FileCheck %s
 
-# CHECK:      Symbol      File
+# CHECK:      Symbol      Location
 # CHECK-NEXT: bar         {{.*exe}}
 # CHECK-NEXT: foo         {{.*exe}}
 
 .globl foo, bar
 foo:
   ret
 
 .section .data
 .type bar, @object
 .size bar, 40
 bar:
   .zero 40

test/ELF/linkerscript/cref.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
+# RUN: echo "SECTIONS { foo = 1; }" > %t.script
+# RUN: ld.lld -o %t --script %t.script %t.o -cref -defsym=bar=2 | FileCheck %s
+
+# CHECK:      Symbol     Location
+# CHECK-NEXT: bar        -defsym
+# CHECK-NEXT:            {{.*}}.o
+# CHECK-NEXT: foo        {{.*}}.script:1
+# CHECK-NEXT:            {{.*}}.o
+
+.global foo, bar
+.quad foo
+.quad bar

test/ELF/linkerscript/relocation-relative-absolute.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
+# RUN: echo "SECTIONS { answer = 1; }" > %t.script
+# RUN: not ld.lld -o %t --script %t.script %t.o -pie 2>&1 | FileCheck %s
+
+# CHECK:      error: relocation R_X86_64_PLT32 cannot refer to absolute symbol: answer
+# CHECK-NEXT: >>> defined in {{.*}}script:1
+# CHECK-NEXT: >>> referenced by {{.*}}o:(.text+0x1)
+
+.globl _start
+_start:
+
+call answer@PLT

test/ELF/linkerscript/symbol-ordering-file2.s

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
 
 ## Check we do not crash when trying to order linker script symbol.
 
 # RUN: echo "bar" > %t.ord
 # RUN: echo "SECTIONS { bar = 1; }" > %t.script
 # RUN: ld.lld --symbol-ordering-file %t.ord %t.o --script %t.script \
 # RUN:   -o %t.out 2>&1 | FileCheck %s
-# CHECK: warning: <internal>: unable to order absolute symbol: bar
+# CHECK: warning: {{.*}}.script:1: unable to order absolute symbol: bar
 
 ## Check we do not crash when trying to order --defsym symbol.
 
 # RUN: echo "bar" > %t.ord
 # RUN: ld.lld --symbol-ordering-file %t.ord %t.o -defsym=bar=1 \
-# RUN:   -o %t.out 2>&1 | FileCheck %s
+# RUN:   -o %t.out 2>&1 | FileCheck %s --check-prefix=DEFSYM
+# DEFSYM: warning: -defsym: unable to order absolute symbol: bar

test/ELF/linkerscript/trace.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
+
+# RUN: echo "SECTIONS { bar = 1; }" > %t.script
+# RUN: ld.lld %t.o --script %t.script -trace-symbol bar -o %t.out 2>&1 | FileCheck %s
+# CHECK: {{.*}}.script:1: definition of bar