[ASTImporter] Fields are imported first and reordered for correct layout.

[danix800]

Summary

Fields are imported first and reordered for correct layout.

For partially imported record, layout computation is incorrect.

[danix800]

Apply git-clang-format.

[danix800]

Fields are imported first and reordered for correct layout.

[balazske]

If I see correctly this change does that the re-ordered members are imported before all other, specially fields come before functions. This way order of fields is already correct when a function is imported.

clang/lib/AST/ASTImporter.cpp

1863 ↗

(On Diff #540254)

Braces are not required here (and at the other similar places).

[danix800]

In D154764#4505339, @balazske wrote:

If I see correctly this change does that the re-ordered members are imported before all other, specially fields come before functions. This way order of fields is already correct when a function is imported.

That's correct.

[danix800]

Remove unnecessary braces.

[shafik]

clang/unittests/AST/ASTImporterTest.cpp

8018 ↗

(On Diff #541400)

So is it the case that this caused f2 to be imported first and then f1? Which is the opposite of the example in the comments:

struct declToImport {
         int a = c + b;
         int b = 1;
         int c = 2;
     };

which causes the order to be c, b and then a.

[danix800]

clang/unittests/AST/ASTImporterTest.cpp

8018 ↗

(On Diff #541400)

The issue caused by this case

int m() {
   return &((A *)0)->f1 - &((A *)0)->f2;
}
int f1;
int f2;

is that before fixing, method m is imported first, which triggers expr (UnaryOperator) dependency computation, further relies on the correct RecordLayout of declToImport, but f1 & f2 is not imported at this moment.

This is not the opposite to the case like int a = c + b;, but both are caused by circular refs.

Similar record layout issue will also be triggered like:

class B;
class A {
  B* b;
  int c;
};
class B {
   A *f() { return &((B *)0)->a; }
   A a;
};

https://reviews.llvm.org/D156201 removes the unnecessary record layout as another fix to this kind of circular refs
(which cannot be fixed simply by re-adjusting import ordering).

[danix800]

clang/unittests/AST/ASTImporterTest.cpp

8018 ↗

(On Diff #541400)

Sorry for typos, they are both import ordering issues (not circular ref issues).

clang/include/clang/AST/ASTContext.h

   /// Get or compute information about the layout of the specified
   /// record (struct/union/class) \p D, which indicates its size and field
   /// position information.
-  const ASTRecordLayout &getASTRecordLayout(const RecordDecl *D) const;
+  const ASTRecordLayout &getASTRecordLayout(const RecordDecl *D,
+                                            bool NoCache = false) const;
 
   /// Get or compute information about the layout of the specified
   /// Objective-C interface.

clang/lib/AST/ExprConstant.cpp

     if (FD->getParent()->isInvalidDecl()) return false;
     RL = &Info.Ctx.getASTRecordLayout(FD->getParent());
   }
+  if (FD->getFieldIndex() >= RL->getFieldCount()) {
+    RL = &Info.Ctx.getASTRecordLayout(FD->getParent(), /* NoCache = */ true);
+  }
 
   unsigned I = FD->getFieldIndex();
   LVal.adjustOffset(Info.Ctx.toCharUnitsFromBits(RL->getFieldOffset(I)));

clang/lib/AST/RecordLayoutBuilder.cpp

 /// getASTRecordLayout - Get or compute information about the layout of the
 /// specified record (struct/union/class), which indicates its size and field
 /// position information.
-const ASTRecordLayout &
-ASTContext::getASTRecordLayout(const RecordDecl *D) const {
+const ASTRecordLayout &ASTContext::getASTRecordLayout(const RecordDecl *D,
+                                                      bool NoCache) const {
   // These asserts test different things.  A record has a definition
   // as soon as we begin to parse the definition.  That definition is
   // not a complete definition (which is what isDefinition() tests)

   // Look up this layout, if already laid out, return what we have.
   // Note that we can't save a reference to the entry because this function
   // is recursive.
-  const ASTRecordLayout *Entry = ASTRecordLayouts[D];
-  if (Entry) return *Entry;
+  if (!NoCache) {
+    const ASTRecordLayout *Entry = ASTRecordLayouts[D];
+    if (Entry)
+      return *Entry;
+  }
 
   const ASTRecordLayout *NewEntry = nullptr;
 

clang/test/Analysis/astimport-force-relayout/Inputs/api.cc

+#include "v8.h"
+
+typedef long int intptr_t;
+
+namespace v8 {
+
+class Deoptimizer {
+public:
+  static int input_offset() {
+    return (reinterpret_cast<intptr_t>(
+                &(reinterpret_cast<Deoptimizer *>(16)->input_)) -
+            16);
+  }
+
+  static int output_count_offset() {
+    return (reinterpret_cast<intptr_t>(
+                &(reinterpret_cast<Deoptimizer *>(16)->output_count_)) -
+            16);
+  }
+
+private:
+  int input_;
+  int output_count_;
+};
+
+Local<Object> Function::NewInstance(Local<Context> context, int argc,
+                                    Local<Value> argv[]) const {
+  return NewInstanceWithSideEffectType(context, argc, argv);
+}
+
+} // namespace v8

clang/test/Analysis/astimport-force-relayout/Inputs/externalDefMap.txt

+45:c:@N@v8@S@Deoptimizer@F@output_count_offset#S api.cc.ast
+38:c:@N@v8@S@Deoptimizer@F@input_offset#S api.cc.ast
+101:c:@N@v8@S@Function@F@NewInstance#$@N@v8@S@Local>#$@N@v8@S@Context#I#*$@N@v8@S@Local>#$@N@v8@S@Value#1 api.cc.ast

clang/test/Analysis/astimport-force-relayout/Inputs/v8.h

+#ifndef INCLUDE_V8_H_
+#define INCLUDE_V8_H_
+
+namespace v8 {
+
+class Context;
+class Object;
+class Value;
+
+template <class T>
+class Local {};
+
+class Function {
+ public:
+  Local<Object> NewInstance(Local<Context> context, int argc,
+                            Local<Value> argv[]) const;
+
+  Local<Object> NewInstance(Local<Context> context) const {
+    return NewInstance(context, 0, nullptr);
+  }
+
+  Local<Object> NewInstanceWithSideEffectType(Local<Context> context, int argc,
+                                              Local<Value> argv[]) const;
+};
+
+}  // namespace v8
+
+#endif  // INCLUDE_V8_H_

clang/test/Analysis/astimport-force-relayout/date.cpp

+// RUN: rm -rf %t && mkdir %t
+// RUN: mkdir -p %t/ctudir
+
+// RUN: %clang -c -mthumb --target=armv7a-linux-androideabi10000 -x c++ \
+// RUN:   -std=gnu++17 %p/Inputs/api.cc -emit-ast -o %t/ctudir/api.cc.ast
+
+// RUN: cp %p/Inputs/externalDefMap.txt %t/ctudir/externalDefMap.txt
+
+// RUN: %clang_analyze_cc1 -triple thumbv7-unknown-linux-android10000 \
+// RUN:   -std=gnu++17 \
+// RUN:   -analyzer-checker=core.DivideZero \
+// RUN:   -analyzer-opt-analyze-headers \
+// RUN:   -analyzer-config experimental-enable-naive-ctu-analysis=true \
+// RUN:   -analyzer-config ctu-dir=%t/ctudir \
+// RUN:   -analyzer-config display-ctu-progress=true \
+// RUN:   -fno-signed-char %s
+
+// expected-no-diagnostics
+
+#include "Inputs/v8.h"