[PATCH] D104799: [Orc] Add verylazy example for C-bindings

[Lang Hames via Phabricator via llvm-commits]

lhames added a comment.

I found a few more issues while getting the example working on Darwin:

1. Rather than exposing applyDataLayout, it would be better to add an `LLVMOrcLLJITGetDataLayoutString` function, then use the existing `LLVMSetDataLayout` function.

2. You should uses strlen rather than sizeof when building the MemoryBuffers for the source -- MemoryBuffer's null termination guarantee is that the character //one past the end of the buffer// is null, so 'sizeof' causes assertions if the memory following the null terminator happens to be non-zero.

3. You should compares mangled+interned symbol names, rather than doing string comparisons on symbol names. That makes the example work regardless of the platform mangling scheme. (Side note: in a real world example we'd want to intern the comparison symbols once and re-use them, but the extra memory management felt like it would just complicate things here so I've opted to re-intern the strings on each call.)

4. You'll need to add a LLVMOrcDisposeMaterializationResponsibility function: MaterializationResponsibility objects are uniquely owned -- If you hand it off to a base layer you're off the hook, but if you fail the materialization you need a way to dispose of it. This should be called on MR after you fail the materialization in the example.

I've also added some inline notes about ownership in the comments.



================
Comment at: llvm/include/llvm-c/Orc.h:613-620
+/** Returns the symbol flags map for this responsibility instance.
+ * Note: The returned flags may have transient flags (Lazy, Materializing)
+ * set. These should be stripped with JITSymbolFlags::stripTransientFlags
+ * before using.
+ *
+ * The lenght of the array is returned in NumPairs and the caller is responsible
+ * for the returned memory and needs to call LLVMOrcDisposeCSymbolFlagsMap.
----------------
typo in 'lenght'.

The comment should include a note that the pool entries are not retained by this operation: The entries need not be released, but must be retained if the caller wants them to outlive any mutating operations on the MR object.


================
Comment at: llvm/include/llvm-c/Orc.h:624-626
+/**
+ * Disposes of the passed LLVMOrcCSymbolFlagsMap.
+ */
----------------
This should include a note that it does not release the entries.


================
Comment at: llvm/include/llvm-c/Orc.h:639-642
+ * Returns the names of any symbols covered by this
+ * MaterializationResponsibility object that have queries pending. This
+ * information can be used to return responsibility for unrequested symbols
+ * back to the JITDylib via the delegate method.
----------------
Should include a note that the returned entry is not retained -- the caller should retain it if they want it to outlive any mutating operations on the MR.


================
Comment at: llvm/include/llvm-c/Orc.h:649
+/**
+ * Disposes of the passed LLVMOrcSymbolStringPoolEntryRef* .
+ */
----------------
This should include a not that it only frees the list, it does not release the entries.


================
Comment at: llvm/include/llvm-c/Orc.h:740-742
+ * Delegates responsibility for the given symbols to the returned
+ * materialization responsibility. Useful for breaking up work between
+ * threads, or different kinds of materialization processes.
----------------
This should include a note that the function returns ownership of the instance, and the client is responsible for freeing it (after resolving/emitting, or failing).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104799/new/

https://reviews.llvm.org/D104799