Skip to content

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

Merged
merged 1 commit into from
Jan 16, 2020

Conversation

xbauch
Copy link
Contributor

@xbauch xbauch commented Jan 6, 2020

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).

  • Each commit message has a non-empty body, explaining why the change was made.
  • Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
  • The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/
  • Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
  • My commit message includes data points confirming performance improvements (if claimed).
  • My PR is restricted to a single feature or bugfix.
  • White-space or formatting changes outside the feature-related changed lines are in commits of their own.

@xbauch
Copy link
Contributor Author

xbauch commented Jan 6, 2020

May fix #5108.

Copy link
Contributor

@allredj allredj left a 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-io
Copy link

codecov-io commented Jan 6, 2020

Codecov Report

Merging #5211 into develop will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #5211      +/-   ##
===========================================
+ Coverage    67.37%   67.38%   +<.01%     
===========================================
  Files         1157     1157              
  Lines        95087    95092       +5     
===========================================
+ Hits         64068    64073       +5     
  Misses       31019    31019
Flag Coverage Δ
#cproversmt2 42.66% <100%> (ø) ⬆️
#regression 63.89% <100%> (ø) ⬆️
#unit 31.92% <20%> (-0.01%) ⬇️
Impacted Files Coverage Δ
src/ansi-c/c_typecheck_expr.cpp 73.95% <100%> (+0.1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be84cd1...884c360. Read the comment docs.

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)
Copy link
Contributor

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?)

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

@xbauch xbauch force-pushed the fix/incomplete-sizeof branch from 6684042 to a41a250 Compare January 7, 2020 15:26
Copy link
Contributor

@chrisr-diffblue chrisr-diffblue left a 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 :-)

@@ -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()) ||
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@kroening
Copy link
Member

kroening commented Jan 7, 2020

I'd stick the tests into one directory, say "incomplete-sizeof".

@xbauch xbauch force-pushed the fix/incomplete-sizeof branch from 5564772 to 81499c5 Compare January 7, 2020 16:31
Copy link
Contributor

@allredj allredj left a 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

Copy link
Contributor

@allredj allredj left a 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.

Copy link
Contributor

@allredj allredj left a 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).
@xbauch xbauch force-pushed the fix/incomplete-sizeof branch from 81499c5 to 59c6d8f Compare January 8, 2020 10:11
@xbauch xbauch marked this pull request as ready for review January 8, 2020 10:12
@xbauch xbauch requested a review from tautschnig as a code owner January 8, 2020 10:12
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@allredj allredj left a 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

@kroening kroening merged commit 077b5ee into diffblue:develop Jan 16, 2020
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.

8 participants