Skip to content
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

Add an optional stack trace to ArrowError #567

Open
bkietz opened this issue Jul 26, 2024 · 2 comments
Open

Add an optional stack trace to ArrowError #567

bkietz opened this issue Jul 26, 2024 · 2 comments

Comments

@bkietz
Copy link
Member

bkietz commented Jul 26, 2024

It's frequently difficult to tell where error codes were first set, even with the message in ArrowError. It'd be possible to modify NANOARROW_RETURN_NOT_OK() to append file names, line numbers, etc to an ArrowError to produce a stack trace of the error code.

ArrowError would need to be expanded to hold this information, since currently its message can only be 1024 bytes (fine for individual methods but stack traces would exceed that easily). Either that limit could be increased or we could add an optional void* stack_trace; member to the struct whose lifetime is managed separately. (Or we could make ArrowError a type which manages memory but that's probably not desirable because we'd need to add ArrowError{Init,Reset} everywhere.)

Given the added complexity, this would probably need to be an optional feature (controlled by a cmake option()).

@paleolimbot
Copy link
Member

It's worth noting that we don't have an ArrowError everywhere we might want a stacktrace (e.g., functions that can only fail with ENOMEM tend to not accept an ArrowError*). Basically everywhere we do a return <something that is not NANOARROW_OK> should maybe turn into ERROR_RETURN(<something that is not NANOARROW_OK>). The whole stack is nice but even just knowing where the first one was lets you set a breakpoint.

but that's probably not desirable because we'd need to add ArrowError{Init,Reset} everywhere.)

Yes, I think that would be disruptive (although some day it might be the right move, as a 1023 character error might be limiting eventually).

@bkietz
Copy link
Member Author

bkietz commented Jul 26, 2024

An even more disruptive change (but one which only requires deleting code): If we make ArrowError a thread local singleton, we don't need to pass it around as a parameter. There should only be one ArrowError in any stack anyway IIUC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants