This is an archive of the discontinued LLVM Phabricator instance.

[LLVM-C] Audit Inline Assembly APIs for Consistency
ClosedPublic

Authored by CodaFi on Apr 5 2018, 6:48 PM.

Details

Summary
  • Add a missing getter for module-level inline assembly
  • Add a missing append function for module-level inline assembly
  • Deprecate LLVMSetModuleInlineAsm and replace it with LLVMSetModuleInlineAsm2 which takes an explicit length parameter
  • Deprecate LLVMConstInlineAsm and replace it with LLVMGetInlineAsm, a function that allows passing a dialect and is not mis-classified as a constant operation

Diff Detail

Repository
rL LLVM

Event Timeline

CodaFi created this revision.Apr 5 2018, 6:48 PM
CodaFi updated this revision to Diff 141257.Apr 5 2018, 6:51 PM
CodaFi added a comment.Apr 5 2018, 7:00 PM

The original API for LLVMSetModuleInlineAsm implies that only C-style strings can be passed here, but the implementation is copying to a std::string under the hood anyhow. If that's the invariant that we want to maintain here, I will revert the deprecation and add documentation.

whitequark accepted this revision.Apr 5 2018, 7:12 PM

Excellent work, thanks!

Regarding LLVMSetModuleInlineAsm2, I have two thoughts:

  1. It is likely that actually passing a literal \0 in inline assembly will confuse the assembler. In fact, I think both internal and external assemblers will refuse to process it.
  2. Silently truncating on \0 is likely to hide a bug elsewhere instead, so an explicit error is good.
  3. Consistency with other APIs is good, and making the job of FFI bindings easier is also good (some of them autogenerate code from signatures).
  4. Code churn is bad, and in this case even the above isn't a particularly strong justification for code churn.

As a result, I think we should merge this as-is, but do not take aggressive steps to remove LLVMSetModuleInlineAsm. It is enough that all new code will use LLVMSetModuleInlineAsm2.

This revision is now accepted and ready to land.Apr 5 2018, 7:12 PM
whitequark requested changes to this revision.Apr 5 2018, 7:14 PM

Oh sorry, I've missed one API issue.

include/llvm-c/Core.h
684 ↗(On Diff #141257)

I think this should be const char *AsmString, size_t AsmStringSize, const char *Constraints, size_t ConstraintsSize.

This revision now requires changes to proceed.Apr 5 2018, 7:14 PM
whitequark added inline comments.Apr 5 2018, 7:15 PM
include/llvm-c/Core.h
684 ↗(On Diff #141257)

Or rather const char *AsmString, size_t AsmStringLen, const char *Constraints, size_t ConstraintsLen.

CodaFi added inline comments.Apr 5 2018, 7:15 PM
include/llvm-c/Core.h
684 ↗(On Diff #141257)

Aha, good catch.

CodaFi updated this revision to Diff 141260.Apr 5 2018, 7:24 PM
CodaFi marked 2 inline comments as done.
whitequark accepted this revision.Apr 5 2018, 7:25 PM
This revision is now accepted and ready to land.Apr 5 2018, 7:25 PM
This revision was automatically updated to reflect the committed changes.