Skip to content

Don't warn on array_of_expr with zero length #1013

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 2 commits into from
Jul 3, 2017

Conversation

smowton
Copy link
Contributor

@smowton smowton commented Jun 14, 2017

These result from array_set instructions, which when targeting variable-length arrays can end up targeting zero-sized arrays.

Some regression tests in test-gen-support are currently not looking for warning: ignoring because of this.

@smowton smowton requested review from tautschnig and kroening June 14, 2017 12:57
@tautschnig
Copy link
Collaborator

Working with zero-sized arrays seems wrong - would it be possible to get a pointer to a regression test?

@smowton
Copy link
Contributor Author

smowton commented Jun 14, 2017

https://github.com/diffblue/cbmc/tree/test-gen-support/regression/cbmc-java/NondetArray2 running against current test-gen-support with latest master merged (e.g. here: https://github.com/smowton/cbmc/tree/smowton/merge/master_20170614)

Basically new int[0] is legal Java, and it's an open choice whether we try to intercept that at Java -> array-set or array-set -> array-of or array-of -> prop stage.

@tautschnig
Copy link
Collaborator

Oh, I was not aware that this was coming from Java. We should certainly have this backed by a regression test to document the context in which this is expected.

Copy link
Collaborator

@tautschnig tautschnig left a comment

Choose a reason for hiding this comment

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

Please include a regression test.

smowton added 2 commits June 19, 2017 14:54
These result from array_set instructions, which when targeting
variable-length arrays can end up targeting zero-sized arrays.
This should compile warning-free, since it is legal Java. Previously
an array_of expression with nil length yielded a warning, as would a WITH
based upon one.
@smowton smowton force-pushed the smowton/fix/array_of_zero_length branch from e967fd0 to 677f39d Compare June 19, 2017 13:55
@smowton
Copy link
Contributor Author

smowton commented Jun 19, 2017

Added a test and made the checks a little more precise. I considered avoiding calling cpp_new[] at all when the array is zero length, but determined this was unlikely to work as we'd end up with an expression like
x == 0 ? null : new int[x], which I believe would still try to convert the new int[] part regardless of x.

Also I note that https://github.com/diffblue/cbmc/blob/master/src/solvers/flattening/boolbv_if.cpp#L17 already tolerates a zero-length bitvector, so I think it's probably okay to pass these in other places too.

{
// A zero-length array is acceptable;
// an element with unknown size is not.
if(boolbv_width(array_type.subtype())==0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about arrays of arrays of zero length? Would Java permit that? If so, this check needs to be amended.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, at least in Java such a thing would be an array of pointers

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok!

@tautschnig
Copy link
Collaborator

I'm not sure what's going on here with regard to the build checks...

@smowton
Copy link
Contributor Author

smowton commented Jun 19, 2017

Re: checks I accidentally pushed to origin/smowton/... instead of smowton/smowton/... and then deleted the branch created in error. I think the /push and /branch ones relate to that and the PR ones are working as intended.

@kroening kroening merged commit 1cb1fd9 into diffblue:master Jul 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants