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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: erichkeane.
aaron.ballman added inline comments.


================
Comment at: clang/include/clang/AST/ODRDiagsEmitter.h:119
 
+  /// Diagnose ODR mismatch in attributes between 2 Decl.
+  ///
----------------



================
Comment at: clang/lib/AST/ODRDiagsEmitter.cpp:159
 
 bool ODRDiagsEmitter::diagnoseSubMismatchTypedef(
     const NamedDecl *FirstRecord, StringRef FirstModule, StringRef SecondModule,
----------------
Typedefs can have attributes, so should this be updated as well? (alignment, ext vector types, mode attributes, etc all come to mind)


================
Comment at: clang/lib/AST/ODRDiagsEmitter.cpp:302
+    llvm::raw_string_ostream OutputStream(FullText);
+    A->printPretty(OutputStream, Ctx.getPrintingPolicy());
+    return OutputStream.str();
----------------
Do we want to have more control over the printing policy here? e.g., do we really want to claim an ODR difference if one module's printing policy specifies indentation of 8 and another specifies indentation of 4? Or if one module prints `restrict` while another prints `__restrict`, etc?


================
Comment at: clang/lib/AST/ODRDiagsEmitter.cpp:311
+           << FirstModule.empty() << FirstModule << (FirstContainer != nullptr)
+           << FirstDecl->getIdentifier() << DiffType;
+  };
----------------
You can probably drop the `getIdentifier()` here as the diagnostics engine knows how to print named declarations properly already.


================
Comment at: clang/lib/AST/ODRDiagsEmitter.cpp:318
+           << SecondModule << (FirstContainer != nullptr)
+           << SecondDecl->getIdentifier() << DiffType;
+  };
----------------
Same here.


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


================
Comment at: clang/lib/AST/ODRHash.cpp:494
+
+  // FIXME: This should be auto-generated as part of Attr.td
+  switch (A->getKind()) {
----------------
100% agreed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135472