Verify that clang's max alignment is <= LLVM's max alignment

[lebedev.ri]

SGTM, thank you.

[rnk]

clang/lib/CodeGen/CGValue.h

18

This includes Sema.h into every codegen file that uses CGValue.h (most of them). That seems bad for build time. :(

This also seems like a layering violation. CodeGen has no dependency on Sema:
https://github.com/llvm/llvm-project/blob/master/clang/lib/CodeGen/CMakeLists.txt#L104

[rnk]

I moved this include in rG01943a59f51d8b5ede062305941c1f864b8a6a13. I meant to paste this in the message, but I'll put it here, since the results were significant:

$ diff -u deps-before.txt deps-after.txt |  grep '^[-+] ' |  sort | uniq -c | sort -nr
     50 -    ../clang/include/clang/Sema/Weak.h
     50 -    ../clang/include/clang/Sema/TypoCorrection.h
     50 -    ../clang/include/clang/Sema/SemaConcept.h
     50 -    ../clang/include/clang/Sema/Sema.h
     50 -    ../clang/include/clang/Sema/Scope.h
     50 -    ../clang/include/clang/Sema/ObjCMethodList.h
     50 -    ../clang/include/clang/Sema/IdentifierResolver.h
     50 -    ../clang/include/clang/Sema/ExternalSemaSource.h
     50 -    ../clang/include/clang/Sema/CleanupInfo.h
     50 -    ../clang/include/clang/Sema/AnalysisBasedWarnings.h
     50 -    ../clang/include/clang/Basic/TemplateKinds.h
     50 -    ../clang/include/clang/AST/MangleNumberingContext.h
     50 -    ../clang/include/clang/AST/LocInfoType.h
     50 -    ../clang/include/clang/AST/Availability.h
     49 -    tools/clang/include/clang/Basic/AttrSubMatchRulesList.inc
     49 -    ../llvm/include/llvm/ADT/SmallBitVector.h
     49 -    ../clang/include/clang/Sema/ParsedAttr.h
     49 -    ../clang/include/clang/Sema/Ownership.h
     49 -    ../clang/include/clang/Sema/DeclSpec.h
     49 -    ../clang/include/clang/Basic/BitmaskEnum.h
     49 -    ../clang/include/clang/Basic/AttrSubjectMatchRules.h
     49 -    ../clang/include/clang/AST/NSAPI.h
     47 -    ../clang/include/clang/Lex/Token.h
     36 -    ../clang/include/clang/AST/ExprConcepts.h
     31 -    ../clang/include/clang/AST/StmtCXX.h
     21 -    tools/clang/include/clang/AST/Attrs.inc
     21 -    ../clang/include/clang/AST/Attr.h
     20 -    tools/clang/include/clang/Sema/AttrParsedAttrList.inc
     20 -    ../clang/include/clang/Basic/AttributeCommonInfo.h
      7 -    ../clang/include/clang/Basic/ExpressionTraits.h
      7 -    ../clang/include/clang/AST/ExprObjC.h
      7 -    ../clang/include/clang/AST/ExprCXX.h
      7 -    ../clang/include/clang/AST/DeclTemplate.h
      7 -    ../clang/include/clang/AST/ASTConcept.h

[aaron.ballman]

clang/lib/CodeGen/CGValue.h

18

I agree that this is a layering violation (Sema relies on CodeGen which now relies on Sema due to this change). We just ran into it in a downstream fork when we had to add clangSema to the codegen linker input to avoid linking errors. I'm a bit surprised given that the only use appears to be a static_assert that shouldn't require anything to be linked in, but here we are just the same.

I think this should be rolled back so that we don't get additional dependencies on Sema within CodeGen by accident. It helps that @rnk moved this change into CGDecl.cpp (limits the scope of where we may introduce accidental dependencies), but I don't think we should be including anything from Sema.h here.

[davezarzycki]

clang/lib/CodeGen/CGValue.h

18

It was fixed over a year ago: 01943a59f51d8b5ede062305941c1f864b8a6a13

[aaron.ballman]

clang/lib/CodeGen/CGValue.h

18

That fixes the transitive include issues @rnk raised, but is not a fix for the layering violation. It's still a layering violation because it's including Sema from within CodeGen.

[davezarzycki]

clang/lib/CodeGen/CGValue.h

18

Ah, sorry. It does seem odd and probably a bug that a static_assert would trigger this. Did you verify that the imported symbol dependency is triggered by this static_assert? Or is the problem the mere inclusion, not the static_assert itself? And is the actual dependency only happen in unoptimized/debug builds or release too?

In any case, I believe it was discussed at the time that the LLVM variable ought to be moved to some kind of policy/config header that both LLVM's CodeGen and clang's Sema can include. Would you be open to that?

[aaron.ballman]

clang/lib/CodeGen/CGValue.h

18

Ah, sorry. It does seem odd and probably a bug that a static_assert would trigger this. Did you verify that the imported symbol dependency is triggered by this static_assert? Or is the problem the mere inclusion, not the static_assert itself? And is the actual dependency only happen in unoptimized/debug builds or release too?

I'd have to go back to the developer who found it to get the exact details, but my belief is that it's the inclusion more so than the static assertion itself. FWIW, the dependency was happening when doing a shared libraries build (I believe in release mode). I can dig up those details if you'd like, but my concern is that even if the issue is the static assertion, including the header file makes it easy for someone to accidentally use other facilities from Sema.h under the mistaken belief they're fine to do so.

In any case, I believe it was discussed at the time that the LLVM variable ought to be moved to some kind of policy/config header that both LLVM's CodeGen and clang's Sema can include. Would you be open to that?

That sounds like a very good idea to me!

[davezarzycki]

clang/lib/CodeGen/CGValue.h

18

Okay. I sent out the RFC to llvm-dev. It was long overdue. Somebody just needed to volunteer, start the discussion, and get it over with.

[aaron.ballman]

clang/lib/CodeGen/CGValue.h

18

Thank you for your help on this!

clang/include/clang/Sema/Sema.h

bool shouldLinkPossiblyHiddenDecl(const NamedDecl *Old,
     return false;
   }
   bool shouldLinkPossiblyHiddenDecl(LookupResult &Old, const NamedDecl *New);
 
   void setupImplicitSpecialMemberType(CXXMethodDecl *SpecialMem,
                                       QualType ResultTy,
                                       ArrayRef<QualType> Args);
 
+public:
   /// The maximum alignment, same as in llvm::Value. We duplicate them here
   /// because that allows us not to duplicate the constants in clang code,
   /// which we must to since we can't directly use the llvm constants.
+  /// The value is verified against llvm here: lib/CodeGen/CGValue.h
   ///
   /// This is the greatest alignment value supported by load, store, and alloca
   /// instructions, and global values.
   static const unsigned MaxAlignmentExponent = 29;
   static const unsigned MaximumAlignment = 1u << MaxAlignmentExponent;
 
-public:
   typedef OpaquePtr<DeclGroupRef> DeclGroupPtrTy;
   typedef OpaquePtr<TemplateName> TemplateTy;
   typedef OpaquePtr<QualType> TypeTy;
 
   OpenCLOptions OpenCLFeatures;
   FPOptions FPFeatures;
 
   const LangOptions &LangOpts;

clang/lib/CodeGen/CGValue.h

 // fully represent the range of values for C L- and R- values.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_LIB_CODEGEN_CGVALUE_H
 #define LLVM_CLANG_LIB_CODEGEN_CGVALUE_H
 
 #include "clang/AST/ASTContext.h"
+#include "clang/Sema/Sema.h"
 #include "clang/AST/Type.h"
 #include "llvm/IR/Value.h"
 #include "llvm/IR/Type.h"
 #include "Address.h"
 #include "CodeGenTBAA.h"
 
+static_assert(clang::Sema::MaximumAlignment <= llvm::Value::MaximumAlignment,
+              "Clang max alignment greater than what LLVM supports?");
+
 namespace llvm {
   class Constant;
   class MDNode;
 }
 
 namespace clang {
 namespace CodeGen {
   class AggValueSlot;