[PATCH] D73363: Verify that clang's max alignment is <= LLVM's max alignment

[David Zarzycki via Phabricator via cfe-commits]

davezarzycki added inline comments.


================
Comment at: clang/lib/CodeGen/CGValue.h:18
 #include "clang/AST/ASTContext.h"
+#include "clang/Sema/Sema.h"
 #include "clang/AST/Type.h"
----------------
aaron.ballman wrote:
> davezarzycki wrote:
> > aaron.ballman wrote:
> > > davezarzycki wrote:
> > > > aaron.ballman wrote:
> > > > > rnk wrote:
> > > > > > 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
> > > > > 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.
> > > > It was fixed over a year ago: 01943a59f51d8b5ede062305941c1f864b8a6a13
> > > 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.
> > 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?
> > 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!
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73363/new/

https://reviews.llvm.org/D73363