[PATCH] D138859: [ODRHash] Drive attribute hashing through TableGen. NFC intended.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D138859#3955578 <https://reviews.llvm.org/D138859#3955578>, @vsapsai wrote:

> In D138859#3954975 <https://reviews.llvm.org/D138859#3954975>, @erichkeane wrote:
>
>> The hash there isn't the problem, its that you're adding a field to Attr.td that isn't serialized in ASTWriter/ASTReader.
>
> That's a good point. Sorry I haven't realized it at first and thanks for your patience. So, `IsODRHashable` is a property of the attribute kind, not the attribute instance. Unfortunately, I don't know what is the appropriate way to fix FIXME below (and how urgent it is).
>
>   // FIXME: These are properties of the attribute kind, not state for this
>   // instance of the attribute.
>   ...
>   unsigned IsODRHashable : 1;
>
> Anyway, big chunk of attribute deserialization happens in "AttrPCHRead.inc" and for AMDGPUFlatWorkGroupSizeAttr (non-trivial example) we have
>
>   case attr::AMDGPUFlatWorkGroupSize: {
>     bool isInherited = Record.readInt();
>     bool isImplicit = Record.readInt();
>     bool isPackExpansion = Record.readInt();
>     Expr * min = Record.readExpr();
>     Expr * max = Record.readExpr();
>     New = new (Context) AMDGPUFlatWorkGroupSizeAttr(Context, Info, min, max);
>     cast<InheritableAttr>(New)->setInherited(isInherited);
>     New->setImplicit(isImplicit);
>     New->setPackExpansion(isPackExpansion);
>     break;
>   }
>
> which calls the generated constructor
>
>   AMDGPUFlatWorkGroupSizeAttr::AMDGPUFlatWorkGroupSizeAttr(ASTContext &Ctx, const AttributeCommonInfo &CommonInfo
>                 , Expr * Min
>                 , Expr * Max
>                )
>     : InheritableAttr(Ctx, CommonInfo, attr::AMDGPUFlatWorkGroupSize, false, false, false)
>                 , min(Min)
>                 , max(Max)
>     {
>   }
>
> where `false, false, false` part corresponds to `bool IsLateParsed, bool IsODRHashable, bool InheritEvenIfAlreadyPresent`. So `IsODRHashable` is never serialized/deserialized and compiler knows what the value should be. It can be a problem if one clang version writes a .pcm file and another version reads it because we can change if an attribute is hashable over time. But as far as I know, we don't support such cross-version scenario already.

The explanation makes sense to me.

And I think we should add tests for this. e.g., in the ASTImporterTests.cpp.



================
Comment at: clang/lib/AST/AttrImpl.cpp:16
 #include "clang/AST/Expr.h"
+#include "clang/AST/ODRHash.h"
 #include "clang/AST/Type.h"
----------------
Is this change necessary?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138859