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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

> In the near future I was planning to add various Objective-C and Swift attributes. For other attributes I don't know which are high-value. I definitely want to check attributes affecting the memory layout (alignment, packing) and believe I've addressed them (totally could have missed something).

It looks like you're planning to add them one by one, or do I misunderstand? It looks not so good to add them one by one. Maybe it'll be a better idea to add them in the Attr.td?



================
Comment at: clang/lib/AST/ODRDiagsEmitter.cpp:337-354
+    if (!LHS || !RHS || LHS->getKind() != RHS->getKind()) {
+      DiagError(AttributeKind)
+          << (LHS != nullptr) << LHS << FirstDecl->getSourceRange();
+      DiagNoteAttrLoc(LHS);
+      DiagNote(AttributeKind)
+          << (RHS != nullptr) << RHS << SecondDecl->getSourceRange();
+      DiagNoteAttrLoc(RHS);
----------------
vsapsai wrote:
> ChuanqiXu wrote:
> > I feel like we can merge these two statements.
> Sorry, I don't really get what two statements you are talking about. Is it 
> * `!LHS || !RHS || LHS->getKind() != RHS->getKind()`
> * `ComputeAttrODRHash(LHS) != ComputeAttrODRHash(RHS)`
> ?
Sorry for the ambiguity. Since `LHS->getKind() != RHS->getKind()` is covered by `ComputeAttrODRHash(LHS) != ComputeAttrODRHash(RHS)`. I feel like it is reasonable to:

```
if (!LHS || !RHS || ComputeAttrODRHash(LHS) != ComputeAttrODRHash(RHS)) {
    DiagError();
    DiagNote();
    DiagNote();
    DiagNoteAttrLoc();
    return true;
}
```


================
Comment at: clang/lib/AST/ODRHash.cpp:479-480
+
+  llvm::copy_if(D->attrs(), std::back_inserter(HashableAttrs),
+                [](const Attr *A) { return !A->isImplicit(); });
+}
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135472