[lld] Provide a hook to customize missing symbols error handling

[serge-sans-paille]

Summary

This is a follow up to https://reviews.llvm.org/D87758, implementing the missing symbol part, as done by binutils.

[MaskRay]

lld/test/ELF/error-handling-script-linux.test

9

You may want to test (default)/--no-demangle with a _Z symbol.

[jhenderson]

Looks reasonable to me, once @MaskRay's comment has been addressed.

lld/Common/ErrorHandler.cpp

243

I take it missing-symbol is already in use in GNU bfd? undefined-symbol otherwise seems like a better name (since the symbol is present, just undefined).

lld/docs/ld.lld.1

189

[MaskRay]

lld/Common/ErrorHandler.cpp

243

Agree that undefined-symbol is more appropriate.

[MaskRay]

Requested changes because

(1) There is a point about missing-symbol -> undefined-symbol
(2) The interaction with correctSpelling is important and should be tested. In the presence of a spelling corrector, I am on the fence whether this script should run.

[serge-sans-paille]

Fixed undefined vs missing symbol

Added demangling test case

[serge-sans-paille]

In D89687#2351415, @MaskRay wrote:

Requested changes because

(1) There is a point about missing-symbol -> undefined-symbol
(2) The interaction with correctSpelling is important and should be tested. In the presence of a spelling corrector, I am on the fence whether this script should run.

Pointers would be greatly appreciated for (2) :-)

[MaskRay]

In D89687#2353121, @serge-sans-paille wrote:

In D89687#2351415, @MaskRay wrote:

Requested changes because

(1) There is a point about missing-symbol -> undefined-symbol
(2) The interaction with correctSpelling is important and should be tested. In the presence of a spelling corrector, I am on the fence whether this script should run.

Pointers would be greatly appreciated for (2) :-)

I filed https://sourceware.org/bugzilla/show_bug.cgi?id=26781 for (2). It is referenced in https://sourceware.org/bugzilla/show_bug.cgi?id=26626#c18
I have seen that Nick has changed 'missing-symbol' to 'undefined-symbol' in GNU ld. Thanks!

[MaskRay]

lld/test/ELF/error-handling-script-linux.test

13

My feeling is that despite --demangle/--no-demangle, the handling script should receive the original symbol name.

[jhenderson]

lld/docs/ld.lld.1

189

Not addressed yet?

lld/test/ELF/error-handling-script-linux.test

13

+1. The script might well try looking for symbols in certain files, for example, in which case it would need to have the original symbol name.

A case could be made for an optional second argument that takes the demangled name, so that the script can print the demangled name without needing to actually do demangling itself.

[MaskRay]

lld/test/ELF/error-handling-script-linux.test

13

I will go for not taking another argument for the unmangled symbol. The linker can support Itanium C++ mangling rules but probably not others. Assuming the error handling script is more tight to the distribution, it knows about the environment better and can support other mangling schemes.

[serge-sans-paille]

Update documentation as asked by @jhenderson
Add a test case with symbol suggestion as suggested by @MaskRay
Always pass mangled symbol name to the script

[jhenderson]

lld/test/ELF/error-handling-script-linux.test

15–20

Did you mean to line up the output for all 5 cases? Looks like the first one isn't lined up.

Also, should the 2nd-5th cases be using -NEXT?

[serge-sans-paille]

Rebase on missing-lib patch now that it landed.
Update test cases and documentation.

[jhenderson]

LGTM, but needs @MaskRay to approve too.

[MaskRay]

In D89687#2353656, @MaskRay wrote:

In D89687#2353121, @serge-sans-paille wrote:

In D89687#2351415, @MaskRay wrote:

Requested changes because

(1) There is a point about missing-symbol -> undefined-symbol
(2) The interaction with correctSpelling is important and should be tested. In the presence of a spelling corrector, I am on the fence whether this script should run.

Pointers would be greatly appreciated for (2) :-)

I filed https://sourceware.org/bugzilla/show_bug.cgi?id=26781 for (2). It is referenced in https://sourceware.org/bugzilla/show_bug.cgi?id=26626#c18
I have seen that Nick has changed 'missing-symbol' to 'undefined-symbol' in GNU ld. Thanks!

My point here has not been resolved so I am still waiting. What shall we do if the linker has an alternative spelling suggestion (typo correction or suggesting extern "C")?

[jhenderson]

In D89687#2371692, @MaskRay wrote:

In D89687#2353656, @MaskRay wrote:

In D89687#2353121, @serge-sans-paille wrote:

In D89687#2351415, @MaskRay wrote:

Requested changes because

(1) There is a point about missing-symbol -> undefined-symbol
(2) The interaction with correctSpelling is important and should be tested. In the presence of a spelling corrector, I am on the fence whether this script should run.

Pointers would be greatly appreciated for (2) :-)

I filed https://sourceware.org/bugzilla/show_bug.cgi?id=26781 for (2). It is referenced in https://sourceware.org/bugzilla/show_bug.cgi?id=26626#c18
I have seen that Nick has changed 'missing-symbol' to 'undefined-symbol' in GNU ld. Thanks!

My point here has not been resolved so I am still waiting. What shall we do if the linker has an alternative spelling suggestion (typo correction or suggesting extern "C")?

The testing highlights that the proposed name correction is printed after the script message, as part of the normal linker error (see the final case). I think this makes sense - we definitely should call the script with the name as described by the actual symbol (not a corrected version), and we also definitely should call the script when there is a spelling suggestion as otherwise it would be inconsistent with the cases where no suggestion could be made. I could maybe see a case for not suggesting the spelling suggestion in the presence of the script, but I personally don't think the extra logic for that is worthwhile - it seems to me like the script could be doing something completely orthoganol to the undefined symbol reporting, so the spelling suggestion would still be useful.

lld/test/ELF/error-handling-script-linux.test

32

Nit: delete extra blank line.

[MaskRay]

In D89687#2372976, @jhenderson wrote:

In D89687#2371692, @MaskRay wrote:

In D89687#2353656, @MaskRay wrote:

In D89687#2353121, @serge-sans-paille wrote:

In D89687#2351415, @MaskRay wrote:

Requested changes because

(1) There is a point about missing-symbol -> undefined-symbol
(2) The interaction with correctSpelling is important and should be tested. In the presence of a spelling corrector, I am on the fence whether this script should run.

Pointers would be greatly appreciated for (2) :-)

I filed https://sourceware.org/bugzilla/show_bug.cgi?id=26781 for (2). It is referenced in https://sourceware.org/bugzilla/show_bug.cgi?id=26626#c18
I have seen that Nick has changed 'missing-symbol' to 'undefined-symbol' in GNU ld. Thanks!

My point here has not been resolved so I am still waiting. What shall we do if the linker has an alternative spelling suggestion (typo correction or suggesting extern "C")?

The testing highlights that the proposed name correction is printed after the script message, as part of the normal linker error (see the final case). I think this makes sense - we definitely should call the script with the name as described by the actual symbol (not a corrected version), and we also definitely should call the script when there is a spelling suggestion as otherwise it would be inconsistent with the cases where no suggestion could be made. I could maybe see a case for not suggesting the spelling suggestion in the presence of the script, but I personally don't think the extra logic for that is worthwhile - it seems to me like the script could be doing something completely orthoganol to the undefined symbol reporting, so the spelling suggestion would still be useful.

In that case, I would worry that the linker may print duplicated information (one for an alternative spelling suggestion and one from the script). The script needs some more information to avoid duplication, which can be confusing to the user.

[jhenderson]

In D89687#2374170, @MaskRay wrote:

In D89687#2372976, @jhenderson wrote:

In D89687#2371692, @MaskRay wrote:

In D89687#2353656, @MaskRay wrote:

In D89687#2353121, @serge-sans-paille wrote:

In D89687#2351415, @MaskRay wrote:

Requested changes because

(1) There is a point about missing-symbol -> undefined-symbol
(2) The interaction with correctSpelling is important and should be tested. In the presence of a spelling corrector, I am on the fence whether this script should run.

Pointers would be greatly appreciated for (2) :-)

I filed https://sourceware.org/bugzilla/show_bug.cgi?id=26781 for (2). It is referenced in https://sourceware.org/bugzilla/show_bug.cgi?id=26626#c18
I have seen that Nick has changed 'missing-symbol' to 'undefined-symbol' in GNU ld. Thanks!

My point here has not been resolved so I am still waiting. What shall we do if the linker has an alternative spelling suggestion (typo correction or suggesting extern "C")?

The testing highlights that the proposed name correction is printed after the script message, as part of the normal linker error (see the final case). I think this makes sense - we definitely should call the script with the name as described by the actual symbol (not a corrected version), and we also definitely should call the script when there is a spelling suggestion as otherwise it would be inconsistent with the cases where no suggestion could be made. I could maybe see a case for not suggesting the spelling suggestion in the presence of the script, but I personally don't think the extra logic for that is worthwhile - it seems to me like the script could be doing something completely orthoganol to the undefined symbol reporting, so the spelling suggestion would still be useful.

In that case, I would worry that the linker may print duplicated information (one for an alternative spelling suggestion and one from the script). The script needs some more information to avoid duplication, which can be confusing to the user.

I disagree. The two messages are printed at different points. From the test output:

script: info: called with undefined-symbol a # script output - not labelled with "error:" or "ld.lld:"
ld.lld: error: undefined symbol: a           # linker error message.
>>> referenced by [[OBJ]]:                   # linker error additional context, with ">>>" used to highlight it is additional context of the message.
>>> did you mean: a_                         # also additional context, similarly highlighted with the ">>>" output, separated from the script output by the error message and "referenced by" context.

I think it's very unlikely that the script output could cause confusion here. There are plenty of things that distinguish the spelling suggestion produced by the linker from the output of the script. Even if there weren't, I would suggest that the spelling correction functionality should be an opt-out command-line option of the linker, rather than any script arguments. That way if a user wants to use a script and the output of the script could potentially create confusion with the spelling correction functionality, they can specify the option, whilst everyone else still gets the suggestion, even if they use the script, without either side needing to add potentially complicated logic to the script.

[serge-sans-paille]

As a user, I would find it confusing if I specify an error handler script and it doesn't get called because there's a spelling suggestion.

[MaskRay]

If the script prints nothing if it cannot find a candidate library, I think having both is fine. If the script prints something, it will be confusing.

If the script prints nothing if it cannot find a candidate library, this patch LGTM.

[jhenderson]

In D89687#2376491, @MaskRay wrote:

If the script prints nothing if it cannot find a candidate library, I think having both is fine. If the script prints something, it will be confusing.

If the script prints nothing if it cannot find a candidate library, this patch LGTM.

(I assume you meant "cannot find a symbol") Surely it's up to the user what the script prints? I really don't think LLD should change it's behaviour based on what the script might or might not do, and that includes what information it passes to the script.

[MaskRay]

[lld] Provide a hook to customize missing symbols error handling

missing -> undefined

lld/test/ELF/error-handling-script-linux.test

9

-unknown-linux can be deleted. This is generic ELF behavior applying to other OSes (e.g. FreeBSD)

[serge-sans-paille]

Thanks a lot @MaskRay and @jhenderson for the careful reviews. I've learnt a lot in the process \o/

lld/Common/ErrorHandler.cpp

if (errorHandlingScript.empty()) {
     return;
   }
   SmallVector<StringRef, 4> scriptArgs;
   scriptArgs.push_back(errorHandlingScript);
   switch (tag) {
   case ErrorTag::LibNotFound:
     scriptArgs.push_back("missing-lib");
     break;
+  case ErrorTag::SymbolNotFound:
+    scriptArgs.push_back("missing-symbol");
+    break;
   default:
     llvm_unreachable("unsupported ErrorTag");
   }
   scriptArgs.insert(scriptArgs.end(), args.begin(), args.end());
   int res = llvm::sys::ExecuteAndWait(errorHandlingScript, scriptArgs);
   switch (res) {
   case 0:
     error(msg);

lld/ELF/Relocations.cpp

static void reportUndefinedSymbol(const UndefinedDiag &undef,
   if (sym.getName().startswith("_ZTV"))
     msg +=
         "\n>>> the vtable symbol may be undefined because the class is missing "
         "its key function (see https://lld.llvm.org/missingkeyfunction)";
 
   if (undef.isWarning)
     warn(msg);
   else
-    error(msg);
+    error(msg, ErrorTag::SymbolNotFound, {toString(sym)});
 }
 
 template <class ELFT> void elf::reportUndefinedSymbols() {
   // Find the first "undefined symbol" diagnostic for each diagnostic, and
   // collect all "referenced from" lines at the first diagnostic.
   DenseMap<Symbol *, UndefinedDiag *> firstRef;
   for (UndefinedDiag &undef : undefs) {
     assert(undef.locs.size() == 1);

lld/docs/ld.lld.1

 .It Fl -error-limit Ns = Ns Ar value
 Maximum number of errors to emit before stopping.
 A value of zero indicates that there is no limit.
 .It Fl -error-unresolved-symbols
 Report unresolved symbols as errors.
 .It Fl -error-handing-script Ns = Ns Ar script_path
 Call script
 .Ar script_path
-when failing to find a library, with
+upon some error, with
 .Ar tag
-as first argument, and the name of the missing library as second argument.
-The script is expected to return 0 on success, 1 if
-.Ar tag
-is not supported. Any other value is considered a generic error.
+as first argument, and an extra parameter as second argument. The script is expected to return 0 on success, any other value is considered a generic error.
 .Ar tag
 may be
-.Cm missing-lib.
+.Cm missing-lib
+followed by the name of the missing library, or
+.Cm missing-symbol
+followed by the name of the missing symbol.
 .It Fl -execute-only
 Mark executable sections unreadable.
 This option is currently only supported on AArch64.
 .It Fl -exclude-libs Ns = Ns Ar value
 Exclude static libraries from automatic export.
 .It Fl -export-dynamic , Fl E
 Put symbols in the dynamic symbol table.
 .It Fl -export-dynamic-symbol Ns = Ns Ar glob

lld/include/lld/Common/ErrorHandler.h

 // We wrap stdout and stderr so that you can pass alternative stdout/stderr as
 // arguments to lld::*::link() functions.
 extern llvm::raw_ostream *stdoutOS;
 extern llvm::raw_ostream *stderrOS;
 
 llvm::raw_ostream &outs();
 llvm::raw_ostream &errs();
 
-enum class ErrorTag { LibNotFound };
+enum class ErrorTag { LibNotFound, SymbolNotFound };
 
 class ErrorHandler {
 public:
   uint64_t errorCount = 0;
   uint64_t errorLimit = 20;
   StringRef errorLimitExceededMsg = "too many errors emitted, stopping now";
   StringRef errorHandlingScript;
   StringRef logName = "lld";

lld/test/ELF/error-handling-script-linux.test

 #!/bin/sh
 # REQUIRES: x86
 # UNSUPPORTED: system-windows
 
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux /dev/null -o %t0.o
 # RUN: not ld.lld -o /dev/null -lidontexist --error-handling-script=%s %t0.o 2>&1 | FileCheck --check-prefix=CHECK-LIB %s
 # RUN: not ld.lld -o /dev/null -lidontexist --error-handling-script=%s.nope %t0.o 2>&1 | FileCheck --check-prefix=CHECK-SCRIPT-DOES-NOT-EXIST %s
 
+# RUN: echo 'bar: movl a(%rip), %eax' | llvm-mc -filetype=obj -triple=x86_64-unknown-linux - -o %t1.o
+# RUN: not ld.lld -o /dev/null --error-handling-script=%s %t1.o 2>&1 | FileCheck --check-prefix=CHECK-SYM %s
+
 # CHECK-LIB:      script: info: called with missing-lib idontexist
 # CHECK-LIB-NEXT: ld.lld: error: unable to find library -lidontexist
 
 # CHECK-SCRIPT-DOES-NOT-EXIST:      ld.lld: error: unable to find library -lidontexist
 # CHECK-SCRIPT-DOES-NOT-EXIST-NEXT: '{{.*}}': error handling script failed to execute
 
+# CHECK-SYM:      script: info: called with missing-symbol a
+
 echo "script: info: called with $*"