-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix exceptions not being inspectable when running binary from PATH #8807
Conversation
Ok I thought alpine was failing because it didn't support |
LGTM, almost wish we'd fix PROGRAM_NAME to always be absolute instead but... :) Here's what I get on my box without it:
If you search for read_dwarf_sections there are a few reports out there :) |
It’s #8738 do not worry about alpine |
I'm not sure this should use I'd also like to make this behaviour more resilient in case the file can't be read at all. For example because the executable doesn't exist anymore. We should make sure to still print a meaningful account of the original error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @straight-shoota, and we already do it for Darwin/Mach-O targets. Let's follow up for ELF based targets!
src/callstack.cr
Outdated
@@ -367,7 +367,7 @@ struct CallStack | |||
@@base_address : UInt64|UInt32|Nil | |||
|
|||
protected def self.read_dwarf_sections | |||
Debug::ELF.open(PROGRAM_NAME) do |elf| | |||
Debug::ELF.open(Process.executable_path || PROGRAM_NAME) do |elf| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debug::ELF.open(Process.executable_path || PROGRAM_NAME) do |elf| | |
return unless path = Process.executable_path | |
Debug::ELF.open(path) do |elf| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if exception_path
is present but File.open
fails?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we're back in the same position, but I think making that more robust should be done in another PR - it'll have to be done for the other executable types as well.
I've added a check for if the file exists (like the MacOS version), unsure of what to do to indicate that there was a problem reading the backtrace, but I think that can be addressed in another PR. |
If the binary is added to $PATH, PROGRAM_NAME is the name of the file, but not the path to read debug information from. This causes a file open error, which is incredibly hard to debug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine, though adding Debug::ELF.open?
which returned nil
without running the block if the open
call failed would be neater.
If the binary is added to $PATH, PROGRAM_NAME is the name of the file,
but not the path to read debug information from. This causes a file open
error, which is incredibly hard to debug. Using
Process.executable_path
willinstead always be the current executable.
A simple demonstration of this issue is:
Build and move this binary somewhere in your
$PATH
:With this patch, the exception is printed as expected: