Add flag to infer checked pointers types for undefined functions (#691)

Add a new flag (`-infer-types-for-undefs`) that enables inferring pointer types
for undefined functions. Undefined functions are still constrained to be
internally unchecked, so they can only be rewritten to itypes but never fully
checked types. For example: `void foo(int *);` converts to
`void foo(int * : itype(_Ptr<int>));`.

This change also eliminates some code that was previously duplicated inside
`ProgramInfo::link()` for constraining undefined external and static functions.
The root cause message emitted for undefined functions has been changed to use
the same message for both static and external functions.

Author: john-h-kastner

clang/include/clang/3C/3CGlobalOptions.h

@@ -67,6 +67,8 @@ struct _3COptions {
   bool AllowRewriteFailures;
 
   bool ItypesForExtern;
+
+  bool InferTypesForUndefs;
 };
 
 // NOLINTNEXTLINE(readability-identifier-naming)

clang/include/clang/3C/ProgramInfo.h

@@ -269,6 +269,8 @@ class ProgramInfo : public ProgramVariableAdder {
   // constraint system for that pointer type.
   void addVariable(clang::DeclaratorDecl *D,
                    clang::ASTContext *AstContext) override;
+
+  void linkFunction(FunctionVariableConstraint *FV);
 };
 
 #endif

clang/include/clang/3C/Utils.h

@@ -181,6 +181,8 @@ bool isInSysHeader(clang::Decl *D);
 
 std::string getSourceText(const clang::SourceRange &SR,
                           const clang::ASTContext &C);
+std::string getSourceText(const clang::CharSourceRange &SR,
+                          const clang::ASTContext &C);
 
 // Find the longest common subsequence.
 unsigned longestCommonSubsequence(const char *Str1, const char *Str2,

clang/lib/3C/ConstraintVariables.cpp

@@ -556,12 +556,21 @@ std::string PointerVariableConstraint::tryExtractBaseType(DeclaratorDecl *D,
                                                           QualType QT,
                                                           const Type *Ty,
                                                           const ASTContext &C) {
-  bool FoundBaseTypeInSrc = false;
-  if (D && !TSI)
+  // Implicit parameters declarations from typedef function declarations will
+  // still have valid and non-empty source ranges, but implicit declarations
+  // aren't written in the source, so extracting the base type from this range
+  // gives incorrect type strings. For example, the base type for the implicit
+  // parameter for `foo_decl` in `typedef void foo(int*); foo foo_decl;` would
+  // be extracted as "foo_decl", when it should be "int".
+  if (!D || D->isImplicit())
+    return "";
+
+  if (!TSI)
     TSI = D->getTypeSourceInfo();
-  if (!QT->isOrContainsCheckedType() && !Ty->getAs<TypedefType>() && D && TSI) {
+  if (!QT->isOrContainsCheckedType() && !Ty->getAs<TypedefType>() && TSI) {
     // Try to extract the type from original source to preserve defines
     TypeLoc TL = TSI->getTypeLoc();
+    bool FoundBaseTypeInSrc = false;
     if (isa<FunctionDecl>(D)) {
       FoundBaseTypeInSrc = D->getAsFunction()->getReturnType() == QT;
       TL = getBaseTypeLoc(TL).getAs<FunctionTypeLoc>();
@@ -2220,8 +2229,30 @@ FVComponentVariable::FVComponentVariable(const QualType &QT,
   // parameters similarly to how we do it for pointers in regular function
   // declarations.
   if (D && D->getType() == QT) {
-    SourceRange SR = D->getSourceRange();
-    SourceDeclaration = SR.isValid() ? getSourceText(SR, C) : "";
+    SourceRange DRange = D->getSourceRange();
+    if (DRange.isValid()) {
+      const SourceManager &SM = C.getSourceManager();
+      SourceLocation DLoc = D->getLocation();
+      CharSourceRange CSR;
+      if (SM.isBeforeInTranslationUnit(DRange.getEnd(), DLoc)) {
+        // It's not clear to me why, but the end of the SourceRange for the
+        // declaration can come before the SourceLocation for the declaration.
+        // This can result in SourceDeclaration failing to capture the whole
+        // declaration string, causing rewriting errors. In this case it is also
+        // necessary to use CharSourceRange::getCharRange to work around some
+        // cases where CharSourceRange::getTokenRange (the default behavior of
+        // getSourceText when passed a SourceRange) would not get the full
+        // declaration. For example: a parameter declared without a name such as
+        // `_Ptr<char>` was captured as `_Ptr`.
+        CSR = CharSourceRange::getCharRange(DRange.getBegin(), DLoc);
+      } else {
+        CSR = CharSourceRange::getTokenRange(DRange);
+      }
+
+      SourceDeclaration = getSourceText(CSR , C);
+    } else {
+      SourceDeclaration = "";
+    }
   }
 }
 

clang/lib/3C/DeclRewriter.cpp

@@ -567,7 +567,14 @@ bool FunctionDeclBuilder::VisitFunctionDecl(FunctionDecl *FD) {
 
   // If this is an external function, there is no need to rewrite the
   // declaration. We cannot change the signature of external functions.
-  if (!FDConstraint->hasBody())
+  // Under the flag -infer-types-for-undef, however, undefined functions do need
+  // to be rewritten. If the rest of the 3c inference and rewriting code is
+  // correct, short-circuiting here shouldn't be necessary; the rest of the
+  // logic in this function should successfully not rewrite undefined functions
+  // when -infer-types-for-undef is not passed. This assumption could be
+  // transformed into an assertion if we're confident it won't fail in too many
+  // places.
+  if (!_3COpts.InferTypesForUndefs && !FDConstraint->hasBody())
     return true;
 
   // RewriteParams and RewriteReturn track if we will need to rewrite the

clang/lib/3C/ProgramInfo.cpp

@@ -417,78 +417,54 @@ bool ProgramInfo::link() {
 
   // For every global function that is an unresolved external, constrain
   // its parameter types to be wild. Unless it has a bounds-safe annotation.
-  for (const auto &U : ExternalFunctionFVCons) {
-    std::string FuncName = U.first;
-    FVConstraint *G = U.second;
-
-    // If there was a checked type on a variable in the input program, it
-    // should stay that way. Otherwise, we shouldn't be adding a checked type
-    // to an extern function.
-    std::string Rsn = (G->hasBody() ? ""
-                                    : "Unchecked pointer in parameter or "
-                                      "return of external function " +
-                                          FuncName);
-
-    // Handle the cases where itype parameters should not be treated as their
-    // unchecked type.
-    // TODO: Ditto re getting a PSL (in the case in which Rsn is non-empty and
-    // it is actually used).
-    G->equateWithItype(*this, Rsn, nullptr);
-
-    // If we've seen this symbol, but never seen a body for it, constrain
-    // everything about it.
-    // Some global symbols we don't need to constrain to wild, like
-    // malloc and free. Check those here and skip if we find them.
-    if (!G->hasBody()) {
-      const FVComponentVariable *Ret = G->getCombineReturn();
-      Ret->getInternal()->constrainToWild(CS, Rsn);
-      if (!Ret->getExternal()->srcHasItype() &&
-          !Ret->getExternal()->isGeneric())
-        Ret->getExternal()->constrainToWild(CS, Rsn);
-
-      for (unsigned I = 0; I < G->numParams(); I++) {
-        const FVComponentVariable *Param = G->getCombineParam(I);
-        Param->getInternal()->constrainToWild(CS, Rsn);
-        if (!Param->getExternal()->srcHasItype() &&
-            !Param->getExternal()->isGeneric())
-          Param->getExternal()->constrainToWild(CS, Rsn);
-      }
-    }
-  }
+  for (const auto &U : ExternalFunctionFVCons)
+    linkFunction(U.second);
 
   // Repeat for static functions.
   //
   // Static functions that don't have a body will always cause a linking
   // error during compilation. They may still be useful as code is developed,
   // so we treat them as if they are external, and constrain parameters
   // to wild as appropriate.
-  for (const auto &U : StaticFunctionFVCons) {
-    for (const auto &V : U.second) {
+  for (const auto &U : StaticFunctionFVCons)
+    for (const auto &V : U.second)
+      linkFunction(V.second);
 
-      std::string FileName = U.first;
-      std::string FuncName = V.first;
-      FVConstraint *G = V.second;
-
-      std::string Rsn = (G->hasBody() ? ""
-                                      : "Unchecked pointer in parameter or "
-                                        "return of static function " +
-                                            FuncName + " in " + FileName);
-
-      // TODO: Ditto re getting a PSL
-      G->equateWithItype(*this, Rsn, nullptr);
+  return true;
+}
 
-      if (!G->hasBody()) {
+void ProgramInfo::linkFunction(FunctionVariableConstraint *FV) {
+  // If there was a checked type on a variable in the input program, it
+  // should stay that way. Otherwise, we shouldn't be adding a checked type
+  // to an undefined function.
+  std::string Rsn = (FV->hasBody() ? "" : "Unchecked pointer in parameter or "
+                                          "return of undefined function " +
+                                          FV->getName());
+
+  // Handle the cases where itype parameters should not be treated as their
+  // unchecked type.
+  // TODO: Ditto re getting a PSL (in the case in which Rsn is non-empty and
+  // it is actually used).
+  FV->equateWithItype(*this, Rsn, nullptr);
+
+  // Used to apply constraints to parameters and returns for function without a
+  // body. In the default configuration, the function is fully constrained so
+  // that parameters and returns are considered unchecked. When 3C is run with
+  // --infer-types-for-undefs, only internal variables are constrained, allowing
+  // external variables to solve to checked types meaning the parameter will be
+  // rewritten to an itype.
+  auto LinkComponent = [this, Rsn](const FVComponentVariable *FVC) {
+    FVC->getInternal()->constrainToWild(CS, Rsn);
+    if (!_3COpts.InferTypesForUndefs &&
+        !FVC->getExternal()->srcHasItype() && !FVC->getExternal()->isGeneric())
+      FVC->getExternal()->constrainToWild(CS, Rsn);
+  };
 
-        if (!G->getExternalReturn()->isGeneric())
-          G->getExternalReturn()->constrainToWild(CS, Rsn);
-        for (unsigned I = 0; I < G->numParams(); I++)
-          if (!G->getExternalParam(I)->isGeneric())
-            G->getExternalParam(I)->constrainToWild(CS, Rsn);
-      }
-    }
+  if (!FV->hasBody()) {
+    LinkComponent(FV->getCombineReturn());
+    for (unsigned I = 0; I < FV->numParams(); I++)
+      LinkComponent(FV->getCombineParam(I));
   }
-
-  return true;
 }
 
 // Populate Variables, VarDeclToStatement, RVariables, and DepthMap with

clang/lib/3C/Utils.cpp

@@ -387,11 +387,15 @@ bool isInSysHeader(clang::Decl *D) {
 
 std::string getSourceText(const clang::SourceRange &SR,
                           const clang::ASTContext &C) {
+  return getSourceText(CharSourceRange::getTokenRange(SR), C);
+}
+
+std::string getSourceText(const clang::CharSourceRange &SR,
+                          const clang::ASTContext &C) {
   assert(SR.isValid() && "Invalid Source Range requested.");
   auto &SM = C.getSourceManager();
   auto LO = C.getLangOpts();
-  llvm::StringRef Srctxt =
-      Lexer::getSourceText(CharSourceRange::getTokenRange(SR), SM, LO);
+  llvm::StringRef Srctxt = Lexer::getSourceText(SR, SM, LO);
   return Srctxt.str();
 }
 

clang/test/3C/implicit_casts_root_cause.c

@@ -11,7 +11,7 @@ void test_no_cause() {
   has_void(c);
 }
 
-void has_float(float* v); // expected-warning {{Unchecked pointer in parameter or return of external function has_float}}
+void has_float(float* v); // expected-warning {{Unchecked pointer in parameter or return of undefined function has_float}}
 void test_float_cause() {
   int *b, *c;
   has_float(b); // expected-warning {{1 unchecked pointer: Cast from int * to float *}}