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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: lld/COFF/Symbols.cpp:60
+  if (d == nullptr)
+    return;
   StringRef nameStr =
----------------
j0le wrote:
> mstorsjo wrote:
> > 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.
> Should we instead put an assertion in the constructor of DefinedSynthetic, that the name is not allowed to be empty?
> Like this:
> 
> ```
>   explicit DefinedSynthetic(StringRef name, Chunk *c)
>       : Defined(DefinedSyntheticKind, name), c(c) {
>     assert(!name.empty());
>   }
> ```
> 
> Or maybe even better in the constructor of Symbol?:
> ```
>   explicit Symbol(Kind k, StringRef n = "")
>       : symbolKind(k), isExternal(true), isCOMDAT(false),
>         writtenToSymtab(false), pendingArchiveLoad(false), isGCRoot(false),
>         isRuntimePseudoReloc(false), deferUndefined(false), canInline(true),
>         nameSize(n.size()), nameData(n.empty() ? nullptr : n.data()) {
>     assert((!n.empty() || k <= LastDefinedCOFFKind) &&
>            "If the name is empty, the Symbol must be a DefinedCOFF.");
>   }
> ```
Yes, I think that might make sense (the latter probably is better), if this is the design of the Symbol classes. But I'd like to hear @rnk's opinion on it too (feel free to update the patch, but I'll at least hold off of landing it until @rnk has commented on it).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133201