[PATCH] D135472: [ODRHash] Hash attributes on declarations.

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Most of my concerns change based on the feedback others have given, so after those are dealt with, I'll do another depever view.



================
Comment at: clang/include/clang/Basic/DiagnosticASTKinds.td:852
+def err_module_odr_violation_attribute : Error<
+  "%q0 has different definitions in different modules; first difference is "
+  "%select{definition in module '%2'|defined here}1 found"
----------------
Wowzer these are tough to read... can you provide a magic-decoder ring for me of some sort?


================
Comment at: clang/lib/AST/ODRHash.cpp:479-480
+
+  llvm::copy_if(D->attrs(), std::back_inserter(HashableAttrs),
+                [](const Attr *A) { return !A->isImplicit(); });
+}
----------------
aaron.ballman wrote:
> ChuanqiXu wrote:
> > vsapsai wrote:
> > > I'm not sure `isImplicit` is the best indicator of attributes to check, so suggestions in this area are welcome. I think we can start strict and relax some of the checks if needed.
> > > 
> > > If people have strong opinions that some attributes shouldn't be ignored, we can add them to the tests to avoid regressions. Personally, I believe that alignment and packed attributes should never be silently ignored.
> > Agreed. I feel `isImplicit` is enough for now.
> The tricky part is -- sometimes certain attributes add additional implicit attributes and those implicit attributes matter (https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDeclAttr.cpp#L9380). And some attributes seem to just do the wrong thing entirely: https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDeclAttr.cpp#L7344
> 
> So I think `isImplicit()` is a good approximation, but I'm more wondering what the principle is for whether an attribute should or should not be considered part of the ODR hash. Type attributes, attributes that impact object layout, etc all seem like they should almost certainly be part of ODR hashing. Others are a bit more questionable though.
> 
> I think this is something that may need a per-attribute flag in Attr.td so attributes can opt in or out of it because I'm not certain what ODR issues could stem from `[[maybe_unused]]` or `[[deprecated]]` disagreements across module boundaries.
I don't think 'isImplicit' is particularly good.  I think the idea of auto-adding 'type' attributes and having the 'rest' be analyzed to figure out which are important.

Alternatively, I wonder if we're better off just adding ALL attributes and seeing what the fallout is. We can later decide when we don't want them to be ODR significant (which, might be OTHERWISE meaningful later!).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135472