[PATCH] D111101: [lld][WebAssembly] Remove redundant check for undefined global (NFC)
Heejin Ahn via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Oct 5 00:07:50 PDT 2021
aheejin added inline comments.
================
Comment at: lld/wasm/Relocations.cpp:45
- if (g->importName)
- return true;
return config->allowUndefinedSymbols.count(sym->getName()) != 0;
----------------
aheejin wrote:
> sbc100 wrote:
> > aheejin wrote:
> > > sbc100 wrote:
> > > > aheejin wrote:
> > > > > sbc100 wrote:
> > > > > > Actually there was a recent refactor that made `importName` a (optional) member of the Symbol superclass.. so I think this can be written as:
> > > > > >
> > > > > > ```
> > > > > > if (sym->importName)
> > > > > > return true;
> > > > > > if (isa<UndefinedFunction>(sym) && config->importUndefined)
> > > > > > return true;
> > > > > > ```
> > > > > >
> > > > > > But if you would rather land this as is that is fine too.
> > > > > > if (isa<UndefinedFunction>(sym) && config->importUndefined)
> > > > >
> > > > > Isn't this `||` in the current code?
> > > > >
> > > > > Also, if we return `true` unconditionally when `sym->importName` is defined, I think we return `true` for data or tables. Is that what you intended?
> > > > Yes, any symbol with an explicit `importName` should always be imported and never have "undefined symbol" reported about it.
> > > Which one is correct?
> > > ```
> > > if (isa<UndefinedFunction>(sym) && config->importUndefined)
> > > ```
> > > vs.
> > > ```
> > > if (isa<UndefinedFunction>(sym) || config->importUndefined)
> > > ```
> > > (The current code is like the latter and you suggested the former)
> > >
> > > Also, `config->importUndefined` doesn't affect `UndefinedGlobal`, or `UndefinedTag` I am going to add?
> > I think we should leave `importUndefined` as only effecting functions symbols for now. We could followup by extending it but that would not be NFC.
> >
> > Thus I think the `if (isa<UndefinedFunction>(sym) && config->importUndefined)` matches the original semantics and is that we should do here for now.
> >
> 1. You suggested to add
> ```
> if (sym->importName)
> return true;
> ```
> But doesn't this affect not only functions but all symbols?
>
> 2.
> > I think we should leave `importUndefined` as only effecting functions symbols for now. We could followup by extending it but that would not be NFC.
>
> The current code already handles globals, even twice, one of which I removed. So if we make this only handle functions that will not be NFC instead.. No?
Ah, sorry, I misread your comment. Nevermind the comment above. I can't believe you can't delete comments from Phabricator...
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D111101/new/
https://reviews.llvm.org/D111101
More information about the llvm-commits
mailing list