[PATCH] D67039: [ELF] Add a spell corrector for "undefined symbol" diagnostics

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked an inline comment as done.
MaskRay added inline comments.


================
Comment at: ELF/Relocations.cpp:721
+  StringRef name = sym.getName();
+  for (size_t i = 0, e = name.size(); i != e + 1; ++i) {
+    // Insert a character before name[i].
----------------
ruiu wrote:
> MaskRay wrote:
> > grimar wrote:
> > > ruiu wrote:
> > > > IIUC, this function costs (N+1)*74 where N is the length of a symbol name. Symbol names can be very long. Doesn't this too slow?
> > > It's enabled for the first two undefined symbols. Even with large N, like 100 it shouldn't probably be too slow for the error reporting case?
> > I think the complexity is fine. I avoid making the size of symtab a factor in the time complexity. Doing mutation in the input is probably fine.
> > 
> > In a large executable, I get a long mangled name of N=4398 bytes.
> > O(74*N) is not too bad.
> Maybe you are right. A symbol that is 4398 bytes long is perhaps an outlier, but even for a 1000 byte long symbol, it is 74,000 hash table lookup, which may not too bad. Did you run this code against a long symbol name? If it doesn't feel sluggish, I'm fine.
I tested linking a large executable. It took 19.8s in total.

For the long symbol (sym.getName().size() = 4379), if I patched `getAlternativeSpelling` to attempt all suggestions before returning, `suggest` was called 665682 times, and it took about 0.76s, a small portion of the whole link time.

(As a comparison, symVector.size() = 4195114.)



Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D67039