[PATCH] D133201: [LLD][COFF] Fix Bug, occouring if Symbol has no name
Martin Storsjö via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Sep 5 01:48:35 PDT 2022
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
More information about the llvm-commits
mailing list