[lld][WebAssembly] Remove redundant check for undefined global (NFC)

[sbc100]

lld/wasm/Relocations.cpp

45

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.

[aheejin]

lld/wasm/Relocations.cpp

45

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?

[sbc100]

we return true uncondition

lld/wasm/Relocations.cpp

45

Yes, any symbol with an explicit importName should always be imported and never have "undefined symbol" reported about it.

[aheejin]

lld/wasm/Relocations.cpp

45

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?

[sbc100]

lld/wasm/Relocations.cpp

45

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.

[aheejin]

lld/wasm/Relocations.cpp

45

1. You suggested to add

if (sym->importName)
  return true;

But doesn't this affect not only functions but all symbols?

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?

[aheejin]

lld/wasm/Relocations.cpp

45

Ah, sorry, I misread your comment. Nevermind the comment above. I can't believe you can't delete comments from Phabricator...

[aheejin]

Address comments

[aheejin]

lld/wasm/Relocations.cpp

34–35

Now that this function does not specifically target globals, is this comment correct? Can data or tables have explicit import names and return true here?

[sbc100]

lld/wasm/Relocations.cpp

34–35

Yes, anything that has an explicit import name should return true here.

I guess we don't do much testing for those cases because the use of tables outsides of the singleton __indirect_function_table is still basically non-existent.

[sbc100]

lld/wasm/Relocations.cpp

34–35

Maybe change the comment too: // Symbols with explicit import names are always allowed to be undefined at link time.

[aheejin]

Comment fix

[sbc100]

Please confirm all the tests in lld/test/wasm still pass before landing.

[aheejin]

Oops, || -> &&

lld/wasm/Relocations.cpp

if (sym->isHidden() || sym->isLocal())
     return false;
   // With `-Bsymbolic` (or when building an executable) as don't need to use
   // the GOT for symbols that are defined within the current module.
   if (sym->isDefined() && (!config->shared || config->bsymbolic))
     return false;
   return true;
 }
 
 static bool allowUndefined(const Symbol* sym) {
-  // Undefined functions and globals with explicit import name are allowed to be
-  // undefined at link time.
-  if (auto *f = dyn_cast<UndefinedFunction>(sym))
-    if (f->importName || config->importUndefined)
-      return true;
-  if (auto *g = dyn_cast<UndefinedGlobal>(sym))
-    if (g->importName)
-      return true;
-  if (auto *g = dyn_cast<UndefinedGlobal>(sym))
-    if (g->importName)
-      return true;
+  // Symbols with explicit import names are always allowed to be undefined at
+  // link time.
+  if (sym->importName)
+  if (isa<UndefinedFunction>(sym) && config->importUndefined)
+
   return config->allowUndefinedSymbols.count(sym->getName()) != 0;
 }
 
 static void reportUndefined(Symbol *sym) {
   if (!allowUndefined(sym)) {
     switch (config->unresolvedSymbols) {
     case UnresolvedPolicy::ReportError:
       error(toString(sym->getFile()) + ": undefined symbol: " + toString(*sym));