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

[Lang Hames via Phabricator via llvm-commits]

lhames added inline comments.


================
Comment at: llvm/unittests/ExecutionEngine/Orc/OrcCAPITest.cpp:432
+
+  // This hangs
+  // LLVMOrcRetainSymbolStringPoolEntry(TargetSym.Name);
----------------
vchuravy wrote:
> @lhames any ideas what I am doing wrong here?
`-debug-only=orc` tells the story:

```
In main defining materializing symbols { ("_other", [Callable]) }
In main replacing symbols with MU at 0x10d721db0 ("<Absolute Symbols>", { ("_other", [Callable]) })
Adding dependencies for _foo: { (main, { _other }) }
In main adding dependencies for _foo: { (main, { _other }) }
Adding dependencies for all symbols in { ("_foo", [Callable]) }: { (main, { _other }) }
In main adding dependencies for _foo: { (main, { _other }) }
In main resolving { ("_foo": 0x00000001000eb8a0 [Callable]) }
In main emitting { ("_foo", [Callable]) }
Done dispatching MaterializationUnits.
```

You've hit a not-yet-considered corner case: Adding a dependency on a symbol (`other`) for which there is no corresponding lookup. Materialization is currently only triggered by lookup, not by adding dependencies, so the lookup for `foo` is left stuck (due to the dependence on `other`) with no way to trigger materialization of `other`.

A real test of the dependence tracking in the success case would require async lookups. You could:
1. Materialize `foo`, making foo depend on `other`.
2. In the caller, verify that the lookup callback for `foo` has not run (due to the dependence)
3. Materialize `other` by looking it up.
4. In the caller, verify that the lookup callback for `foo` has now run.

I don't think you need to implement the APIs required to test this though. For this test I would just go ahead and resolve/emit `other` too, then this should work.

Could you include a FIXME ('write async lookup API, then... ', and the steps listed above) in the test case so that we can go back and do it The Right Way in the future?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104799