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

[Lang Hames via Phabricator via llvm-commits]

lhames added a comment.

I see a couple of warnings when building this code:

  /path/to/llvm-project/llvm/examples/OrcV2Examples/OrcV2CBindingsVeryLazy/OrcV2CBindingsVeryLazy.c:127:14: warning: variable 'TSM' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
    } else if (strcmp(CSym, "bar_body") == 0) {
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  /path/to/llvm-project/llvm/examples/OrcV2Examples/OrcV2CBindingsVeryLazy/OrcV2CBindingsVeryLazy.c:133:10: note: uninitialized use occurs here
    assert(TSM);
           ^~~
  /Library/Developer/CommandLineTools/SDKs/MacOSX11.5.sdk/usr/include/assert.h:93:25: note: expanded from macro 'assert'
      (__builtin_expect(!(e), 0) ? __assert_rtn(__func__, __FILE__, __LINE__, #e) : (void)0)
                          ^
  /path/to/llvm-project/llvm/examples/OrcV2Examples/OrcV2CBindingsVeryLazy/OrcV2CBindingsVeryLazy.c:127:10: note: remove the 'if' if its condition is always true
    } else if (strcmp(CSym, "bar_body") == 0) {
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  /path/to/llvm-project/llvm/examples/OrcV2Examples/OrcV2CBindingsVeryLazy/OrcV2CBindingsVeryLazy.c:120:33: note: initialize the variable 'TSM' to silence this warning
    LLVMOrcThreadSafeModuleRef TSM;
                                  ^
                                   = NULL

and it asserts on execution on Darwin with:

  ./bin/OrcV2CBindingsVeryLazy 
  Assertion failed: ((!RequiresNullTerminator || BufEnd[0] == 0) && "Buffer is not null terminated!"), function init, file /path/to/llvm-project/llvm/lib/Support/MemoryBuffer.cpp, line 49.

I'll see if I can figure out what's going on there tomorrow.

Regarding the `MaterializationResponsibility` API:  it would be good to add the `addDependencies` and `addDependenciesForAll` methods. These declare dependencies for each materializing symbol so that ORC can track them in its dependence graph, which in turn ensures that lookups remain safe regardless of what threads the symbols are being materialized on. These methods are an essential part of the `MaterializationResponsibility` API.



================
Comment at: llvm/include/llvm-c/Orc.h:702-718
+/**
+ * Define the given symbols as non-existent, removing it from the symbol
+ * table and notifying any pending queries. Queries that lookup up the
+ * symbol using the SymbolLookupFlags::WeaklyReferencedSymbol flag will
+ * behave as if the symbol had not been matched in the first place. Queries
+ * that required this symbol will fail with a missing symbol definition
+ * error.
----------------
This can be removed for now, since the operation isn't supported in the C++ API yet.


================
Comment at: llvm/lib/ExecutionEngine/Orc/OrcV2CBindings.cpp:473-482
+// TODO(@lhames) MaterializationResponsibility::defineNonExistent is nonExistent
+// void LLVMOrcMaterializationResponsibilityDefineNonExistent(
+//     LLVMOrcMaterializationResponsibilityRef MR,
+//     LLVMOrcSymbolStringPoolEntryRef *Symbols, size_t NumSymbols) {
+//   SmallVector<SymbolStringPtr, 4> Syms;
+//   for (size_t I = 0; I != NumSymbols; I++) {
+//     Syms.push_back(OrcV2CAPIHelper::moveToSymbolStringPtr(unwrap(Symbols[I])));
----------------
I'll remove this function on the mainline. This code can be dropped from this review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104799