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

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

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



================
Comment at: lib/IR/Attributes.cpp:564
   // For now, say we can't pass in alignment, which no current use does.
   assert(!Attrs.hasAlignmentAttr() && "Attempt to change alignment!");
 
----------------
Incomplete handling.  I think the assert is simply stale and missing a test case.


================
Comment at: lib/IR/Attributes.cpp:1101
   if (!hasAttribute(Index, Kind)) return *this;
-  AttrBuilder B;
-  B.addAttribute(Kind);
-  return removeAttributes(C, Index, B);
+  if (!pImpl)
+    return AttributeList();
----------------
I believe this check is implied by the hasAttribute check above it.


================
Comment at: unittests/IR/AttributesTest.cpp:81
+  EXPECT_FALSE(AL.hasParamAttr(0, Attribute::Alignment));
+}
+
----------------
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.


https://reviews.llvm.org/D41951