Include getting generated struct offsets in CodegenABITypes

[mppf]

Summary

This change adds a new function, CodeGen::getFieldNumber, that
enables a user of clang's code generation to get the field number
in a generated LLVM IR struct that corresponds to a particular field
in a C struct.

It is important to expose this information in Clang's code generation
interface because there is no reasonable way for users of Clang's code
generation to get this information. In particular:

1. LLVM struct types do not include field names.
2. Clang adds a non-trivial amount of logic to the code generation of LLVM IR types for structs, in particular to handle padding and bit fields.

[mppf]

This in some ways is a philosophical continuation of https://reviews.llvm.org/D35180 .

I'm working on a programming language that uses Clang to create seamless C interoperability. Of of the language features is to create "extern records", which are just structs that have a definition in C code. The language needs to be able to code generate element access to particular structure fields by name, and without this patch, I need to use clang-internal headers to do so.

[aprantl]

This needs some kind of testcase. Either a unittest or a test-only executable similar to, for example, clang-index-test.

[mppf]

• Add a test of getFieldNumber (and others)

[rsmith]

I have no philosophical objection to exposing this, but defer to @rjmccall on that. As is usual with our C++ API, there's no guarantee that we will keep this working in future versions. (Eg, if we move to an IR representation using something non-GEPable to represent a source-level class type, we would remove this function as it would be meaningless.)

This interface does not seem to be sufficient to support bit-fields. You should also indicate *which* record layout (complete object type or base subobject type) the field number is for. I don't think there's any guarantee that the same indexes work in both.

[rjmccall]

include/clang/CodeGen/CodeGenABITypes.h

81

"Given a non-bitfield struct field, return its index within the elements of the struct's converted type."

83

I think "getLLVMFieldNumber" is probably the right name for this.

[mppf]

• Improve comments, rename to getLLVMFieldNumber

[mppf]

Thanks for the feedback!

"Given a non-bitfield struct field, return its index within the elements of the struct's converted type."

Done

I think "getLLVMFieldNumber" is probably the right name for this.

Great suggestion, done.

This interface does not seem to be sufficient to support bit-fields.

Right, supporting bit fields would make it more complex & isn't something I need right now anyway. With rjmccal's suggested comment, it's clear that bit fields won't work with this function. (Given a non-bitfield struct field...)

You should also indicate *which* record layout (complete object type or base subobject type) the field number is for. I don't think there's any guarantee that the same indexes work in both.

I added a comment about this - indicating that inherited fields are not supported by this function. My use case is just for C structs at the moment and I'm not actually sure what we'd need to do for the interface to support building GEPs for inherited fields. I'm sure it would complicate the API. I think we should leave that complexity to another function or a future function (when somebody actually needs it).

(Eg, if we move to an IR representation using something non-GEPable to represent a source-level class type, we would remove this function as it would be meaningless.)

In that event, I'd just want to have a way to build the appropriate field access. (e.g. a function to build an appropriate GEP; or a function that adds appropriate instructions to an IRBuilder and returns a Value* that points to the field, say). I'm open to trying to go directly to such an interface but I'd need significant help in designing/implementing it. (The interface I've proposed here meets my needs and I don't think I know enough about clang to design/implement a better one without help).

[rsmith]

In D38473#888159, @mppf wrote:

You should also indicate *which* record layout (complete object type or base subobject type) the field number is for. I don't think there's any guarantee that the same indexes work in both.

I added a comment about this - indicating that inherited fields are not supported by this function. My use case is just for C structs at the moment and I'm not actually sure what we'd need to do for the interface to support building GEPs for inherited fields. I'm sure it would complicate the API. I think we should leave that complexity to another function or a future function (when somebody actually needs it).

I don't think your added comment is related to my request. There are two (potentially) different LLVM types for a C++ class type in general, one for the case where the object is a base class subobject (excluding vbases, and where the tail padding could be reused) and one for where it is a complete object (including vbases, and where the tail padding is known to be owned by the object). I don't know whether we guarantee that field indexes for one layout can be used for the other layout; in particular, I'm concerned that one could be a packed struct when the other isn't, and thus the field indexes could differ. (I think we generally use the complete object type within IR generation, even though that may seem like an odd choice.)

But perhaps we do guarantee the common prefixes are the same (@rjmccall: do we?), or we only ever expose one of the two layouts to code outside Clang, and in either case this wouldn't matter.

(Eg, if we move to an IR representation using something non-GEPable to represent a source-level class type, we would remove this function as it would be meaningless.)

In that event, I'd just want to have a way to build the appropriate field access. (e.g. a function to build an appropriate GEP; or a function that adds appropriate instructions to an IRBuilder and returns a Value* that points to the field, say). I'm open to trying to go directly to such an interface but I'd need significant help in designing/implementing it. (The interface I've proposed here meets my needs and I don't think I know enough about clang to design/implement a better one without help).

Given the generality of kinds of lvalues and rvalues that we support, this would mean exposing a significant chunk of the CodeGen interface, I think -- probably much more than we would be comfortable exposing.

[rjmccall]

In D38473#888597, @rsmith wrote:

In D38473#888159, @mppf wrote:

You should also indicate *which* record layout (complete object type or base subobject type) the field number is for. I don't think there's any guarantee that the same indexes work in both.

I added a comment about this - indicating that inherited fields are not supported by this function. My use case is just for C structs at the moment and I'm not actually sure what we'd need to do for the interface to support building GEPs for inherited fields. I'm sure it would complicate the API. I think we should leave that complexity to another function or a future function (when somebody actually needs it).

I don't think your added comment is related to my request. There are two (potentially) different LLVM types for a C++ class type in general, one for the case where the object is a base class subobject (excluding vbases, and where the tail padding could be reused) and one for where it is a complete object (including vbases, and where the tail padding is known to be owned by the object). I don't know whether we guarantee that field indexes for one layout can be used for the other layout; in particular, I'm concerned that one could be a packed struct when the other isn't, and thus the field indexes could differ. (I think we generally use the complete object type within IR generation, even though that may seem like an odd choice.)

But perhaps we do guarantee the common prefixes are the same (@rjmccall: do we?), or we only ever expose one of the two layouts to code outside Clang, and in either case this wouldn't matter.

I think we should probably just document that they're only valid in the complete-object type, which also, yes, happens to be the only type we expose outside of Clang because it's the type returned by convertTypeForMemory.

(Eg, if we move to an IR representation using something non-GEPable to represent a source-level class type, we would remove this function as it would be meaningless.)

In that event, I'd just want to have a way to build the appropriate field access. (e.g. a function to build an appropriate GEP; or a function that adds appropriate instructions to an IRBuilder and returns a Value* that points to the field, say). I'm open to trying to go directly to such an interface but I'd need significant help in designing/implementing it. (The interface I've proposed here meets my needs and I don't think I know enough about clang to design/implement a better one without help).

Given the generality of kinds of lvalues and rvalues that we support, this would mean exposing a significant chunk of the CodeGen interface, I think -- probably much more than we would be comfortable exposing.

I thought about making this sort of comment and decided that if we added more non-GEPable kinds of fields, we could always just put them after bit-fields in the exceptions list.

[mppf]

• Update comment for "complete object type"

[mppf]

@rjmccall , @rsmith - I added a comment about complete object type. I think I've addressed all the concerns at this point, but speak up if I'm misunderstanding.

I don't have commit access so I'll need help committing it. Thanks for your help!

[rsmith]

Looks good to me.

[mppf]

Since I don't have commit access, could somebody commit this for me please? Thanks.

[rsmith]

@rjmccall Do you have any more comments?

[aprantl]

I can commit this for you if John is happy with it.

[rjmccall]

LGTM.

include/clang/CodeGen/CodeGenABITypes.h

const CGFunctionInfo &arrangeFreeFunctionCall(CodeGenModule &CGM,
                                               RequiredArgs args);
 
 // Returns null if the function type is incomplete and can't be lowered.
 llvm::FunctionType *convertFreeFunctionType(CodeGenModule &CGM,
                                             const FunctionDecl *FD);
 
 llvm::Type *convertTypeForMemory(CodeGenModule &CGM, QualType T);
 
+// Given a non-bitfield struct field, return its index within the elements of
+// the struct's converted type.
+// The returned index refers to a field number in the complete object type
+// which is returned by convertTypeForMemory.
+// FD must be a field in RD directly (i.e. not an inherited field).
+unsigned getLLVMFieldNumber(CodeGenModule &CGM,
+                            const RecordDecl *RD, const FieldDecl *FD);
+
 }  // end namespace CodeGen
 }  // end namespace clang
 
 #endif

lib/CodeGen/CodeGenABITypes.cpp

 // types.
 //
 // The class is implemented as a public wrapper around the private
 // CodeGenTypes class in lib/CodeGen.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/CodeGen/CodeGenABITypes.h"
+#include "CGRecordLayout.h"
 #include "CodeGenModule.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
 #include "clang/Frontend/CodeGenOptions.h"
 #include "clang/Lex/HeaderSearchOptions.h"
 #include "clang/Lex/PreprocessorOptions.h"
 
 using namespace clang;
 using namespace CodeGen;
CodeGen::convertFreeFunctionType(CodeGenModule &CGM, const FunctionDecl *FD) {
 
   return nullptr;
 }
 
 llvm::Type *
 CodeGen::convertTypeForMemory(CodeGenModule &CGM, QualType T) {
   return CGM.getTypes().ConvertTypeForMem(T);
 }
+
+unsigned CodeGen::getLLVMFieldNumber(CodeGenModule &CGM,
+                                     const RecordDecl *RD,
+                                     const FieldDecl *FD) {
+  return CGM.getTypes().getCGRecordLayout(RD).getLLVMFieldNo(FD);
+}

unittests/CodeGen/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Core
   Support
   )
 
 add_clang_unittest(ClangCodeGenTests
   BufferSourceTest.cpp
   IncrementalProcessingTest.cpp
+  CodeGenExternalTest.cpp
   )
 
 target_link_libraries(ClangCodeGenTests
   clangBasic
   clangCodeGen
   clangFrontend
   clangLex
   clangParse
   )

unittests/CodeGen/CodeGenExternalTest.cpp

+//===- unittests/CodeGen/CodeGenExternalTest.cpp - test external CodeGen -===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/AST/ASTConsumer.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/GlobalDecl.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/Basic/TargetInfo.h"
+#include "clang/CodeGen/CodeGenABITypes.h"
+#include "clang/CodeGen/ModuleBuilder.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Lex/Preprocessor.h"
+#include "clang/Parse/ParseAST.h"
+#include "clang/Sema/Sema.h"
+#include "llvm/ADT/Triple.h"
+#include "llvm/IR/Instructions.h"
+#include "llvm/IR/LLVMContext.h"
+#include "llvm/Support/Debug.h"
+#include "llvm/Support/Host.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace clang;
+
+namespace {
+
+// Mocks up a language using Clang code generation as a library and
+// tests some basic functionality there.
+//   - CodeGen->GetAddrOfGlobal
+//   - CodeGen::convertTypeForMemory
+//   - CodeGen::getLLVMFieldNumber
+
+static const bool DebugThisTest = false;
+
+// forward declarations
+struct MyASTConsumer;
+static void test_codegen_fns(MyASTConsumer *my);
+static bool test_codegen_fns_ran;
+
+// This forwards the calls to the Clang CodeGenerator
+// so that we can test CodeGen functions while it is open.
+// It accumulates toplevel decls in HandleTopLevelDecl and
+// calls test_codegen_fns() in HandleTranslationUnit
+// before forwarding that function to the CodeGenerator.
+
+struct MyASTConsumer : public ASTConsumer {
+  std::unique_ptr<CodeGenerator> Builder;
+  std::vector<Decl*> toplevel_decls;
+
+  MyASTConsumer(std::unique_ptr<CodeGenerator> Builder_in)
+    : ASTConsumer(), Builder(std::move(Builder_in))
+  {
+  }
+
+  ~MyASTConsumer() { }
+
+  void Initialize(ASTContext &Context) override;
+  void HandleCXXStaticMemberVarInstantiation(VarDecl *VD) override;
+  bool HandleTopLevelDecl(DeclGroupRef D) override;
+  void HandleInlineFunctionDefinition(FunctionDecl *D) override;
+  void HandleInterestingDecl(DeclGroupRef D) override;
+  void HandleTranslationUnit(ASTContext &Ctx) override;
+  void HandleTagDeclDefinition(TagDecl *D) override;
+  void HandleTagDeclRequiredDefinition(const TagDecl *D) override;
+  void HandleCXXImplicitFunctionInstantiation(FunctionDecl *D) override;
+  void HandleTopLevelDeclInObjCContainer(DeclGroupRef D) override;
+  void HandleImplicitImportDecl(ImportDecl *D) override;
+  void CompleteTentativeDefinition(VarDecl *D) override;
+  void AssignInheritanceModel(CXXRecordDecl *RD) override;
+  void HandleVTable(CXXRecordDecl *RD) override;
+  ASTMutationListener *GetASTMutationListener() override;
+  ASTDeserializationListener *GetASTDeserializationListener() override;
+  void PrintStats() override;
+  bool shouldSkipFunctionBody(Decl *D) override;
+};
+
+void MyASTConsumer::Initialize(ASTContext &Context) {
+  Builder->Initialize(Context);
+}
+
+bool MyASTConsumer::HandleTopLevelDecl(DeclGroupRef DG) {
+
+  for (DeclGroupRef::iterator I = DG.begin(), E = DG.end(); I != E; ++I) {
+    toplevel_decls.push_back(*I);
+  }
+
+  return Builder->HandleTopLevelDecl(DG);
+}
+
+void MyASTConsumer::HandleInlineFunctionDefinition(FunctionDecl *D) {
+  Builder->HandleInlineFunctionDefinition(D);
+}
+
+void MyASTConsumer::HandleInterestingDecl(DeclGroupRef D) {
+  Builder->HandleInterestingDecl(D);
+}
+
+void MyASTConsumer::HandleTranslationUnit(ASTContext &Context) {
+  test_codegen_fns(this);
+  // HandleTranslationUnit can close the module
+  Builder->HandleTranslationUnit(Context);
+}
+
+void MyASTConsumer::HandleTagDeclDefinition(TagDecl *D) {
+  Builder->HandleTagDeclDefinition(D);
+}
+
+void MyASTConsumer::HandleTagDeclRequiredDefinition(const TagDecl *D) {
+  Builder->HandleTagDeclRequiredDefinition(D);
+}
+
+void MyASTConsumer::HandleCXXImplicitFunctionInstantiation(FunctionDecl *D) {
+  Builder->HandleCXXImplicitFunctionInstantiation(D);
+}
+
+void MyASTConsumer::HandleTopLevelDeclInObjCContainer(DeclGroupRef D) {
+  Builder->HandleTopLevelDeclInObjCContainer(D);
+}
+
+void MyASTConsumer::HandleImplicitImportDecl(ImportDecl *D) {
+  Builder->HandleImplicitImportDecl(D);
+}
+
+void MyASTConsumer::CompleteTentativeDefinition(VarDecl *D) {
+  Builder->CompleteTentativeDefinition(D);
+}
+
+void MyASTConsumer::AssignInheritanceModel(CXXRecordDecl *RD) {
+  Builder->AssignInheritanceModel(RD);
+}
+
+void MyASTConsumer::HandleCXXStaticMemberVarInstantiation(VarDecl *VD) {
+   Builder->HandleCXXStaticMemberVarInstantiation(VD);
+}
+
+void MyASTConsumer::HandleVTable(CXXRecordDecl *RD) {
+   Builder->HandleVTable(RD);
+ }
+
+ASTMutationListener *MyASTConsumer::GetASTMutationListener() {
+  return Builder->GetASTMutationListener();
+}
+
+ASTDeserializationListener *MyASTConsumer::GetASTDeserializationListener() {
+  return Builder->GetASTDeserializationListener();
+}
+
+void MyASTConsumer::PrintStats() {
+  Builder->PrintStats();
+}
+
+bool MyASTConsumer::shouldSkipFunctionBody(Decl *D) {
+  return Builder->shouldSkipFunctionBody(D);
+}
+
+const char TestProgram[] =
+    "struct mytest_struct { char x; short y; char p; long z; };\n"
+    "int mytest_fn(int x) { return x; }\n";
+
+// This function has the real test code here
+static void test_codegen_fns(MyASTConsumer *my) {
+
+  bool mytest_fn_ok = false;
+  bool mytest_struct_ok = false;
+
+  CodeGen::CodeGenModule &CGM = my->Builder->CGM();
+
+  for (auto decl : my->toplevel_decls ) {
+    if (FunctionDecl *fd = dyn_cast<FunctionDecl>(decl)) {
+      if (fd->getName() == "mytest_fn") {
+        Constant *c = my->Builder->GetAddrOfGlobal(GlobalDecl(fd), false);
+        // Verify that we got a function.
+        ASSERT_TRUE(c != NULL);
+        if (DebugThisTest) {
+          c->print(dbgs(), true);
+          dbgs() << "\n";
+        }
+        mytest_fn_ok = true;
+      }
+    } else if(clang::RecordDecl *rd = dyn_cast<RecordDecl>(decl)) {
+      if (rd->getName() == "mytest_struct") {
+        RecordDecl *def = rd->getDefinition();
+        ASSERT_TRUE(def != NULL);
+        const clang::Type *clangTy = rd->getCanonicalDecl()->getTypeForDecl();
+        ASSERT_TRUE(clangTy != NULL);
+        QualType qType = clangTy->getCanonicalTypeInternal();
+
+        // Check convertTypeForMemory
+        llvm::Type *llvmTy = CodeGen::convertTypeForMemory(CGM, qType);
+        ASSERT_TRUE(llvmTy != NULL);
+        if (DebugThisTest) {
+          llvmTy->print(dbgs(), true);
+          dbgs() << "\n";
+        }
+
+        llvm::CompositeType* structTy = dyn_cast<CompositeType>(llvmTy);
+        ASSERT_TRUE(structTy != NULL);
+
+        // Check getLLVMFieldNumber
+        FieldDecl *xField = NULL;
+        FieldDecl *yField = NULL;
+        FieldDecl *zField = NULL;
+
+        for (auto field : rd->fields()) {
+          if (field->getName() == "x") xField = field;
+          if (field->getName() == "y") yField = field;
+          if (field->getName() == "z") zField = field;
+        }
+
+        ASSERT_TRUE(xField != NULL);
+        ASSERT_TRUE(yField != NULL);
+        ASSERT_TRUE(zField != NULL);
+
+        unsigned x = CodeGen::getLLVMFieldNumber(CGM, rd, xField);
+        unsigned y = CodeGen::getLLVMFieldNumber(CGM, rd, yField);
+        unsigned z = CodeGen::getLLVMFieldNumber(CGM, rd, zField);
+
+        ASSERT_NE(x, y);
+        ASSERT_NE(y, z);
+
+        llvm::Type* xTy = structTy->getTypeAtIndex(x);
+        llvm::Type* yTy = structTy->getTypeAtIndex(y);
+        llvm::Type* zTy = structTy->getTypeAtIndex(z);
+
+        ASSERT_TRUE(xTy != NULL);
+        ASSERT_TRUE(yTy != NULL);
+        ASSERT_TRUE(zTy != NULL);
+
+        if (DebugThisTest) {
+          xTy->print(dbgs(), true);
+          dbgs() << "\n";
+          yTy->print(dbgs(), true);
+          dbgs() << "\n";
+          zTy->print(dbgs(), true);
+          dbgs() << "\n";
+        }
+
+        ASSERT_GE(xTy->getPrimitiveSizeInBits(), 1u);
+        ASSERT_GE(yTy->getPrimitiveSizeInBits(), 16u); // short is at least 16b
+        ASSERT_GE(zTy->getPrimitiveSizeInBits(), 32u); // long is at least 32b
+
+        mytest_struct_ok = true;
+      }
+    }
+  }
+
+  ASSERT_TRUE(mytest_fn_ok);
+  ASSERT_TRUE(mytest_struct_ok);
+
+  test_codegen_fns_ran = true;
+}
+
+TEST(CodeGenExternalTest, CodeGenExternalTest) {
+    LLVMContext Context;
+    CompilerInstance compiler;
+
+    compiler.createDiagnostics();
+    compiler.getLangOpts().CPlusPlus = 1;
+    compiler.getLangOpts().CPlusPlus11 = 1;
+
+    compiler.getTargetOpts().Triple = llvm::Triple::normalize(
+        llvm::sys::getProcessTriple());
+    compiler.setTarget(clang::TargetInfo::CreateTargetInfo(
+      compiler.getDiagnostics(),
+      std::make_shared<clang::TargetOptions>(
+        compiler.getTargetOpts())));
+
+    compiler.createFileManager();
+    compiler.createSourceManager(compiler.getFileManager());
+    compiler.createPreprocessor(clang::TU_Prefix);
+
+    compiler.createASTContext();
+
+
+    compiler.setASTConsumer(std::unique_ptr<ASTConsumer>(
+          new MyASTConsumer(std::unique_ptr<CodeGenerator>(
+             CreateLLVMCodeGen(compiler.getDiagnostics(),
+                               "MemoryTypesTest",
+                               compiler.getHeaderSearchOpts(),
+                               compiler.getPreprocessorOpts(),
+                               compiler.getCodeGenOpts(),
+                               Context)))));
+
+    compiler.createSema(clang::TU_Prefix, nullptr);
+
+    clang::SourceManager &sm = compiler.getSourceManager();
+    sm.setMainFileID(sm.createFileID(
+        llvm::MemoryBuffer::getMemBuffer(TestProgram), clang::SrcMgr::C_User));
+
+    clang::ParseAST(compiler.getSema(), false, false);
+
+    ASSERT_TRUE(test_codegen_fns_ran);
+}
+
+} // end anonymous namespace