[PATCH] D133201: [LLD][COFF] Fix Bug, occouring if Symbol has no name

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added a reviewer: rnk.
mstorsjo added inline comments.


================
Comment at: lld/COFF/Symbols.cpp:60
+  if (d == nullptr)
+    return;
   StringRef nameStr =
----------------
This fix kind of changes the assumptions here; the original assumption (see the comment in the assert above in line 57) is that this only ever gets called for DefinedCOFF symbols.


================
Comment at: lld/COFF/Symbols.h:85
+      return StringRef{}; // I don't know, if we can be sure, that nameSize is 0
+                          // in this case.
     return StringRef(nameData, nameSize);
----------------
I'm pretty sure that we can be sure that nameSize is zero here; see line 112 below. (And if we strictly want to keep this, I think we should arrange it like this:
```
if (nameData == nullptr) {
  computeName();
  if (if nameData == nullptr)
    return StringRef{};
}
```

But I think we can leave this out entirely.


================
Comment at: lld/COFF/Writer.cpp:399
   }
-  Defined *d = make<DefinedSynthetic>("", c);
+  Defined *d = make<DefinedSynthetic>("range_extension_thunk", c);
   lastThunk = d;
----------------
j0le wrote:
> some questions:
> - Should we give the symbol a name to fix the bug, instead of changing `Symbol::computeName()`?
> - Is it allowed, that multiple symbols have the same name?
> 
> some questions not really related to the bug, but answers would help me understand LLD better. 
> - why don't we use SymbolTable::addSynthetic()?
> - Do we need to have this symbol in the symbol table?
> - Is it added later to the symbol table?
> 
I think just adding a symbol name to fix the bug is better - since apparently the assumption is that only DefinedCOFF symbols may have a non-empty symbol name.

Yes, it's generally allowed that multiple symbols have the same name (consider static symbols within object files).

We don't use `SymbolTable::addSynthetic` because we don't want to add these symbols into the symbol table (where the name clash with multiple symbols with the same name would be an issue). The exact mechanics of this is a bit of a hazy memory at the moment though, since it's been a few years since I wrote it, and I believe the code might have been restructured at least somewhat since.


================
Comment at: lld/test/COFF/arm-thumb-thunks.s:3
 // RUN: llvm-mc -filetype=obj -triple=thumbv7-windows %s -o %t.obj
-// RUN: lld-link -entry:main -subsystem:console %t.obj -out:%t.exe -verbose 2>&1 | FileCheck -check-prefix=VERBOSE %s
+// RUN: lld-link -entry:main -subsystem:console %t.obj -out:%t.exe -map -verbose 2>&1 | FileCheck -check-prefix=VERBOSE %s
 // RUN: llvm-objdump -d %t.exe --start-address=0x401000 --stop-address=0x401022 | FileCheck --check-prefix=MAIN %s
----------------
I think it might be good to add the same to one of the aarch64 range extension thunk testcases too. From reading the code, it's fairly evident that the same fix automatically applies to both, but it'd be nice to have some test coverage for it too.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133201/new/

https://reviews.llvm.org/D133201