Page MenuHomePhabricator

Append new attributes to the end of an AttributeList.
ClosedPublic

Authored by Meinersbur on Jun 12 2018, 3:12 PM.

Details

Summary

... instead of prepending it at the beginning (the original behavior since implemented in r122535 2010-12-23). This builds up an AttributeList in the the order in which the attributes appear in the source.

The reverse order caused nodes for attributes in the AST (e.g. LoopHint) to be in the reverse order, and therefore printed in the wrong order in -ast-dump. Some TODO comments mention this. The order was explicitly reversed for enable_if attribute overload resolution and name mangling, which is not necessary anymore with this patch.

The change unfortunately has some secondary effect, especially on diagnostic output. In the simplest cases, the CHECK lines or expected diagnostic were changed to the the new output. If the kind of error/warning changed, the attribute' order was changed instead.

This unfortunately causes some 'previous occurrence here' hints to be textually after the main marker. This typically happens when attributes are merged, but are incompatible to each other. Interchanging the role of the the main and note SourceLocation will also cause the case where two different declaration's attributes (in contrast to multiple attributes of the same declaration) are merged to be reverse. There is no easy fix because sometimes previous attributes are merged into a new declaration's attribute list, sometimes new attributes are added to a previous declaration's attribute list. Since 'previous occurrence here' pointing to locations after the main marker is not rare, I left the markers as-is; it is only relevant when the attributes are declared in the same declaration anyway.

Diff Detail

Repository
rL LLVM

Event Timeline

Meinersbur created this revision.Jun 12 2018, 3:12 PM
nicholas added inline comments.Jun 12 2018, 3:32 PM
lib/Sema/SemaOverload.cpp
6194 ↗(On Diff #151027)

This function shouldn't be necessary any more. All it's doing now is making an unnecessary copy.

8943 ↗(On Diff #151027)

This would become "auto Cand1Attrs = Cand1->specific_attrs<EnableIfAttr>();" but I think you can simplify that even further if you want. To do that you'd need to sink the "return Comparison::Worse;" inside the loop that follows it. If you don't do that you'll have to switch the calls to Cand1Attrs.size() and Cand2Attrs.size() into calls to std::distance, since llvm::iterator_range doesn't have a size() method.

Meinersbur added inline comments.Jun 12 2018, 3:39 PM
lib/Sema/SemaOverload.cpp
6194 ↗(On Diff #151027)

I tried to keep the diff small and obvious. I'll refactor it in the next diff update.

8943 ↗(On Diff #151027)

Thanks for the hint.

  • Remove obsolete comment
  • Refactor getOrderedEnableIfAttrs
Meinersbur marked 4 inline comments as done.Jun 13 2018, 2:24 PM
Meinersbur added inline comments.
lib/AST/ItaniumMangle.cpp
710–711 ↗(On Diff #151209)

Will remove this FIXME in the next update.

aaron.ballman added a subscriber: aaron.ballman.

Thank you for working on this odd detail of attributes!

Can you also simplify hasSameOverloadableAttrs() in ASTReaderDecl.cpp similar to what you did in SemaOverload.cpp (the copy seems spurious)?

include/clang/Sema/AttributeList.h
767 ↗(On Diff #151209)

This now means adding an attribute requires walking the entire attribute list to get to the end of it. I don't think this is a huge issue (attribute lists tend to be short), but it might be good to leave a comment explaining the issue and why it's acceptable for now.

Meinersbur marked an inline comment as done.
  • Remove FIXME
  • Add comment about O(n^2) execution time when adding attributes
  • Do not store enable_if attributes in temporary list
Meinersbur marked an inline comment as done.Jun 14 2018, 10:43 AM

Can you also simplify hasSameOverloadableAttrs() in ASTReaderDecl.cpp similar to what you did in SemaOverload.cpp (the copy seems spurious)?

Changed hasSameOverloadableAttrs. Apart to the comment about that the reversal of attributes being unimportant, it does not seem related to the new attribute order, though.
I made the iteration over AEnableIfAttrs and BEnableIfAttrs symmetric (and IMHO easier to understand), which unfortunately is not possible with a foreach (there are also a zip-iterators, but I am not sure how to check for different lengths without std::distance iterating over it separately).
I can change compareEnableIfAttrs as well on request.

aaron.ballman accepted this revision.Jun 19 2018, 8:17 AM

LGTM, thank you for this!

This revision is now accepted and ready to land.Jun 19 2018, 8:17 AM
This revision was automatically updated to reflect the committed changes.

I've added a couple of inline comments here - between this and the comments in the post-commit review from dlj it seems like we might want to revert this for now and figure out the best way forward.

Thanks!

-eric

test/Sema/attr-micromips.c
9 ↗(On Diff #151998)

This seems to reverse? What's going on here? There are other occurrences too.

test/Sema/attr-target-mv.c
98 ↗(On Diff #151998)

This appears to have broken a particular error message?

erichkeane added inline comments.
test/Sema/attr-target-mv.c
98 ↗(On Diff #151998)

Yeah, I actually noticed this last week, and @aaron.ballman and I have been discussing it. Unfortunately, a small list of attributes (including all calling conventions) cause some redistributing of attributes between DeclSpec and Declarators. Due to a bug in that area, linked-list pointers are being rewritten in a fairly mean fashion (it was previously not noticeable since these were at the end).

It is going to take a pretty good sized effort to fix (basically, remove the attributelist linked list-ness), but Aaron and I have been working on it for a few days.

Reverting this won't change my effort so I'll try to fix this in the meantime.

rsmith added a subscriber: rsmith.Jun 25 2018, 2:35 PM

This patch appears to have at least caused us to process attributes too many times in some cases. For example:

__attribute__((noreturn,noinline,unused)) void f();

before your patch gives this with clang -Xclang -ast-dump:

`-FunctionDecl 0xccef1e0 <<stdin>:1:1, col:50> col:48 f 'void () __attribute__((noreturn))'
  |-UnusedAttr 0xccef280 <col:34> unused
  `-NoInlineAttr 0xccef2c8 <col:25>

and after your patch gives this:

`-FunctionDecl 0xb913740 <<stdin>:1:1, col:50> col:48 f 'void () __attribute__((noreturn))'
  |-NoInlineAttr 0xb9137e0 <col:25>
  |-UnusedAttr 0xb913828 <col:34> unused
  |-NoInlineAttr 0xb913838 <col:25>
  `-UnusedAttr 0xb913848 <col:34> unused
Meinersbur added inline comments.Jun 25 2018, 2:50 PM
test/Sema/attr-micromips.c
9 ↗(On Diff #151998)

This is caused by the same phenomenon as the 'previous' marker changing to the attribute that is textually after the main marker. The order within the same message seems less important to me.

For following happens when attributes are merged:

  1. If the attributes are of the same declaration, compatibility is checked with the existing attributes. With the current order: textually later attributes are already in the list of accepted attributes and textually earlier attributes are added to it.
  2. If the attributes are in two different declarations, the new (textually later) clang::Decl with its attributes is taken and the attributes of the old (textually earlier) clang::Decl are added to it while checking for compatibility.

That is, in both cases the textually earlier attribute is added to the existing list of textually later attributes. The diagnostics are printed with the exiting attribute on the right and the attribute-to-be added on the left (at least in this case. test/Sema/attr-swiftcall.c:12 is a counterexample where this patch changes it to the textual order).

Conflicts are resolved by throwing away the conflicting attribute in the list (i.e. textually later) to make room for the new (textually earlier) attribute.

This does not seem to have evolved intentionally when picking in which order to print to two conflicting attributes. I would prefer if the the AST represents the source as accurately as possible, including ordering of attributes (e.g. test/Sema/attr-print.c), which in some cases carry semantic meaning: EnableIfAttr and LoopHintAttr. Today, when dumping the AST, these are just incorrect.

I spent some time trying to reverse the order in which the attributes appear in the diagnostic messages (including previous occurance here), but this would also require changing case 2., which is more difficult.

This patch appears to have at least caused us to process attributes too many times in some cases. For example:

__attribute__((noreturn,noinline,unused)) void f();

before your patch gives this with clang -Xclang -ast-dump:

`-FunctionDecl 0xccef1e0 <<stdin>:1:1, col:50> col:48 f 'void () __attribute__((noreturn))'
  |-UnusedAttr 0xccef280 <col:34> unused
  `-NoInlineAttr 0xccef2c8 <col:25>

and after your patch gives this:

`-FunctionDecl 0xb913740 <<stdin>:1:1, col:50> col:48 f 'void () __attribute__((noreturn))'
  |-NoInlineAttr 0xb9137e0 <col:25>
  |-UnusedAttr 0xb913828 <col:34> unused
  |-NoInlineAttr 0xb913838 <col:25>
  `-UnusedAttr 0xb913848 <col:34> unused

Yep, this is a symptom of the bug I was discussing earlier. This isn't a problem with this patch per-se, but it exposes it by having certain attributes (of which calling conventions, abis, and noreturn are some) cause some linked-list rewriting. I'm currently attempting to remove the AttributeList's linked-listness.

I'm currently attempting to remove the AttributeList's linked-listness.

Thank you. This should also resolve the non-optimal asymptotic execution time to append new attributes at the end of the list.

Meinersbur reopened this revision.Aug 1 2018, 12:57 PM

Reopen after revert (and to be able to update the diff)

This revision is now accepted and ready to land.Aug 1 2018, 12:57 PM

Rebase after de-listifying in r336945

I am unsure how to proceed. Commit since already accepted? Wait for reconfirmation? Open new differential?

erichkeane added inline comments.Aug 1 2018, 1:13 PM
test/Sema/attr-ownership.c
22 ↗(On Diff #158610)

This seems wrong to me, the 2nd one should be the error condition, right?

test/Sema/attr-print.c
25 ↗(On Diff #158610)

This TODO doesn't apply, right? Or is at least wrong...

test/Sema/attr-visibility.c
18 ↗(On Diff #158610)

This order issue is likely to appear a couple of times I suspect.

test/SemaOpenCL/address-spaces.cl
64 ↗(On Diff #158610)

These changes have me concerned... The error message isn't specific, but we have to change the order anyway?

Meinersbur marked an inline comment as done.Aug 1 2018, 2:24 PM
Meinersbur added inline comments.
test/Sema/attr-ownership.c
22 ↗(On Diff #158610)

This is the result of how attributes are combined. There are the two possibilities

void f15(int, int) __attribute__((ownership_returns(foo, 1)));
void f15(int, int) __attribute__((ownership_returns(foo, 2)));

and

void f15(int, int) __attribute__((ownership_returns(foo, 1)))
                   __attribute__((ownership_returns(foo, 2)));

The error diagnosis seem to have been written with the first case in mind and emits in the order there. In the second case attributes merged in the other order (which is the naively correct order, see https://reviews.llvm.org/D48100#1142865), resulting diagnosis to be emitted in the reverse-textual order. There is no consistency in which SourceLocation is the first and which one is the second, and many diagnoses are already not printed in textual order.

I cannot change this without massively reworking how attributes are processed in clang.

test/Sema/attr-print.c
25 ↗(On Diff #158610)

Correct, I missed this todo.

test/Sema/attr-visibility.c
18 ↗(On Diff #158610)

See my previous reply and https://reviews.llvm.org/D48100#1142865

test/SemaOpenCL/address-spaces.cl
64 ↗(On Diff #158610)

An additional error is emitted with the reverse order (non-kernel function variable cannot be declared in local address space; a result of which attribute 'wins' in error resolution which is again related to which attribute is already in the AttributeList). I'd say it is the responsibility diagnosis code to emit the same set of messages independent of attribute order which I do not try to fix here.

Meinersbur updated this revision to Diff 158639.Aug 1 2018, 2:27 PM
Meinersbur marked an inline comment as done.
  • Remove TODOs about the attribute order
hfinkel accepted this revision.Aug 1 2018, 5:59 PM

Assuming that @aaron.ballman is still okay with this, I think that we should move forward. @erichkeane, I suspect that fixing the ordering problems, which were often arbitrary before and are still so, is a larger change that we'd want to split out regardless (not to mention the fact that we'd need to decide how to fix them). Erich, is that alright with you?

Assuming that @aaron.ballman is still okay with this, I think that we should move forward. @erichkeane, I suspect that fixing the ordering problems, which were often arbitrary before and are still so, is a larger change that we'd want to split out regardless (not to mention the fact that we'd need to decide how to fix them). Erich, is that alright with you?

That works fine with me. I'm still concerned about the ordering issues, since I think the error-on-the-second made more sense in the past, but after digging in for a bit, I think it is perhaps not worth the effort. I'm OK if @aaron.ballman is.

aaron.ballman accepted this revision.Aug 2 2018, 1:29 PM

I am still okay with this, and agree that the ordering issues are a separate thing to tackle.

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.

I have two approaches to tackle the wrong marker order: D50215 and D50216. IMHO both are too invasive to be justified for the small issue.

I have two approaches to tackle the wrong marker order: D50215 and D50216. IMHO both are too invasive to be justified for the small issue.

I think you're right here. I despise D50215, and only mostly hate D50216. I think I'd prefer leaving this as a "FIXME" somewhere.

ABataev removed a reviewer: ABataev.Aug 3 2018, 7:03 AM

As a possible idea (that may or may not work): the Attr object itself has a SourceRange on it; perhaps a solution is to keep the > attributes in sorted order within DeclBase::addAttr() based on the SourceRanges passed in?

Interestingly, I think I came up with that idea in a comment on D50214. I think that we should either keep the attributes sorted, or make the iterator give a sorted version.