This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by davezarzycki on Jan 24 2020, 8:59 AM.

Diff Detail

Event Timeline

davezarzycki created this revision.Jan 24 2020, 8:59 AM
lebedev.ri accepted this revision.Jan 24 2020, 9:15 AM

SGTM, thank you.

This revision is now accepted and ready to land.Jan 24 2020, 9:15 AM
This revision was automatically updated to reflect the committed changes.
rnk added a subscriber: rnk.Jan 30 2020, 1:32 PM
rnk added inline comments.
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 added a comment.Jan 30 2020, 1:39 PM

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 added inline comments.
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 added inline comments.Mar 5 2021, 5:11 AM
clang/lib/CodeGen/CGValue.h
18

It was fixed over a year ago: 01943a59f51d8b5ede062305941c1f864b8a6a13

aaron.ballman added inline comments.Mar 5 2021, 5:17 AM
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 added inline comments.Mar 5 2021, 5:30 AM
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 added inline comments.Mar 5 2021, 5:40 AM
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 added inline comments.Mar 5 2021, 6:10 AM
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 added inline comments.Mar 5 2021, 6:23 AM
clang/lib/CodeGen/CGValue.h
18

Thank you for your help on this!