Remove all the old workarounds for unnamed structs.

mattmccutchen-cci · mattmccutchen-cci · commit 81845566f008 · 2021-09-14T15:56:19.000-04:00
Now that we automatically name them:

- When -alltypes is on, we no longer need to warn that the multi-decl
  may be rewritten incorrectly.

- When -alltypes is off, we no longer generate constraints to try to
  avoid the need to rewrite the multi-decl at all.

InlineStructDetector did one thing unrelated to its name: it constrained
all union fields wild. Move that logic to ProgramInfo::addVariable, make
the constraint reason message more specific, and update root_cause.c to
expect the new message.
diff --git a/clang/lib/3C/ConstraintBuilder.cpp b/clang/lib/3C/ConstraintBuilder.cpp
@@ -21,133 +21,18 @@
 using namespace llvm;
 using namespace clang;
 
-// This class is intended to locate inline struct definitions
-// in order to mark them wild or signal a warning as appropriate.
-class InlineStructDetector {
-public:
-  explicit InlineStructDetector() : LastRecordDecl(nullptr) {}
-
-  void processRecordDecl(RecordDecl *Declaration, ProgramInfo &Info,
-                         ASTContext *Context, ConstraintResolver CB) {
-    LastRecordDecl = Declaration;
-    if (RecordDecl *Definition = Declaration->getDefinition()) {
-      auto LastRecordLocation = Definition->getBeginLoc();
-      FullSourceLoc FL = Context->getFullLoc(Definition->getBeginLoc());
-      if (FL.isValid()) {
-        SourceManager &SM = Context->getSourceManager();
-        FileID FID = FL.getFileID();
-        const FileEntry *FE = SM.getFileEntryForID(FID);
-
-        // Detect whether this RecordDecl is part of an inline struct.
-        bool IsInLineStruct = false;
-        Decl *D = Declaration->getNextDeclInContext();
-        if (VarDecl *VD = dyn_cast_or_null<VarDecl>(D)) {
-          auto VarTy = VD->getType();
-          auto BeginLoc = VD->getBeginLoc();
-          auto EndLoc = VD->getEndLoc();
-          SourceManager &SM = Context->getSourceManager();
-          IsInLineStruct =
-              !isPtrOrArrayType(VarTy) && !VD->hasInit() &&
-              SM.isPointWithin(LastRecordLocation, BeginLoc, EndLoc);
-        }
-
-        if (FE && FE->isValid()) {
-          // We only want to re-write a record if it contains
-          // any pointer types, to include array types.
-          for (const auto &F : Definition->fields()) {
-            auto FieldTy = F->getType();
-            // If the RecordDecl is a union or in a system header
-            // and this field is a pointer, we need to mark it wild.
-            bool FieldInUnionOrSysHeader =
-                (FL.isInSystemHeader() || Definition->isUnion());
-            // Mark field wild if the above is true and the field is a pointer.
-            if (isPtrOrArrayType(FieldTy) &&
-                (FieldInUnionOrSysHeader || IsInLineStruct)) {
-              std::string Rsn = "Union or external struct field encountered";
-              CVarOption CV = Info.getVariable(F, Context);
-              CB.constraintCVarToWild(CV, Rsn);
-            }
-          }
-        }
-      }
-    }
-  }
-
-  void processVarDecl(VarDecl *VD, ProgramInfo &Info, ASTContext *Context,
-                      ConstraintResolver CB) {
-    // If the last seen RecordDecl is non-null and coincides with the current
-    // VarDecl (i.e. via an inline struct), we proceed as follows:
-    // If the struct is named, do nothing.
-    // If the struct is anonymous:
-    //      When alltypes is on, do nothing, but signal a warning to
-    //                           the user indicating its presence.
-    //      When alltypes is off, mark the VarDecl WILD in order to
-    //                           ensure the converted program compiles.
-    if (LastRecordDecl != nullptr) {
-      auto LastRecordLocation = LastRecordDecl->getBeginLoc();
-      auto BeginLoc = VD->getBeginLoc();
-      auto EndLoc = VD->getEndLoc();
-      auto VarTy = VD->getType();
-      SourceManager &SM = Context->getSourceManager();
-      bool IsInLineStruct =
-          SM.isPointWithin(LastRecordLocation, BeginLoc, EndLoc) &&
-          isPtrOrArrayType(VarTy);
-      bool IsNamedInLineStruct =
-          IsInLineStruct && LastRecordDecl->getNameAsString() != "";
-      if (IsInLineStruct && !IsNamedInLineStruct) {
-        if (!_3COpts.AllTypes) {
-          CVarOption CV = Info.getVariable(VD, Context);
-          CB.constraintCVarToWild(CV, "Inline struct encountered.");
-        } else {
-          reportCustomDiagnostic(Context->getDiagnostics(),
-                                 DiagnosticsEngine::Warning,
-                                 "\n Rewriting failed"
-                                 "for %q0 because an inline "
-                                 "or anonymous struct instance "
-                                 "was detected.\n Consider manually "
-                                 "rewriting by inserting the struct "
-                                 "definition inside the _Ptr "
-                                 "annotation.\n "
-                                 "EX. struct {int *a; int *b;} x; "
-                                 "_Ptr<struct {int *a; _Ptr<int> b;}>;",
-                                 VD->getLocation())
-              << VD;
-        }
-      }
-    }
-  }
-
-private:
-  RecordDecl *LastRecordDecl;
-};
-
 // This class visits functions and adds constraints to the
 // Constraints instance assigned to it.
 // Each VisitXXX method is responsible for looking inside statements
 // and imposing constraints on variables it uses
 class FunctionVisitor : public RecursiveASTVisitor<FunctionVisitor> {
 public:
   explicit FunctionVisitor(ASTContext *C, ProgramInfo &I, FunctionDecl *FD)
-      : Context(C), Info(I), Function(FD), CB(Info, Context),
-        ISD() {}
+      : Context(C), Info(I), Function(FD), CB(Info, Context) {}
 
   // T x = e
   bool VisitDeclStmt(DeclStmt *S) {
     // Introduce variables as needed.
-    for (const auto &D : S->decls()) {
-      if (RecordDecl *RD = dyn_cast<RecordDecl>(D)) {
-        ISD.processRecordDecl(RD, Info, Context, CB);
-      }
-      if (VarDecl *VD = dyn_cast<VarDecl>(D)) {
-        if (VD->isLocalVarDecl()) {
-          FullSourceLoc FL = Context->getFullLoc(VD->getBeginLoc());
-          SourceRange SR = VD->getSourceRange();
-          if (SR.isValid() && FL.isValid() && isPtrOrArrayType(VD->getType())) {
-            ISD.processVarDecl(VD, Info, Context, CB);
-          }
-        }
-      }
-    }
 
     // Process inits even for non-pointers because structs and union values
     // can contain pointers
@@ -433,7 +318,6 @@ class FunctionVisitor : public RecursiveASTVisitor<FunctionVisitor> {
   ProgramInfo &Info;
   FunctionDecl *Function;
   ConstraintResolver CB;
-  InlineStructDetector ISD;
 };
 
 class PtrToStructDef : public RecursiveASTVisitor<PtrToStructDef> {
@@ -482,15 +366,14 @@ class PtrToStructDef : public RecursiveASTVisitor<PtrToStructDef> {
 class ConstraintGenVisitor : public RecursiveASTVisitor<ConstraintGenVisitor> {
 public:
   explicit ConstraintGenVisitor(ASTContext *Context, ProgramInfo &I)
-      : Context(Context), Info(I), CB(Info, Context), ISD() {}
+      : Context(Context), Info(I), CB(Info, Context) {}
 
   bool VisitVarDecl(VarDecl *G) {
 
     if (G->hasGlobalStorage() && isPtrOrArrayType(G->getType())) {
       if (G->hasInit()) {
         CB.constrainLocalAssign(nullptr, G, G->getInit(), Same_to_Same);
       }
-      ISD.processVarDecl(G, Info, Context, CB);
     }
     return true;
   }
@@ -537,16 +420,10 @@ class ConstraintGenVisitor : public RecursiveASTVisitor<ConstraintGenVisitor> {
     return true;
   }
 
-  bool VisitRecordDecl(RecordDecl *Declaration) {
-    ISD.processRecordDecl(Declaration, Info, Context, CB);
-    return true;
-  }
-
 private:
   ASTContext *Context;
   ProgramInfo &Info;
   ConstraintResolver CB;
-  InlineStructDetector ISD;
 };
 
 // Some information about variables in the program is required by the type
diff --git a/clang/lib/3C/ProgramInfo.cpp b/clang/lib/3C/ProgramInfo.cpp
@@ -684,6 +684,11 @@ void ProgramInfo::addVariable(clang::DeclaratorDecl *D,
       unifyIfTypedef(QT, *AstContext, P);
       NewCV = P;
       NewCV->setValidDecl();
+      if (FlD->getParent()->isUnion()) {
+        // REVIEW: Should we also call NewCV->equateWithItype here? The previous
+        // code in InlineStructDetector didn't call it.
+        NewCV->constrainToWild(CS, "Union field encountered", &PLoc);
+      }
     }
   } else
     llvm_unreachable("unknown decl type");
diff --git a/clang/test/3C/root_cause.c b/clang/test/3C/root_cause.c
@@ -43,9 +43,9 @@ void test1() {
 
   union u {
     // unwritable-expected-warning@+1 {{0 unchecked pointers: Source code in non-writable file}}
-    int *a; // expected-warning {{1 unchecked pointer: Union or external struct field encountered}}
+    int *a; // expected-warning {{1 unchecked pointer: Union field encountered}}
     // unwritable-expected-warning@+1 {{0 unchecked pointers: Source code in non-writable file}}
-    int *b; // expected-warning {{1 unchecked pointer: Union or external struct field encountered}}
+    int *b; // expected-warning {{1 unchecked pointer: Union field encountered}}
   };
 
   void (*c)(void); // unwritable-expected-warning {{0 unchecked pointers: Source code in non-writable file}}
