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

Chuanqi Xu via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Oct 13 01:14:05 PDT 2022


ChuanqiXu added inline comments.


================
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:
> > > vsapsai wrote:
> > > > erichkeane wrote:
> > > > > 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!).
> > > > One criteria to decide which attributes should be hashed is if they affect IRGen. But that's not exhaustive and I'm not sure how practical it is.
> > > > 
> > > > The rule I'm trying to follow right now is if declarations with different attributes can be substituted instead of each other. For example, structs with different memory layout cannot be interchanged, so it is reasonable to reject them.
> > > > 
> > > > But maybe we should review attributes on case-by-case basis. For example, for `[[deprecated]]` I think the best for developers is not to complain about it but merge the attributes and then have regular diagnostic about using a deprecated entity.
> > > One option was to hash `isa<InheritedAttr>` attributes as they are "sticky" and should be more significant, so shouldn't be ignored. But I don't think this argument is particularly convincing.
> > > 
> > > At this stage I think we can still afford to add all attributes and exclude some as-needed because modules adoption is still limited. Though I don't have strong feelings about this approach, just getting more restrictive later can be hard.
> > It looks not a bad idea to add all attributes as experiments, @vsapsai how do you feel about this?
> My intuition is that we're going to want this to be controlled from Attr.td on a case-by-case basis and to automatically generate the ODR hash code for attribute arguments.
> 
> We can either force every attribute to decide explicitly (this seems pretty disruptive as a final design but would be a really good way to ensure we audit all attributes if done as an experiment), or we can pick a default behavior to ODR hash/not. I suspect we're going to want to pick a default behavior based on whatever the audit tells us the most common situation is.
> 
> I think we're going to need something a bit more nuanced than "this attribute matters for ODR" because there are attribute arguments that don't always contribute to the entity (for example, we have "fake" arguments that are used to give the semantic attribute more information, there may also be cases where one argument matter and another argument does not such as `enable_if` where the condition matters greatly but the message doesn't matter much). So we might need a marking on the attribute level *and* on the parameter level to determine what all factors into attribute identity for generating the ODR hashing code. Hopefully we can avoid needing more granularity than that.
I feel the default behavior would be to do ODR hashes. I just took a quick look in Attr.td. And I feel most of them affecting the code generation. For example, there're a lot of attributes which is hardware related.

> because there are attribute arguments that don't always contribute to the entity

When you're talking about "entities", I'm not sure if you're talking the "entities" in the language spec or a general abstract ones. I mean, for example, the `always_inline` attribute doesn't contribute to the declaration from the language perspective. But it'll be super odd if we lost it. So I feel it may be better to change the requirement to be "affecting code generation"

Also, to be honest, I am even not sure if we should take "affecting code generation " as the requirement. For example, for the `preferred_name` attribute, this is used for better printing names. It doesn't affect code generation nor the entity you mentioned. But if we drop it, then the printing name may not be what users want. I'm not sure if this is desired.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135472



More information about the cfe-commits mailing list