Page MenuHomePhabricator

[LLVM-C] Fix ByVal Attribute crashing
ClosedPublic

Authored by Wallbraker on Aug 13 2019, 8:24 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

Wallbraker created this revision.Aug 13 2019, 8:24 AM

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.

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?

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.

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.

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 added a comment.EditedAug 27 2019, 8:05 AM

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.

Wallbraker edited the summary of this revision. (Show Details)Aug 27 2019, 8:09 AM
hans added a comment.Aug 27 2019, 9:27 AM

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.

@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.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 28 2019, 2:20 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 28 2019, 2:20 AM
hans added a comment.Aug 28 2019, 2:24 AM

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.

Thanks for the review and merging this!