Skip to content

Correctly identify extern statics#2054

Merged
tedinski merged 5 commits into
model-checking:mainfrom
tedinski:extern-static
Jan 3, 2023
Merged

Correctly identify extern statics#2054
tedinski merged 5 commits into
model-checking:mainfrom
tedinski:extern-static

Conversation

@tedinski
Copy link
Copy Markdown
Contributor

Description of changes:

Fixes Kani's detection of extern static variables. This has no observable effect, because I believe this has no animating semantics on the CBMC side. I suspect this flag on the symbol exists for the CBMC C compiler's benefit only.

Nevertheless, this corrects our behavior. The old linkage check was wrong: that only detects the presence of a #[linkage] attribute, not the externness of the declaration.

Resolved issues:

Resolves #388
Resolves #400

Testing:

  • How is this change tested? N/A apparently: a comment describes my experiments. It doesn't matter.

  • Is this a refactor change? no

Checklist

  • Each commit message has a non-empty body, explaining why the change was made
  • Methods or procedures are documented
  • Regression or unit tests are included, or existing tests cover the modified code
  • My PR is restricted to a single feature or bugfix

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.

@tedinski tedinski requested a review from a team as a code owner December 30, 2022 23:01
Comment on lines +508 to +511
// CBMC shares C's notion of "extern" global variables. However, I believe
// CBMC actually only uses this information during C typechecking.
// A quick `git grep is_extern` in the CBMC sources seems to support this theory.
// Replacing `is_extern` with a constant `true` or `false` all pass the regression.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

CBMC does also use this information when constructing the initialiser code for objects with static lifetime. When is_extern is true and there is no initialisation value provided then a non-deterministic value is constructed. I suspect that either you never have symbols without initialiser, or perhaps those are just never used (which would be the desirable!).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll add that info to the comment, but: we... might not want that behavior for Kani? I would be inclined to see errors if we try to use an uninitialized static variable, just like we do for missing function definitions.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In CBMC, is !is_extern with a missing initializer an error or zero-initialized?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Aha, this is the function static_lifetime_init in static_lifetime_init.cpp and I think I see the zero-init. :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes re zero-init; for the "error" case: I think that's the "poison value" problem, so we really need/want a solution for that one.

Copy link
Copy Markdown
Contributor

@celinval celinval left a comment

Choose a reason for hiding this comment

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

Is it possible to add some tests that declare and use extern statics?

@tedinski
Copy link
Copy Markdown
Contributor Author

tedinski commented Jan 3, 2023

Is it possible to add some tests that declare and use extern statics?

Given Michael's clarification about the actual semantics difference, I can add a test for that at least.

Interestingly, the previous behavior was extra-broken. It was setting is_extern to rlinkage.is_none() which... was probably the opposite of the intention. Of course, it didn't break anything because no current test tries to exercise a missing/uninitialized extern static.

Copy link
Copy Markdown
Contributor

@celinval celinval left a comment

Choose a reason for hiding this comment

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

Thanks

@tedinski tedinski merged commit 5b6e99a into model-checking:main Jan 3, 2023
@tedinski tedinski deleted the extern-static branch January 3, 2023 22:51
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.

rlinkage.is_none() assertion fails locally Ensure that when rlinkage is Some, then the static is not extern

3 participants