[LLVM-C] Fix ByVal Attribute crashing

[Wallbraker]

Summary

With the introduction of the typed byval attribute change there was no way that the LLVM-C API could create the correct class Attribute. If a program that uses the C API creates a ByVal attribute and annotates a function with that attribute LLVM will crash when it assembles or write that module containing the function out as bitcode.

This change is a minimal fix to at least allow code to work, this is because the byval change is on the 9.0 and I don't want to introduce new LLVM-C API this late in the release cycle.

Cheers, Jakob.

[whitequark]

Can you post a link to the change that's causing the problem here? Your patch looks good but I'd like to understand it a bit better.

[Wallbraker]

It's in: SVN id 362128
https://github.com/llvm/llvm-project/commit/b7141207a483d39b99c2b4da4eb3bb591eca9e1a

I think the proper fix is to LLVMCreateTypeAttribute (and LLVMCreateIntAttribute) not sure if we should/can add that on the 9.0 branch tho. And maybe check the that the type is right for the name?

[whitequark]

I think the proper fix is to LLVMCreateTypeAttribute (and LLVMCreateIntAttribute)

I agree.

not sure if we should/can add that on the 9.0 branch tho.

This seems fairly un-invasive, so why not?

And maybe check the that the type is right for the name?

I think it is.

Regarding your hot-fix, it seems fine to me as well, perhaps with a comment that explains why it's there.

[Wallbraker]

Ugh, I completely dropped the ball on this change.

Hmm thinking about it a bit the LLVM code calling getType should probably check that the Attribute is of the right kind.

[hans]

I don't fully understand the severity here and what the status is. Is it needed for the 9 release?

+Tim who wrote r362128.

[Wallbraker]

In D66144#1647091, @hans wrote:

I don't fully understand the severity here and what the status is. Is it needed for the 9 release?

+Tim who wrote r362128.

It's a critical regression. If a program that uses the C API creates a ByVal attribute and annotates a function with that attribute LLVM will crash as it tries to write that bitcude out or assemble the module. Which means to avoid that crash this fix has to go in or the program has to produce broken binaries.

And since r362128 is in the LLVM 9 release this fix has to be ported there.

[hans]

Okay, thanks for the update. And is the current patch the final state? It looks pretty straight-forward but I'm not familiar with this change.

[Wallbraker]

@whitequark Suggested we add a API to let you create typed attributes, but was also happy with the change as is.

Personally I feel the change is fine for 9 as it's a very short time to add new API until the release goes out. But new API should be added for master/10, but it requires more thought as I think there are some subtleties with Attribute.

[hans]

Thanks! I'll go ahead and commit this just to get things unblocked as soon as possible. r370176.

And merging that to the release_90 branch in r370178.

[Wallbraker]

Thanks for the review and merging this!

llvm/trunk/lib/IR/Core.cpp

 }
 
 unsigned LLVMGetLastEnumAttributeKind(void) {
   return Attribute::AttrKind::EndAttrKinds;
 }
 
 LLVMAttributeRef LLVMCreateEnumAttribute(LLVMContextRef C, unsigned KindID,
                                          uint64_t Val) {
-  return wrap(Attribute::get(*unwrap(C), (Attribute::AttrKind)KindID, Val));
+  auto &Ctx = *unwrap(C);
+  auto AttrKind = (Attribute::AttrKind)KindID;
+
+  if (AttrKind == Attribute::AttrKind::ByVal) {
+    // After r362128, byval attributes need to have a type attribute. Provide a
+    // NULL one until a proper API is added for this.
+    return wrap(Attribute::getWithByValType(Ctx, NULL));
+  } else {
+    return wrap(Attribute::get(Ctx, AttrKind, Val));
+  }
 }
 
 unsigned LLVMGetEnumAttributeKind(LLVMAttributeRef A) {
   return unwrap(A).getKindAsEnum();
 }
 
 uint64_t LLVMGetEnumAttributeValue(LLVMAttributeRef A) {
   auto Attr = unwrap(A);