Skip to content

Add checking of redeclarations of variables with bounds. #96

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 4 commits into from
Dec 22, 2016

Conversation

dtarditi
Copy link
Member

This change adds checking of redeclarations of variables with bounds, ensuring that the redeclarations follow the Checked C rules. This completes the work for issue #30. For now, when bounds are required to match, we require them to be identical syntactically. This will be generalized later.

The checking is done when bounds are attached to variable declarations, not during the merging of declarations. Bounds are attached to variable declarations after declarators have been processed (the bounds may refer to the variable being declared, so the variable declaration needs to be built before we build the bounds expression). This means bounds can't be checked during merging of declarations, which operates on just the declarators.

  • ActOnBoundsDecl does the checking for conflicting bounds expressions on variable declarations.
  • Generalize the existing code for checking for conflicting bounds expressions on declaration to handle non-parameter variables. Also generalize the check for variables with unchecked types to include unchecked arrays. Declarations of variables with unchecked pointer and unchecked array types and bounds-safe interfaces are compatible with the declarations that omit the bounds-safe interfaces.
  • Rework the existing error messages for variable redeclarations to use the clang select mechanism for error messages. This lets us use one diagnostic id in place of several diagnostic ids and simplify the code.

I discovered an error in the parsing of bounds expressions for function declarators. I believe it was possible to write a bounds expression after a function declarator, which would have triggered an internal compiler assert: array_ptr<int> f(int len) : count(len) : count(5), for example. The fix is to not try to parse a bounds expression after a function declarator. The parsing of the function declarator already handled the bounds expression.

Testing:

  • Added tests for redeclarations of variables to the Checked C regression tests. This will be committed separately to the Checked C repo.
  • Passes existing Checked C tests.
  • Passes clang regression test suite.

This change integrates checking of bounds for variables into the process
for merging declarations.  However, bounds don't actually affect how
declarations are merged.  Also, bounds are attached to variable declarations
after the declarator has been processed, so they can't be processed
during merging.  Checkpoint things before moving the checking later.
This completes issue checkedc#30.
- Move checking for conflicting bounds expressions on variable declarations
  later to ActOnBoundsDecl.  A bounds expression for a variable is not
  constructed/built until after the declarator has been processed, so
  we cannot check it as part of checking the declarator.
- Generalize the existing code for checking for conflict bounds expression
  on declaration to handle variables.
- I discovered an error in the parsing of bounds expressions for
  function declarators.  I believe it was possible to write a bounds expression
  after a function declarator, which would have triggered an internal
  compiler assert:  array_ptr<int> f(int len) : count(len) : count(5),
  for example.   The fix is to not try to parse a bounds expression after
  a function declarator, since the parsing of the function declarator
  already handled it.
The more relaxed compatiblity rules for bounds declarations
were not being applied to unchecked array variables.  Generalize
the check for uncheckedness to include unchecked arrays.
@dtarditi dtarditi added this to the Sprint 11 milestone Dec 22, 2016
@dtarditi dtarditi merged commit 5c63597 into checkedc:master Dec 22, 2016
awruef pushed a commit that referenced this pull request Dec 29, 2016
This change adds checking of redeclarations of variables with bounds, ensuring that the redeclarations follow the Checked C rules.  This completes the work for issue #30.  For now, when bounds are required to match, we require them to be identical syntactically.  This will be generalized later.

The checking is done when bounds are attached to variable declarations, not during the merging of declarations.  Bounds are attached to variable declarations after declarators have been processed (the bounds may refer to the variable being declared, so the variable declaration needs to be built before we build the bounds expression).  This means bounds can't be checked during merging of declarations, which operates on just the declarators.
- ActOnBoundsDecl does the checking for conflicting bounds expressions on variable declarations.
- Generalize the existing code for checking for conflicting bounds expressions on declaration to handle non-parameter variables.  Also generalize the check for variables with unchecked types to include unchecked arrays.   Declarations of variables with unchecked pointer and unchecked array types and bounds-safe interfaces are compatible with the declarations that omit the bounds-safe interfaces.
- Rework the existing error messages for variable redeclarations to use the clang select mechanism for error messages.   This lets us use one diagnostic id in place of several diagnostic ids and simplify the code.

I discovered an error in the parsing of bounds expressions for function declarators.  I believe it was possible to write a bounds expression after a function declarator, which would have triggered an internal compiler assert:  `array_ptr<int> f(int len) : count(len) : count(5)`, for example.   The fix is to not try to parse a bounds expression after a function declarator.  The parsing of the function declarator already handled the bounds expression.

Testing:
- Added tests for redeclarations of variables to the Checked C regression tests.  This will be committed separately to the Checked C repo.
- Passes existing Checked C tests.
- Passes clang regression test suite.
@dtarditi dtarditi deleted the issue30 branch February 6, 2017 21:36
dopelsunce pushed a commit to dopelsunce/checkedc-clang that referenced this pull request Sep 28, 2020
)

This change matches a corresponding change in the Checked C
clang repo.  The compiler will be checking that automatic variables with
_Ptr types or bounds declarations are always initialized using an
initializer.

This adds missing initializers to existing tests.  It also adds tests that
the compiler produces errors for uninitialized automatic variables.

Note that static variables are always initialized to 0, if they do not
have an initializer.  This means that we do not have to check that they
are initialized.  0 is valid for any bounds declaration, so it works well as
a default initialization value.
sulekhark pushed a commit that referenced this pull request Jul 21, 2021
This patch implemented TTI.IntImmCost() properly.
Each BPF insn has 32bit immediate space, so for any immediate
which can be represented as 32bit signed int, the cost
is technically free. If an int cannot be presented as
a 32bit signed int, a ld_imm64 instruction is needed
and a TCC_Basic is returned.

This change is motivated when we observed that
several bpf selftests failed with latest llvm trunk, e.g.,
  #10/16 strobemeta.o:FAIL
  #10/17 strobemeta_nounroll1.o:FAIL
  #10/18 strobemeta_nounroll2.o:FAIL
  #10/19 strobemeta_subprogs.o:FAIL
  #96 snprintf_btf:FAIL

The reason of the failure is due to that
SpeculateAroundPHIsPass did aggressive transformation
which alters control flow for which currently verifer
cannot handle well. In llvm12, SpeculateAroundPHIsPass
is not called.

SpeculateAroundPHIsPass relied on TTI.getIntImmCost()
and TTI.getIntImmCostInst() for profitability
analysis. This patch implemented TTI.getIntImmCost()
properly for BPF backend which also prevented
transformation which caused the above test failures.

Differential Revision: https://reviews.llvm.org/D96448

(cherry picked from commit a260ae7)
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.

2 participants