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

Fixed StackTrace not being resolved when panic is invalid #1615

Merged
merged 2 commits into from Oct 1, 2018

Conversation

Projects
None yet
2 participants
@Hejsil
Copy link
Member

commented Oct 1, 2018

Fixes #1610

@andrewrk
Copy link
Member

left a comment

I think the idea & test case is good but it should resolve zero bits, not fully resolve the type.

g->stack_trace_type = stack_trace_type_val->data.x_type;
assertNoError(ensure_complete_type(g, g->stack_trace_type));

This comment has been minimized.

Copy link
@andrewrk

andrewrk Oct 1, 2018

Member

I think this could be potentially problematic - get_pointer_to_type only needs to know that the type is not zero bits. (you can see that in the assertion failed message). ensure_complete_type is deprecated - it's the same as type_resolve(type, ResolveStatusSizeKnown). So I think this should be type_resolve(g->stack_trace_type, ResolveStatusZeroBitsKnown).

The assertNoError would be a problem stack_trace_type were a user type, because then a compile error in that type would make this assertion trip. So this is ok as long as we know that a compile error in the StackTrace builtin type will result in a compiler assertion. (I do think that is OK.)

@andrewrk
Copy link
Member

left a comment

LGTM, merge at will!

@Hejsil Hejsil merged commit bc3e99c into master Oct 1, 2018

0 of 4 checks passed

continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@Hejsil Hejsil deleted the fixed-stacktrace-not-resolved branch Oct 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.