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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
