aaron.ballman added inline comments.
================
Comment at: clang/lib/AST/ODRDiagsEmitter.cpp:302
+ llvm::raw_string_ostream OutputStream(FullText);
+ A->printPretty(OutputStream, Ctx.getPrintingPolicy());
+ return OutputStream.str();
----------------
vsapsai wrote:
> aaron.ballman wrote:
> > 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?
> `FullAttributeText` is used for diagnostics and not for the mismatch
> detection, so we shouldn't complain about `restrict/__ restrict` mismatches
> (`DifferentAlignmentKeywords` tests that `__attribute__((aligned(8)))` and
> `alignas(8)` are not a mismatch).
>
> We use the same `ASTContext` to print both attributes, so that shouldn't be
> confusing. Diagnostic can be unstable across clang versions and probably
> across language modes. But I think that can happen to other diagnostic
> messages too, so think that's acceptable.
Oh, that's great, thank you!
================
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:
> > 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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135472/new/
https://reviews.llvm.org/D135472
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
