kaleidoscope ch4 jit example regression?

https://llvm.org/docs/tutorial/LangImpl04.html has an example where
the function `foo` gets redefined, and the JIT returns evaluation of
the latest definition. I thought my code was wrong, but it seems that
the binary produced by `ninja Kaleidoscope-Ch4` has the same bug.

Granted, my LLVM checkout is from Nov 3 2018 (r346062). Is this a known issue?
~Nick Desaulniers

+Lang who does JIT things

Yeah, I can confirm my local build (using LLVM source from today) of Chapter 4 behaves as you describe, and not as the documentation shows. Looks like somethnig needs updating (either source or the documentation).

Hi Nick,

I was not aware of it, but it makes sense given the recent switch to ORC2, which has different symbol resolution rules.

I am out on vacation this week, but will take a look when I get back and see if I can restore the old behavior.

Cheers,
Lang.

Ping - did this end up getting addressed?

Not yet unfortunately. I’ve had my head down working on a jut-linker replacement.

Let me take a look right now…

Ok. For the curious: The problem is that ORCv2 did away with symbol resolvers in favor of fixed symbol tables with resolution rules intended to match the static and dynamic linkers. That gave us a structure with which we could coordinate concurrent compilation, but took away our ability to arbitrarily hide old definitions (which is what we were relying on in the tutorial).

In the short term I think we can hack around this by detecting symbol clashes (DuplicateDefinition errors) in the KaleidoscopeJIT class and starting a new JITDylib each time we see one: Duplicate symbols within a dylib are not allowed, but duplicate symbols in different dylibs are. Then we just search our dylibs from last to first when resolving.

In the long term we would like a way to rename or delete symbol table entries (without deleting their underlying memory). This is probably still a little way off: deleting / renaming in the context of concurrent compilation is tricky.

Cheers,
Lang.

Just saw this (while looking for another patch).
https://github.com/llvm/llvm-project/commit/ddf91af5a67bef9ca7a6f14277420e8d2dc0c66e

Thanks! (Can we add a test so this part of kaleidoscope doesn’t regress again?)

Hi Nick,

It looks like we already have one: llvm/test/Examples/Kaleidoscope/Chapter4.test

I wonder why we did not see it fail? I guess none of the builders are building with examples turned on. I will look in to that.

— Lang.