-
Notifications
You must be signed in to change notification settings - Fork 274
Sizeof should fail on incomplete types #5211
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
Conversation
May fix #5108. |
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.
✔️
Passed Diffblue compatibility checks (cbmc commit: 884c360).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/143254568
Codecov Report
@@ Coverage Diff @@
## develop #5211 +/- ##
===========================================
+ Coverage 67.37% 67.38% +<.01%
===========================================
Files 1157 1157
Lines 95087 95092 +5
===========================================
+ Hits 64068 64073 +5
Misses 31019 31019
Continue to review full report at Codecov.
|
src/ansi-c/c_typecheck_expr.cpp
Outdated
to_struct_type(follow_tag(to_struct_tag_type(type))).is_incomplete()) | ||
{ | ||
error().source_location = expr.source_location(); | ||
error() << "calling sizeof for incomplete type: " << to_string(type) |
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.
Looks good in principal, I just have one suggestion, and one query:
- I think you could pretty much copy the compiler error message from gcc for the error message here:
gcc cbmc-test.c
cbmc-test.c:8:31: error: invalid application of 'sizeof' to an incomplete type
'struct foo'
struct foo* thefoo = malloc(sizeof(struct foo));
- This works for incomplete structs - but it won't catch other incomplete types? (e.g. unions, arrays with no dimensions, etc?)
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've change the message and extended the check to unions and enums. Arrays are weird: at the time of typechecking we already see variables of type int[]
as if having type int
.
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.
Hmm, so does that mean you don't even know they are an array at that point? Also, what's the difference at that point between a variable of type int[]
and a variable of type int[10]
- does one have a length attribute somewhere? If you can know it's an array, and know it's length then it's a complete type. If you don't know it's length, then it's incomplete.
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.
My bad: I was looking at a different sizeof
. Now it should work for enums and arrays as well.
6684042
to
a41a250
Compare
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.
Looks good, many thanks :-)
src/ansi-c/c_typecheck_expr.cpp
Outdated
@@ -951,6 +951,21 @@ void c_typecheck_baset::typecheck_expr_sizeof(exprt &expr) | |||
} | |||
else | |||
{ | |||
if( | |||
(type.id() == ID_struct_tag && | |||
to_struct_type(follow_tag(to_struct_tag_type(type))).is_incomplete()) || |
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.
No need for the to_struct_type
etc.
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.
I'd stick the tests into one directory, say "incomplete-sizeof". |
5564772
to
81499c5
Compare
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.
✔️
Passed Diffblue compatibility checks (cbmc commit: a41a250).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/143432884
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 PR failed Diffblue compatibility checks (cbmc commit: 5564772).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/143445520
Status will be re-evaluated on next push.
Common spurious failures include: the cbmc commit has disappeared in the mean time (e.g. in a force-push); the author is not in the list of contributors (e.g. first-time contributors); compatibility was already broken by an earlier merge.
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.
✔️
Passed Diffblue compatibility checks (cbmc commit: 81499c5).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/143449884
And so we check for it when type-checking the `sizeof` expression. Extends to incomplete enums, structs, unions, and arrays (zero-length). Note: previously, the `size-of-expr` function would return zero (sum of the struct components sizes).
81499c5
to
59c6d8f
Compare
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.
LGTM
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.
✔️
Passed Diffblue compatibility checks (cbmc commit: 59c6d8f).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/143553136
And so we check for it when type-checking the
sizeof
expression.Note: previously, the
size-of-expr
function would return zero (sum of the struct components sizes).