call_linker now returns exit_code for better user response on linking_error, fixes #7141#9389
call_linker now returns exit_code for better user response on linking_error, fixes #7141#9389gasche merged 3 commits intoocaml:trunkfrom
Conversation
gasche
left a comment
There was a problem hiding this comment.
I think the approach in this PR is not completely right: it assumes that the exit code is always 127 and the linker called always g++, but this is not always the case. I would propose to:
-
Focus on the exit code only: have a working PR that prints the exit code in the error message, and forget about the linker program name for now; we can always add this in a later PR.
-
Change
Ccomp.call_linkerto return the exit code of the call, instead of just a boolean (this means that all callers have to be adapted to check= 0themselves) -
Add the exit code number as a parameter of the exception:
exception Linking_error of int. (Note: instead of defining a new exception, we just extendLinking_error.)
asmcomp/asmlink.ml
Outdated
| in | ||
| if not (Ccomp.call_linker mode output_name files c_lib) | ||
| then raise(Error Linking_error) | ||
| then raise(Error Linking_error_c_compiler) |
There was a problem hiding this comment.
The error message says that the exit code is 127 and the caller program is g++, but the way you raise the exception does not ensure this: call_linker may return false even with other exit codes, and we don't know which linker was called.
|
In principle, #1727 should have fixed the problem. I still see a bad error my machine, but it looks like a glibc bug where system() returns wrong values (the child gets E2BIG in exec, which is errno 7, which gets reinterpreted as status 7, which is signalled by sibgus, which Sys.command returns as 255). |
833af0a to
a4203e3
Compare
|
@sliquister ah, I had missed this change. (cc @trefis in case). Do you think the change in the present PR, which adds the exit code to the non-127 linking-error case, would possibly be helpful for users? |
|
The only confusing case I have ever seen is with exit code 127 (because that's what system uses whenever anything goes wrong), because the rest of the time the program should print an error. It might be useful if the linker was killed by a signal, but I haven't tried seeing if any error is printed then. |
|
@gasche Can you please review once? |
gasche
left a comment
There was a problem hiding this comment.
From my reading of the reports on this error, the exit code is not often necessary but it may still be useful in some cases (including the tool being buggy and returning something else than 127 when it should): it does not hurt and can help. I am in favor in merging if the style-small style-level nitpicks I made are adressed.
Created additional error
Linking_error_c_compilerfixes #7141