Add support for polymorphic bounds safe interface functions #546

Prabhuk · 2018-08-11T02:29:27Z

Added new keyword _Itype_for_any which is a function specifier.

Pending:

Parsing logic of _For_any and _Itype_for_any seems to fail in linux
Spec needs to be updated

Initial parsing code to identify our new checked c keyword "_Itype_for_any" is added.

Design document md file.

The parser now treats void * and pointer to TypeVariableType as equivalent types when the TypeVariableType is encountered under the _Itype_for_any scope

The ItypeGenericFunctions are now compatible with declarations of non bounds safe generic function allowing redeclaration of interface funcions with _Itype_for_any specifier and type variables.

Parsing of calls to itype generic functions from checked scope is partially complete. Pending: 1. Calls from unchecked scope 2. Parameter type mismatch errors

Parsing of calls to itype generic functions from checked scope is complete. Pending: 1. Parse calls from unchecked scope where type variables may not be substituted with a call site provided type (Apply default type which is "void") 2. Add tests

…ill throw an error now. Task: Adding support for polymorphic bounds safe interface functions. Pending: 1. Parse calls from unchecked scope where type variables may not be substituted with a call site provided type (Apply default type which is "void") 2. Add tests 3. Fix Checked C specifications to reflect the modified syntax for interface type generic functions

…ubstituted with a "call site provided type". (Apply default type for params and return types in this case.) Task: Adding support for polymorphic bounds safe interface functions. Pending: 1. Add tests 2. Fix Checked C specifications to reflect the modified syntax for interface type generic functions

Task: Adding support for polymorphic bounds safe interface functions. Pending: 1. Fix Checked C specifications to reflect the modified syntax for interface type generic functions 2. Add examples to the spec

Fixed linux checked c test failures.

AST serialization inconsistencies fixed.

Reordering initializers in constructor call to avoid compile time reorder warning during compilation of the compiler.

Fixed the regression bug created by messing with the way type information was stored in the Profile object in clang.

Refined type argument parsing code for polymorphic bounds safe interface functions. Cleaned up comments and code.

Prabhuk · 2018-08-14T20:12:02Z

This PR is ready for review.

dtarditi

This is great to see!

There are some code formatting issues to address so that we meet the clang coding style guidelines.
I think there is an issue around generic function types being compatible with itype generic function types. I think this should be handled after this PR is complete.
I think you should try to common (share) some of the itype_for_any and for_any code.
You should add tests of AST dumping to clang\tests\Checkedc\generic-function.
You should also add tests for pre-compiled header for clang\tests\Checkedc\pch.c and pch.h,

dtarditi · 2018-08-15T17:01:19Z

include/clang/AST/ASTContext.h

  bool isAtLeastAsCheckedAs(QualType T1, QualType T2) const;

  /// \brief Determine whether a pointer, array, or function type T1
  /// is the same as the other pointer, array, or function type T2 if
  /// checkedness is ignored.  Return true if does or false if the types
  /// differ in some other way than checkedness.
+  /// Note:: Checked C - In bounds safe interface scopes, this function


You don't need to prefix this with "Checked C", as this function is specific to the Checked C implementation.

dtarditi · 2018-08-15T17:01:37Z

include/clang/AST/ASTContext.h

@@ -2486,12 +2488,16 @@ class ASTContext : public RefCountedBase<ASTContext> {
  /// at least as much checking as the other type T2.  Return true if it does
  /// or false if it does not or the types differ in some other way than
  /// checkedness.
+  /// Note:: Checked C - In bounds safe interface scopes, this function


You don't need to prefix this with "Checked C", as this function is specific to the Checked C implementation.

dtarditi · 2018-08-15T17:08:58Z

lib/AST/ASTContext.cpp

  if (lproto && rproto) { // two C99 style function prototypes
    assert(!lproto->hasExceptionSpec() && !rproto->hasExceptionSpec() &&
           "C++ shouldn't be here");
    // Compatible functions must have the same number of parameters
    if (lproto->getNumParams() != rproto->getNumParams())
      return QualType();

-    // Compatible functions must have the same number of type variables.
-    if (lproto->getNumTypeVars() != rproto->getNumTypeVars())
-      return QualType();


Place put a space after the 'if' keyword.

dtarditi · 2018-08-15T17:11:57Z

lib/AST/ASTContext.cpp

@@ -8194,6 +8204,12 @@ QualType ASTContext::mergeFunctionTypes(QualType lhs, QualType rhs,
        } else if (!rReturnAnnots.IsEmpty() && lReturnAnnots.IsEmpty()) {
          ReturnAnnots = rReturnAnnots;
          allLTypes = false;
+        } else if (lproto->isItypeGenericFunction() && !rproto->isItypeGenericFunction()) {


I think you need to check that rproto is also not a generic function. Generic function types should not be compatible with itype generic function type. We will allow itype generic function types to be converted to generic types at assignments, call sites, and returns. We will not allow the reverse.

I think you should leave the code "as is" and open an issue to handle this after this PR is complete.
This is already a large pull request. You'll need to write some tests and alter some of the assignment compatibility code. It's a special case that can wait.

dtarditi · 2018-08-15T17:17:02Z

lib/Parse/ParseDecl.cpp

@@ -7204,6 +7220,91 @@ void Parser::ParseCheckedPointerSpecifiers(DeclSpec &DS) {
        Diag(StartLoc, DiagID) << PrevSpec;
 }

+void Parser::ParseItypeforanySpecifier(DeclSpec &DS) {


I think you should common this code with parsing for regular forall functions. You can use formatting specifiers in clang error messages to customize parts of error message for specific cases.

dtarditi · 2018-08-15T17:23:03Z

lib/Sema/DeclSpec.cpp

+bool DeclSpec::setFunctionSpecItypeforany(SourceLocation Loc,
+                                              const char *&PrevSpec,
+                                              unsigned &DiagID) {
+  if (FS_itypeforany_specified || FS_forany_specified) {


Same here, in terms of this being a conflict, not a duplicate.

dtarditi · 2018-08-15T17:24:36Z

lib/Sema/SemaDecl.cpp

@@ -8729,6 +8737,17 @@ Sema::ActOnFunctionDeclarator(Scope *S, Declarator &D, DeclContext *DC,
    }
  }

+  if (D.getDeclSpec().isItypeforanySpecified()) {


Can you common this code with the existing case with IsForanySpecified?

dtarditi · 2018-08-15T17:25:29Z

lib/Serialization/ASTWriterStmt.cpp

@@ -402,9 +402,22 @@ void ASTStmtWriter::VisitDeclRefExpr(DeclRefExpr *E) {
  Record.push_back(E->hasTemplateKWAndArgsInfo());
  Record.push_back(E->hadMultipleCandidates());
  Record.push_back(E->refersToEnclosingVariableOrCapture());
-  bool isGenericFunction = E->GetTypeArgumentInfo() != nullptr;
+  bool isGenericFunction = false;
+  if(E->getDecl() && isa<FunctionDecl>(E->getDecl()))


I think the brace should be on the prior line.

dtarditi · 2018-08-15T17:25:57Z

lib/AST/ASTContext.cpp

@@ -8923,6 +8951,18 @@ bool ASTContext::isEqualIgnoringChecked(QualType T1, QualType T2) const {
    return true;

  Type::TypeClass T1TypeClass = T1->getTypeClass();
+  if(T1TypeClass == Type::Pointer)
+  {


I think the brace should be on the prior line.

dtarditi · 2018-08-15T17:39:29Z

include/clang/Basic/DiagnosticParseKinds.td

@@ -1204,12 +1204,27 @@ def err_forany_type_variable_identifier_expected : Error<
 def err_forany_unexpected_token : Error<
  "unexpected token in _For_any specifier">;

+def err_itypeforany_comma_or_parenthesis_expected : Error<


Clang development guidelines favor sharing diagnostic messages when possible, to minimize the number of different messages to manage. Diagnostic message can contain formatting specifiers that allow the message to be customized for a specific circumstance. We should try to follow those guidelines.

Addressed PR comments. 1. Alignment fixes 2. Merged duplicate code 3. Fixed duplicate error messages

Prabhuk · 2018-08-15T23:56:54Z

The PR comments are addressed.
Follow-up tasks:

Create a new issue for itype_for_any and for_any function type compatibility.
Change the signature of malloc's signature in stdlib_checked header file and fix the uses in checked scopes in tests.

dtarditi

There's a fix needed a comment. Otherwise looks good! Thanks.

dtarditi · 2018-08-16T03:33:51Z

include/clang/AST/ASTContext.h

@@ -2486,12 +2488,16 @@ class ASTContext : public RefCountedBase<ASTContext> {
  /// at least as much checking as the other type T2.  Return true if it does
  /// or false if it does not or the types differ in some other way than
  /// checkedness.
+  /// Note:: In bounds safe interface scopes, this function


T1 and T2 in this comment should be reversed.

Fixed comments.

…gnment (#546) When assigning from a itype(nt_array_ptr) to a regular nt_array_ptr, we need to insert a cast to work around a CheckedC compiler bug (microsoft#614) (fix #545).

Prabhuk added 16 commits July 27, 2018 17:20

Add support for polymorphic bounds safe interface functions.

454c63e

Initial parsing code to identify our new checked c keyword "_Itype_for_any" is added.

Add support for polymorphic bounds safe interface functions.

eec9aaa

Design document md file.

Add support for polymorphic bounds safe interface functions.

1fa153a

The parser now treats void * and pointer to TypeVariableType as equivalent types when the TypeVariableType is encountered under the _Itype_for_any scope

Add support for polymorphic bounds safe interface functions.

6d0ff23

The ItypeGenericFunctions are now compatible with declarations of non bounds safe generic function allowing redeclaration of interface funcions with _Itype_for_any specifier and type variables.

Add support for polymorphic bounds safe interface functions.

4b53bd2

Parsing of calls to itype generic functions from checked scope is partially complete. Pending: 1. Calls from unchecked scope 2. Parameter type mismatch errors

Regression: failures of of redeclartion tests fixed.

1802837

Comments cleanup.

0132dee

Task: Adding support for polymorphic bounds safe interface functions. Pending: 1. Fix Checked C specifications to reflect the modified syntax for interface type generic functions 2. Add examples to the spec

Comments cleanup.

55aee0b

Task: Adding support for polymorphic bounds safe interface functions. Pending: 1. Fix Checked C specifications to reflect the modified syntax for interface type generic functions 2. Add examples to the spec

Adding support for polymorphic bounds safe interface functions.

13ec4dc

Fixed linux checked c test failures.

Adding support for polymorphic bounds safe interface functions.

dabd7ee

AST serialization inconsistencies fixed.

Adding support for polymorphic bounds safe interface functions.

6c5f1ed

Reordering initializers in constructor call to avoid compile time reorder warning during compilation of the compiler.

Adding support for polymorphic bounds safe interface functions.

c8c0ddb

Fixed the regression bug created by messing with the way type information was stored in the Profile object in clang.

Adding support for polymorphic bounds safe interface functions.

b5221ed

Refined type argument parsing code for polymorphic bounds safe interface functions. Cleaned up comments and code.

dtarditi reviewed Aug 15, 2018

View reviewed changes

dtarditi mentioned this pull request Aug 15, 2018

Adding support for polymorphic bounds safe interface functions. checkedc/checkedc#303

Merged

Adding support for polymorphic bounds safe interface functions.

52c4581

Addressed PR comments. 1. Alignment fixes 2. Merged duplicate code 3. Fixed duplicate error messages

dtarditi approved these changes Aug 16, 2018

View reviewed changes

Adding support for polymorphic bounds safe interface functions.

582e627

Fixed comments.

Prabhuk merged commit 4dc7631 into master Aug 16, 2018

dtarditi deleted the bounds_safe_interface branch August 28, 2018 16:32

This was referenced Jan 15, 2022

Adding support for polymorphic bounds safe interface functions. checkedc/checkedc-fork#303

Closed

Add support for polymorphic bounds safe interface functions checkedc/checkedc-llvm-project#545

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add support for polymorphic bounds safe interface functions #546

Add support for polymorphic bounds safe interface functions #546

Uh oh!

Prabhuk commented Aug 11, 2018

Uh oh!

Prabhuk commented Aug 14, 2018

Uh oh!

dtarditi left a comment •

edited

Loading

Uh oh!

dtarditi Aug 15, 2018

Uh oh!

dtarditi Aug 15, 2018

Uh oh!

dtarditi Aug 15, 2018

Uh oh!

dtarditi Aug 15, 2018

Uh oh!

dtarditi Aug 15, 2018

Uh oh!

dtarditi Aug 15, 2018

Uh oh!

dtarditi Aug 15, 2018

Uh oh!

dtarditi Aug 15, 2018

Uh oh!

dtarditi Aug 15, 2018

Uh oh!

dtarditi Aug 15, 2018

Uh oh!

Prabhuk commented Aug 15, 2018

Uh oh!

dtarditi left a comment

Uh oh!

dtarditi Aug 16, 2018

Uh oh!

Uh oh!

Add support for polymorphic bounds safe interface functions #546

Add support for polymorphic bounds safe interface functions #546

Uh oh!

Conversation

Prabhuk commented Aug 11, 2018

Uh oh!

Prabhuk commented Aug 14, 2018

Uh oh!

dtarditi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Prabhuk commented Aug 15, 2018

Uh oh!

dtarditi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dtarditi left a comment •

edited

Loading