Skip to content

Add support for _Return_value expression. #544

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 9 commits into from
Aug 19, 2018
Merged

Add support for _Return_value expression. #544

merged 9 commits into from
Aug 19, 2018

Conversation

dtarditi
Copy link
Member

The _Return_value expression allows programmers to name the value returned by a function in a return bounds declaration. This changes extends the clang IR to represent it and _Current_expr_value. It also adds lexing, parsing, and type system support for _Return_value. This addresses the first part of issue #205. Support for checking of bounds declarations using _Return_value will be implemented next.

We create a new expression class BoundsValueExpr to represent these expressions. This class is modelled on CXXThisExpr. Constructing a_Return_value expression requires knowing the return type of the function that the return bounds is associated with. We must have the _Return_value expression constructed before we can build the function type, since function types incorporate the return bounds expressions. Unfortunately, we don't know the function return type until the middle of GetFullTypeForDeclarator in SemaType.cpp. Because clang intertwines AST building, type checking, and other static semantic checking, we have to "defer parse" return bounds expressions. "Defer parse" means that the compiler recognize the tokens that make up a return bounds expression, saves them, and parses them later to build the IR for the return bounds expressions.

To allow parsing of the return bounds to happen in the middle of SemaType.cpp, we introduce a callback from semantic checking to the parser. This lets us preserve the interface between the two. We have to bring the parameters into scope, since they've already gone out of scope by the time GetFullTypeForDeclarator runs. We aren't the first people to have to do this. Other parsing phases such as parsing of attributes have to do this too.

After parsing the return bounds expression, we attach it to the declarator whose type is being constructed. This lets us get the line number information right for error messages involving redeclarations with conflicting return bounds. (Long explanation: memoization of function types also memoizes bounds expressions embedded in ty pes. It does this by ignoring line numbers and abstracting function parameters to parameter locations. The end result is that line numbers for expressions embedded in types are meaningless. Clang has a complicated mechanism for tracking source line information for types called TypeSourceInfo, but we haven't implemented support for embedded expressions in types and are not likely to anytime soon).

This change also contains some unrelated clean up:

  1. Fix the line ending for the test case for bug526 to be line feeds.
  2. Delete identifiers for _Dynamic_bounds_cast and _Assume_bounds_cast. These are keywords, so we don't need identifiers.

Testing:

  • There will be a separate pull request to the Checked C repo for parsing and typechecking tests for_Return_value.
  • Added clang tests for AST dumping and checking of bounds declarations. The checking of bounds declarations involving _Return_value doesn't work yet, so the tests document this.
  • Passes automated Checked C and clang testing with these changes.

Copy link
Collaborator

@Prabhuk Prabhuk 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! Just a couple of alignment and comment related fixes.


ExprResult Parser::ParseReturnValueExpression() {
assert(Tok.is(tok::kw__Return_value) &&
"Not bounds value expression");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Double space between bounds and value in the message "Not bounds value expression"

@@ -215,8 +216,8 @@ DeclaratorChunk DeclaratorChunk::getFunction(bool hasProto,
TrailingReturnType.isInvalid();
I.Fun.TrailingReturnType = TrailingReturnType.get();
I.Fun.ReturnAnnotsColonLoc = ReturnAnnotsColonLoc.getRawEncoding();
I.Fun.ReturnBounds = ReturnAnnotsExpr.getBoundsExpr();
I.Fun.ReturnInteropType = ReturnAnnotsExpr.getInteropTypeExpr();
I.Fun.ReturnBounds = ReturnBounds.release();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove space before "ReturnBounds.release()"

BoundsExpr *ReturnBounds;
// as individual fields because the return bounds are deferred-parsed.
// Note: ReturnBounds is actually a unique_ptr. However unique_ptr requires
// a constructor and this struct can't have one, so so we cast it to a
Copy link
Collaborator

Choose a reason for hiding this comment

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

Double space before "However" and duplicate "so".

// function declarator. Diagnosis this, suggest a fix, and bail out.
// It is easy to misplace it. Handle the case where the return bounds
// expression is misplaced for a complex function declarator.
// Diagnosis this, suggest a fix, and bail out.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Diagnosis -> Diagnose

dtarditi added a commit to checkedc/checkedc that referenced this pull request Aug 19, 2018
This changes add tests for parsing and type checking of return_value expressions.  This matches the compiler pull request at checkedc/checkedc-clang#544.
@dtarditi dtarditi merged commit 7a73aa6 into master Aug 19, 2018
@dtarditi dtarditi deleted the issue205 branch August 25, 2018 17:33
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.

2 participants