Wrap LLVMAttributeRef API #126

kyouko-taiga · 2018-03-10T01:36:51Z

Disclaimer: I'm quite a newbie with LLVM

This is my attempt at wrapping the LLVM 4.0.0+ LLVMAttributeRef API (as mentioned in #10).

I extracted the list of parameters from LLVM's docs. I didn't make any unit tests so far, and haven't tested all parameters.

Due to my limited knowledge of LLVM, I'm unsure of the expected result of calling LLVMAddAttributeAtIndex on parameters whose arguments aren't a single integer value (e.g. allocsize), nor of the difference between attributes specified with or without quote in the documentation (e.g. "patchable-function"). Some review from a more experienced developer would be greatly appreciated.

I tried to follow the coding convention I could guess from the existing code to the best of my ability, but don't hesitate to comment on that as well.

CodaFi · 2018-03-10T02:47:59Z

Sources/LLVM/Function+Attributes.swift

+  public func addAttribute(_ attr: FunctionAttribute, value: UInt64 = 0) {
+    let ctx = LLVMGetModuleContext(LLVMGetGlobalParent(llvm))
+    let attrRef = LLVMCreateEnumAttribute(ctx, attr.kindID, value)
+    LLVMAddAttributeAtIndex(llvm, 0, attrRef)


This is not correct. This is the index for the return value of the function.

CodaFi · 2018-03-10T02:48:26Z

Sources/LLVM/Function+Attributes.swift

+  /// - parameterIndex: The index of the parameter to which add the
+  ///   attribute, starting from 0.
+  public func addParameterAttribute(_ attr: ParameterAttribute, value: UInt64 = 0,
+                                    for parameterIndex: LLVMAttributeIndex) {


We have a policy of never exposing LLVM's types to the user. I have an idea of how we can lift this into an enum in one case.

Please use Int here.

CodaFi · 2018-03-10T02:49:34Z

I think we can kill two birds with one stone here by lifting AttributeList's sorta ad-hoc indexing model to an enum:

enum AttributeIndex: UInt32 {
  case returnValue // 0
  case function // ~0
}

The RawRepresentable conformance will have to be done by hand.

CodaFi · 2018-03-10T02:51:14Z

Sources/LLVM/Function+Attributes.swift

+#endif
+
+/// Enumerates the attributes of LLVM functions.
+public enum FunctionAttribute: String {


Thank you for documenting this patch.

CodaFi · 2018-03-10T02:56:01Z

Do you know if lli or the AttributeSet does bounds checking for the parameter indices? If not we should put in a precondition.

CodaFi · 2018-03-10T03:02:15Z

Sources/LLVM/Function+Attributes.swift

+  /// This attribute indicates that the function may only access memory that is
+  /// either not accessible by the module being compiled, or is pointed to by
+  /// its pointer arguments.
+  case inaccessiblemem_or_argmemonly


All the snake_case should be camelCase.

CodaFi · 2018-03-10T03:04:53Z

Oh, it'd also be great if you could add a test. It can be as simple as a loop that adds/removes All The Attributes to a dummy function and its parameters and checks that they got registered/remoed.

kyouko-taiga · 2018-03-11T00:54:53Z

Thanks for the feedbacks!

A few comments on the changes I made:

According to LLVM docs, I understand there are several kinds of attributes in LLVM's new scheme. If I understand the docs correctly, this is also where the distinction between attributes specified with or without quotes comes in. The formers describe target-independent attributes, while the latters describe dependent ones. Therefore I took the liberty to create two types to distinguish between those two kind of attributes. I wrapped all LLVM*StringAttribute* functions, which I believe were part of LLVMSwift missing functionalities, as mentioned in #51 .

I implemented a method to retrieve the attributes from a function. I made the choice to return an array rather than a set, so as to avoid having to conform to Hashable (which seemed a bit overkill to me). Unfortunately, it seems like there's no wrapper around C++'s Attribute::getKindAsString() so I can't provide a proper implementation of EnumAttribute.name.

I wrote some tests for all these shenanigans. Those revealed that there seems to be a problem with the removal of valued enum attributes. I tried to understand what's causing this bug within LLVM's source, but I have to admit to find the whole thing quite puzzling. Indeed, removing an integer attribute seems to always fail, because AttributeList::removeAttribute() attempts to add the attribute about to be removed without preserving its value:

AttributeSet AttributeSet::removeAttribute(LLVMContext &C,
                                             Attribute::AttrKind Kind) const {
  if (!hasAttribute(Kind)) return *this;
  AttrBuilder B;
  B.addAttribute(Kind);
  return removeAttributes(C, B);
}

This in turn fails in AttrBuilder::addAttribute() violating the second assertion:

AttrBuilder &AttrBuilder::addAttribute(Attribute::AttrKind Val) {
  assert((unsigned)Val < Attribute::EndAttrKinds && "Attribute out of range!");
  assert(Val != Attribute::Alignment && Val != Attribute::StackAlignment &&
         Val != Attribute::Dereferenceable && Val != Attribute::AllocSize &&
         "Adding integer attribute without adding a value!");
  Attrs[Val] = true;
  return *this;
}

Maybe an issue with my version of LLVM ...

kyouko-taiga · 2018-03-11T01:10:57Z

I confirm the bug about valued enum attributes removal is caused by LLVM: [Attributes] Fix crash when attempting to remove alignment from an attribute list/set.

CodaFi · 2018-03-11T22:19:31Z

Sources/LLVM/Function+Attributes.swift

+
+  /// The name of the attribute's kind.
+  public var name: String {
+    return ""


If we can't provide this then we should omit it.

CodaFi · 2018-03-11T22:19:50Z

Sources/LLVM/Function+Attributes.swift

+}
+
+/// An LLVM attribute.
+public protocol Attribute {


Can you document this and its members a little more

CodaFi · 2018-03-11T22:26:08Z

Sources/LLVM/Function+Attributes.swift

+      let cstring = LLVMGetStringAttributeKind(stringAttr.llvm, &length)
+      LLVMRemoveStringAttributeAtIndex(llvm, index.rawValue, cstring, length)
+    default:
+      fatalError()


Please add an explanation string to this fatal error

CodaFi · 2018-03-11T22:27:55Z

Down to nits. This is looking quite good.

kyouko-taiga · 2018-03-11T22:47:06Z

What would be the best thing to do about the issue with removeAttribute(:from:) on valued enum attributes? The issue seems to be fixed in LLVM 7.0.0svn but if I remove the guard in IRAttributesSpec.swift:138 the tests won't pass with previous versions. I don't know what's LLVMSwift's stance about the version of LLVM it is linked to.

CodaFi · 2018-03-11T22:48:17Z

If we can't safely provide this API then we shouldn't provide it altogether. We can hold out until 7.0 and leave behind a note. I'd much rather that than crashing deep in LLVM.

CodaFi · 2018-03-11T22:51:57Z

What I would recommend is we bake the guard into the API and fatal error with an explanation as to why we cannot safely remove the attribute yet.

kyouko-taiga · 2018-03-11T22:52:51Z

Is there any way we can know what version of LLVM we're linked to?

CodaFi · 2018-03-11T22:54:14Z

LLVM_VERSION_MAJOR/MINOR/PATCH but we tie ourselves to the latest version as it is announced and do not do backwards compatibility.

kyouko-taiga · 2018-03-11T22:57:04Z

Alright, I'd make the guard only for the attributes that may fail, namely align, alignstack, allocsize, dereferenceable and dereferenceable_or_null.

The current version of LLVM crashes when attempting to remove valued enum attributes. The bug was discovered and fixed in LLVM 7 (see llvm-mirror/llvm@9bc0b10). The guard placed in `removeAttribute` should be removed once this version is released.

CodaFi · 2018-03-11T23:24:24Z

Excellent. Thank you kindly.

⛵️

Wrap LLVMAttributeRef API

5a4d51c

CodaFi reviewed Mar 10, 2018

View reviewed changes

kyouko-taiga added 5 commits March 10, 2018 14:13

Lift LLVMAttributeIndex with a raw representable enum

6ffe444

Rewrite snake_case to camelCase

0b8e9bd

Test addAttribute

f7116ab

Distinguish between enum and string attributes

b35f5bc

Test set/unset attributes

880fbbe

CodaFi reviewed Mar 11, 2018

View reviewed changes

CodaFi requested changes Mar 11, 2018

View reviewed changes

kyouko-taiga added 3 commits March 11, 2018 23:30

Remove name property requirements from Attribute

f0485c6

Provide an explanation for failure to cast attribute

0741b13

Add some documentation

7109545

CodaFi approved these changes Mar 11, 2018

View reviewed changes

CodaFi merged commit a9581c7 into llvm-swift:master Mar 11, 2018

kyouko-taiga deleted the llvmattributeref-api branch March 11, 2018 23:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wrap LLVMAttributeRef API #126

Wrap LLVMAttributeRef API #126

kyouko-taiga commented Mar 10, 2018

CodaFi Mar 10, 2018

CodaFi Mar 10, 2018 •

edited

Loading

CodaFi Mar 10, 2018

CodaFi commented Mar 10, 2018 •

edited

Loading

CodaFi Mar 10, 2018

CodaFi commented Mar 10, 2018 •

edited

Loading

CodaFi Mar 10, 2018

CodaFi commented Mar 10, 2018 •

edited

Loading

kyouko-taiga commented Mar 11, 2018 •

edited

Loading

kyouko-taiga commented Mar 11, 2018 •

edited

Loading

CodaFi Mar 11, 2018

CodaFi Mar 11, 2018

CodaFi Mar 11, 2018

CodaFi commented Mar 11, 2018

kyouko-taiga commented Mar 11, 2018

CodaFi commented Mar 11, 2018

CodaFi commented Mar 11, 2018

kyouko-taiga commented Mar 11, 2018

CodaFi commented Mar 11, 2018

kyouko-taiga commented Mar 11, 2018 •

edited

Loading

CodaFi commented Mar 11, 2018

Wrap LLVMAttributeRef API #126

Wrap LLVMAttributeRef API #126

Conversation

kyouko-taiga commented Mar 10, 2018

CodaFi Mar 10, 2018

Choose a reason for hiding this comment

CodaFi Mar 10, 2018 • edited Loading

Choose a reason for hiding this comment

CodaFi Mar 10, 2018

Choose a reason for hiding this comment

CodaFi commented Mar 10, 2018 • edited Loading

CodaFi Mar 10, 2018

Choose a reason for hiding this comment

CodaFi commented Mar 10, 2018 • edited Loading

CodaFi Mar 10, 2018

Choose a reason for hiding this comment

CodaFi commented Mar 10, 2018 • edited Loading

kyouko-taiga commented Mar 11, 2018 • edited Loading

kyouko-taiga commented Mar 11, 2018 • edited Loading

CodaFi Mar 11, 2018

Choose a reason for hiding this comment

CodaFi Mar 11, 2018

Choose a reason for hiding this comment

CodaFi Mar 11, 2018

Choose a reason for hiding this comment

CodaFi commented Mar 11, 2018

kyouko-taiga commented Mar 11, 2018

CodaFi commented Mar 11, 2018

CodaFi commented Mar 11, 2018

kyouko-taiga commented Mar 11, 2018

CodaFi commented Mar 11, 2018

kyouko-taiga commented Mar 11, 2018 • edited Loading

CodaFi commented Mar 11, 2018

CodaFi Mar 10, 2018 •

edited

Loading

CodaFi commented Mar 10, 2018 •

edited

Loading

CodaFi commented Mar 10, 2018 •

edited

Loading

CodaFi commented Mar 10, 2018 •

edited

Loading

kyouko-taiga commented Mar 11, 2018 •

edited

Loading

kyouko-taiga commented Mar 11, 2018 •

edited

Loading

kyouko-taiga commented Mar 11, 2018 •

edited

Loading