Skip to content
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

Handle redeclarations of functions and variables with bounds #30

Closed
dtarditi opened this issue Aug 12, 2016 · 2 comments
Closed

Handle redeclarations of functions and variables with bounds #30

dtarditi opened this issue Aug 12, 2016 · 2 comments
Assignees
Milestone

Comments

@dtarditi
Copy link
Contributor

For interoperation, we will redeclare existing library functions with additional bounds information. We assume that modifying library source code is not possible when modifying programs that use libraries. We'll have to add bounds information separately. C allows functions and extern variable to be redeclared, so long as the redeclared versions are compatible with the existing version.

We need to extend the code for checking redeclaration to take bounds into account. For parameters or variables of array_ptr type, bounds will have to match. For now, we will use syntactic equality. If they have unchecked pointer type, if they both of have bounds, they must match. Otherwise, the bounds will be regard as being part of a more "complete" declaration of the parameter or variable.

@dtarditi dtarditi self-assigned this Aug 12, 2016
@dtarditi dtarditi added this to the Sprint 10 milestone Aug 12, 2016
dtarditi added a commit that referenced this issue Nov 1, 2016
Calls to no-prototype functions are not type checked according to the C specification (Section 6.5.2.2 of the C11 specification).  The lack of type checking allows the checking of bounds declarations to be bypassed.   For the Checked C extension, this is addressed by not allowing redeclarations of no-prototype functions to have prototypes that use checked types, as well as not allowing checked values to be passed to no-prototype functions.  Section 5.5 of the Checked C extension specification describes the restrictions.  

This change implements the Checked C restrictions on redeclarations of no-prototype functions.  It is  part of work on issue #30.    The restrictions are implemented by changing the implementation of C type compatibility in clang.   We make no-prototype function types incompatible with function types that take arguments or return values that have checked types, contain values with checked types embedded within them, or are recursively pointers to function types that take or return check types.  We also change the construction of function types from function declarators to enforce the Checked C rule that a no-prototype function cannot have a checked return type.

When two declarations of a function are incompatible, we try to diagnose whether a failure is due to a Checked C type compatibility rule and print descriptive error messages.   We do this only when the failure  is because of the presence or absence of a prototype of a declaration.  We do not try to diagnose type compatibility problems involving parameters with function pointer types,  In that case, programmers will get a generic "types are not compatible" message.

There is a subtle complication when checking redeclarations in the presence of incomplete structure or union types.  In the Checked C rules, a function type with an incomplete type is compatible with a no-prototype function (because the incomplete type is unknown).  For a series of declarations of a function, clang merges the types so that the latest declaration has the merged type.  This means that a function could start as a no-prototype function, be given a prototype with an incomplete type, the incomplete type could be completed to be  checked type, and then the function could be declared again:
```
int f();      // decl 1
struct S;
int f(S v);   // decl 2
// complete the type
struct S {
  ptr<int> p;
}
...
int f(S v) {  // decl 3 (possibly a definition)
  ...
}
```
clang looks at only the type of the prior declaration when merging declaration types.  For Checked C, this misses the fact that `f` was declared originally  as a no-prototype function at `decl 1`.   The prototype was allowed only because it involved an incomplete type at `decl 2`.  If we had the complete type at `decl 2`, the prototype would not have been allowed.  Instead of looking at just the prior declaration, we look back at all prior declarations of a function to make sure they are compatible with the current declaration of the function.

Note that we still need to prevent `decl 2` from being used after `S` is complete.  The current change only addresses redeclarations of `f`, not uses of `f`.   That is  future work tracked by issue #75. 

In the implementation of type compatibility, we miss the case of no-prototype function pointers being incompatible with prototype function pointers that take integer arguments with bounds.  This depends on issue #20 (adding bounds to function types).  We will address this case as part of issue #20.

Testing:
- This change uncovered places in existing tests in the Checked C repo where we were using no prototype functions.  Update the tests to properly declare the functions as having no arguments (using the `f(void)` pattern).
- Add a new tests on to Checked C repo in typechecking\no_prototype_functions.c.
- Add some clang-specific tests under tests\CheckedC\typechecking.c.
- Passes the updated and new Checked C tests.
- Passes the existing clang regression tests.
@dtarditi dtarditi modified the milestones: Sprint 11, Sprint 10 Nov 22, 2016
@dtarditi
Copy link
Contributor Author

Checking redeclarations of functions was completed by pull request #91

dtarditi added a commit to dtarditi/checkedc-clang that referenced this issue Dec 22, 2016
This completes issue microsoft#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.
dtarditi added a commit that referenced this issue Dec 22, 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
Copy link
Contributor Author

This work is complete.

awruef pushed a commit that referenced this issue 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.
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

No branches or pull requests

1 participant