Page MenuHomePhabricator

[Attributes] Fix crash when attempting to remove alignment from an attribute list/set

Authored by dneilson on Jan 11 2018, 9:25 AM.



Discovered while working on a patch to move alignment in
@llvm.memcpy/move/set from an arg into parameter attributes.

The current implementations of AttributeSet::removeAttribute() and
AttributeList::removeAttribute crash when attempting to remove the
alignment attribute. Currently, these implementations add the
to-be-removed attributes to an AttrBuilder and then remove
the builder from the list/set. Alignment is special in that it
must be added to a builder with an integer value for the alignment;
attempts to add alignment to a builder without a value is an error.

This change fixes the removeAttribute implementations for AttributeSet and
AttributeList to make them able to remove the alignment, and other similar,

Diff Detail


Event Timeline

dneilson created this revision.Jan 11 2018, 9:25 AM
reames requested changes to this revision.Jan 12 2018, 8:56 AM

A few minor changes needed, but generally looks close to ready.

564 ↗(On Diff #129460)

Incomplete handling. I think the assert is simply stale and missing a test case.

1101 ↗(On Diff #129460)

I believe this check is implied by the hasAttribute check above it.

81 ↗(On Diff #129460)

Add a test for remove AttrBuilder case.

Also add a test case for a non-param alignment or at least one of them. Say, stackalign for the AttrBuilder case.

This revision now requires changes to proceed.Jan 12 2018, 8:56 AM
rnk added inline comments.Jan 12 2018, 9:17 AM
1106 ↗(On Diff #129460)

I think we can just assert(Index < AttrSets.size()) because otherwise hasAttribute would return false.

1109–1111 ↗(On Diff #129460)

Can you simplify this to AttrSets[Index] = AttrSets[Index].removeAttribute(Kind);? It seems you fixed up AttributeSet::removeAttribute to handle this case.

1124 ↗(On Diff #129460)

ditto, can strengthen to assert.

1129 ↗(On Diff #129460)

ditto, can use AttributeSet::removeAttribute.

dneilson updated this revision to Diff 129671.Jan 12 2018, 11:16 AM

Address comments by Philip & Reid.

rnk accepted this revision.Jan 17 2018, 10:25 AM

lgtm, thanks!

This revision was not accepted when it landed; it landed in state Needs Review.Jan 17 2018, 11:17 AM
This revision was automatically updated to reflect the committed changes.