Skip to content

Conversation

@roberth
Copy link
Collaborator

@roberth roberth commented Apr 25, 2022

  • currentExceptionTypeName has crashed in what appears to be abi::__cxa_current_exception_type()-> ( Crash on service stop hercules-ci/hercules-ci-agent#417). This fixes at least the crash part. (It's still not pretty)

  • Some other calls have the potential to crash similarly; also fixed.

  • Add a few const so we don't have to strdup for no good reason. Should be uninteresting. (I tried - lazily - to keep working with char * but the demangle function did not like that.)

roberth added 2 commits April 25, 2022 11:10
Also adds a few const qualifiers, so we don't have to copy again.
It didn't trigger the new logic, but an extra test case doesn't hurt.
{
std::type_info *type_info = abi::__cxa_current_exception_type();
if (!type_info)
return strdup("<no exception>");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unrelated to the PR (it was like this to begin with), but could you update the comment in Exception.hs referencing free, while it's actually using unsafePackMallocCString, and also note here that these allocations are freed there?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed a bunch of comments of this sort. I think they do, but do they cover your request?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They do, thank you

@roberth roberth merged commit 9dcd07a into fpco:master Apr 26, 2022
@roberth
Copy link
Collaborator Author

roberth commented Apr 26, 2022

Ohh we were waiting for the CI configuration to be done. Shouldn't have merged yet.

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

Successfully merging this pull request may close these issues.

2 participants