[PATCH] D69650: [ELF] Suggest extern "C" when the definition is mangled while an undefined reference is not

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: lld/ELF/Relocations.cpp:791-794
+    symtab->forEachSymbol([&](Symbol *sym) {
+      if (canSuggestExternCXX(name, sym->getName()))
+        s = sym;
+    });
----------------
MaskRay wrote:
> dblaikie wrote:
> > MaskRay wrote:
> > > dblaikie wrote:
> > > > If we're only going to make one suggestion, is there any way to short-circuit the forEachSymbol to stop on the first candidate found, rather than searching for a lot of candidates that'll be ignored?
> > > > 
> > > > (equally, should we be printing more candidates, and maybe printing the source line/file they were defined on in the error message to help people find the right one?)
> > > Unfortunately, forEachSymbol cannot be short-circuited (D62381). It was intended for parallel execution but it is not implemented.
> > > 
> > > ------ 
> > > 
> > > > (equally, should we be printing more candidates, and maybe printing the source line/file they were defined on in the error message to help people find the right one?)
> > > 
> > > We suggest the object file name: >>> defined in:
> > > it should have provided some hint...
> > the use of forEachSymbol here isn't thread safe, since it'd racily access 's' - so perhaps forEachSymbol could be changed to allow short-circuiting (since its callers likely depend on it not being parallel for now) & a separate parallel version could be added at some point & callers who want that behavior could be migrated to it
> > 
> > Also, at least as a partial alternative to reduce the cost of demangling, the callback itself could short-circuit:
> > 
> >     for (auto &it : map)
> >       if (!s && canSuggestExternCXX(name, it.first))
> >         s = it.second;
> >     if (!s)
> >       symtab->forEachSymbol([&](Symbol *sym) {
> >         if (!s && canSuggestExternCXX(name, sym->getName()))
> >           s = sym;
> >       });
> > 
> > (unless there's a reason you would like to prefer the last result, or the result from "forEachSymbol" over the result from the map? In which case you can put the forEachSymbol before the for loop over the map (& skip the for loop over the map if you already found a result in the forEachSymbol))
> forEachSymbol is defined as:
> 
>   void forEachSymbol(llvm::function_ref<void(Symbol *)> fn) {
>     for (Symbol *sym : symVector)
>       if (!sym->isPlaceholder())
>         fn(sym);
>   }
> 
> It cannot be short-circuited, but there is no thread safety issue. If you or @ruiu think https://reviews.llvm.org/D62381#1518668 looks good, I can refactor the existing forEachSymbol call sites and send a patch for review.
Not quite following the D62381 connection here - oh, a range-based rather than callback-based iteration over non-placeholder symbols? Sure. I'll leave that up to Rui & can be refactored here (before or after this is submitted)

But the map for loop before the forEachSymbol looks like it could be refactored already - and could avoid doing more suggestion work once it finds the first suggestion, since the first is likely to be as good as the last? (so no point continuing to search once you find the first)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69650