Provide a hook to customize missing library error handling

[serge-sans-paille]

Summary

It would be nice to be able to provide a custom script that would help users to find missing libraries. A possible scenario could be:

% clang /tmp/a.c -fuse-ld=lld -loauth -Wl,--error-handling-script=/tmp/addLibrary.py
unable to find library -loauth
looking for relevant packages to provides that library

• liboauth-0.9.7-4.el7.i686
• liboauth-devel-0.9.7-4.el7.i686
• liboauth-0.9.7-4.el7.x86_64
• liboauth-devel-0.9.7-4.el7.x86_64
• pix-1.6.1-3.el7.x86_64

Where addLibrary would be called with the missing library name as first argument (in that case addLibrary.py oauth)

[serge-sans-paille]

Better error handling, more testing, take review into account.

[serge-sans-paille]

lld/Common/ErrorHandler.cpp

233–238

I'm wondering why you prefer the string approach? I like the enum approach because it allows for compile-time checking, while nothing prevents someone from using an unsupported string.

235–244

But using push_back /emplace_back / insert implies a more complex machinery under the hood, while presizing the array + std::copy is easier on the generated code. But it implies some index/size book keeping so I understand your point and will change the code.

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

2 ↗

(On Diff #294652)

I had to duplicate the test case, tried to forge a polyglot file... but that was a mess. I could also just echo "message" but I'd rather test the actual arguments, what do you think?

3 ↗

(On Diff #294652)

It does, but is that pore portable?

[serge-sans-paille]

Realease note + manpage update

[jhenderson]

lld/Common/ErrorHandler.cpp

232

Ping this. Have you considered the case where the error limit is hit and LLD exits? In that case the error handling script will not be called, which I don't think we want (we should call the script for each time an error is hit, not just each time except the last).

233–238

I don't have a strong argument either way, but one advantage of strings is precisely because it means there'll be no compile-time checking, making it easier to add new usages. However, the risk is that a call site will have a typo or similar - at least in the enum case, this point of failure is all in one place.

235–244

I'm not sure it indicates that to me at all. To me push/emplace_back is a standard way of creating a list of things at run time.

247–248

Sorry, maybe I caused confusion with my discussion of unsupported tags. I actually think unsupported tags should be allowed without an error being reported. Having thought more about it, I think that would work best if it's handled by the script. Thus a script can opt into or out of ignoring unsupported tags by returning non-zero for them or not.

The end result is that you can probably get away with deleting case 1.

262

How some variation of this for the default case:
"'" + errorHandlingScript + "': error handling script exited with code " + Twine(Res)

It's quite common for error messages to be prefixed with the file where the problem is, so perhaps the script could be the same. Also, I think it's useful to record the script's exit code in the error message.

You also need a test case for the default label, i.e. where the script exits with some non-zero exit code.

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

1 ↗

(On Diff #295324)

Does this work with an extension other than .bat? It's fine either way, it's just that when I double-click the test in Windows when it has .bat as an extension, it runs the script, which isn't what I want!

Also, it might be worth naming this error-handling-script-windows.bat to make clear the distinction.

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

2 ↗

(On Diff #294652)

Duplicate test case is the best way forward in this case, I think. We do similar things in other cases too.

3 ↗

(On Diff #294652)

I believe lit special cases /dev/null to be the equivalent of an empty file stream. Certainly it's worked for me in the past.

[serge-sans-paille]

Handle review

[MaskRay]

lld/docs/ReleaseNotes.rst

24

This needs to be the ELF Improvements section.

lld/include/lld/Common/ErrorHandler.h

99

StringRef defaults to an empty string. Delete = ""

[MaskRay]

https://sourceware.org/bugzilla/show_bug.cgi?id=26626 @serge-sans-paille Perhaps create a thread on [e-mail] because not many people subscribe bugzilla..

[jhenderson]

You need a test case that shows what happens when the script returns a non-zero exit code.

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

7

Use FileCheck -D to make sure you get the script name right in the error message.

9

12

It's helpful to indent the spaces to make the checked-for text line up.

13

lld/test/ELF/error-handling-script-windows.bat

10–11

This looks like it's missing the script name?

[serge-sans-paille]

Take review into account + add a document specifying the interface expected for the error handling script, so that binutils devs can use it as reference.

[jhenderson]

Looks like you missed context from the latest diff. It also looks like you didn't address my previous set of comments.

lld/Common/ErrorHandler.cpp

249

This needs to happen after the switch statement, or the original message won't be reported in the event of a non-zero exit code.

lld/docs/error_handling_script.rst

5–7 ↗

(On Diff #296686)

(please reflow the suggestion)

16–17 ↗

(On Diff #296686)

I don't think this paragraph is useful. I'd just delete it. The important bits of information in it are already mentioned elsewhere.

22 ↗

(On Diff #296686)

29–30 ↗

(On Diff #296686)

36 ↗

(On Diff #296686)

lld/docs/ld.lld.1

185–192

This description looks stale to me.

[serge-sans-paille]

Update documentation and answer reviewers.

Development have started on ld side using the documentation bundled in that patch as reference.

lld/Common/ErrorHandler.cpp

249

The way I (tried to) address your comment issue was through aggregation of msg in the event of a non-zero exit code. That way there's always a single call to error whatever the error code.

lld/docs/error_handling_script.rst

22 ↗

(On Diff #296686)

According to https://docutils.sourceforge.io/docs/user/rst/quickref.html#literal-blocks that's the way one introduces literal blocks in restructured text.

[jhenderson]

lld/Common/ErrorHandler.cpp

249

Oh, I missed that. Won't that result in only a single "error:" prefix though? The later lines will look a bit odd then, right?

For example:

error: some file: some error message
script.sh: error handling script failed to execute

[MaskRay]

LGTM, but please wait for @jhenderson and Nick on binutils side:)

lld/Common/ErrorHandler.cpp

246

lowercase res

lld/docs/error_handling_script.rst

1 ↗

(On Diff #297827)

If the SPHINX_EXECUTABLE cmake variable is set and SPHINX_OUTPUT_HTML is on, ninja docs-lld-html can build the doc. You can also use rst2html5.py or similar tool too check whether the .rst renders nicely.

[serge-sans-paille]

Synchronize with binutils which also implements missing-symbol.

binutils implementation got commited recently https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=23ae20f5e3afeeab9aa616c63ab3b357668476d5

[serge-sans-paille]

Minor doc and format update

[jhenderson]

It looks like you've uploaded the wrong diff? Also, I'd recommend adding the missing symbol path in a separate patch, to keep this patch smaller.

[serge-sans-paille]

Upload correct diff, with only the missing-lib part

[jhenderson]

None of my most recent comments have been addressed. They need to be before I'm happy for this to land.

[serge-sans-paille]

Tackle (most of ?) @jhenderson comment

• Change the way errors messages are formatted and aggregated, as pointed out
• Update documentation
• Use placeholder in testcase

[jhenderson]

Nearly there.

For some reason this patch doesn't seem to be triggering the pre-merge checks. @MaskRay, any idea why?

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

7–8

You might want to split these lines are they're getting quite long.

lld/test/ELF/error-handling-script-windows.bat

4–5

7

The message checked for here doesn't match what the script actually produces.

[serge-sans-paille]

Split long test lines and update windows test

[jhenderson]

Have you run this on Windows to confirm the Windows test works? If not, I can probably arrange to check that. LGTM once it's been checked.

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

7–8

FWIW, I prefer the format as indicated in my suggested edit, with the | on the first line, and the second line indented with a total of 3 spaces. This means that a user who is reading the first line doesn't need to check the next to see whether the command continues on the next line, whilst a user reading the second line can see from the indentation that it is a continuation of a sequence of commands from the previous line.

[serge-sans-paille]

In D87758#2367699, @jhenderson wrote:

Have you run this on Windows to confirm the Windows test works? If not, I can probably arrange to check that. LGTM once it's been checked.

Yeah, please do!

[serge-sans-paille]

Update multi-line test formatting

[jhenderson]

Requesting changes, since the test is failing on Windows. The problem is that Windows batch files print the command being run before actually running it, so the output is something like this:

C:\llvm\build\tools\lld\test\ELF>echo "script: info: called with missing-lib idontexist"
"script: info: called with missing-lib idontexist"
ld.lld: error: unable to find library -lidontexist

Note that this means the script message appears twice in the output, whilst the LLD error message appears after the second one, which means the CHECK-NEXT for the latter fails. To fix, you probably just want to add the line @echo off immediately before the current echo statement in the windows script.

lld/test/ELF/error-handling-script-windows.bat

7

[serge-sans-paille]

@echo off in windows test.

[jhenderson]

lld/test/ELF/error-handling-script-windows.bat

7

This hasn't been addressed?

14

Nit: I'd add a blank line before this to make the script contents stand out from the lit and FileCheck stuff.

[serge-sans-paille]

Update windows test file

[jhenderson]

LGTM, thanks!