[Lldb-commits] [PATCH] D78972: Treat hasWeakODRLinkage symbols as external in REPL computations

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D78972#2010033 <https://reviews.llvm.org/D78972#2010033>, @labath wrote:

> In D78972#2008129 <https://reviews.llvm.org/D78972#2008129>, @jingham wrote:
>
> > In D78972#2007493 <https://reviews.llvm.org/D78972#2007493>, @labath wrote:
> >
> > > > I don't have a way to write a C-based test for this as I don't know how to craft a user expression in C that will make such a thing. I asked around a bit and nobody had an easy way to do this.
> > >
> > > Grepping clang's tests for `weak_odr` was very educational. The most portable way to produce this linkage seems to be:
> > >
> > >   template<typename T> void f() {}
> > >   template void f<int>();
> > >
> > >
> > > As for whether this patch is correct -- I don't know. The weak_odr functions are allowed to be replaced (that's the `weak` part), but the replacement is supposed to be equivalent (the `odr` part). So, strictly speaking, it may be ok to just export this function, but if we wanted to be closer to what happens for realz, we should check first if there isn't a more definitive version available elsewhere...
> >
> >
> > I can't get the expression parser to successfully run this code and then call the function in a later evaluation.  Apparently we're not all the way up to creating & instantiating templates.  I tried it with and without this patch and it didn't make any difference.
>
>
> It seems to work for me:
>
>   $ bin/lldb bin/lldb
>   (lldb) target create "bin/lldb"
>   Current executable set to 'bin/lldb' (x86_64).
>   (lldb) b main
>   Breakpoint 1: where = lldb`main, address = 0x00000000000099d0
>   (lldb) r
>   Process 4504 launched: 'bin/lldb' (x86_64)
>   Process 4504 stopped
>   * thread #1, name = 'lldb', stop reason = breakpoint 1.1
>       frame #0: 0x000055555555d9d0 lldb`main
>   lldb`main:
>   ->  0x55555555d9d0 <+0>: pushq  %r15
>       0x55555555d9d2 <+2>: xorl   %ecx, %ecx
>       0x55555555d9d4 <+4>: pushq  %r14
>       0x55555555d9d6 <+6>: pushq  %r13
>   (lldb) expr --top-level -- template<typename T> void f() { (int)printf("hello world\n"); } template void
>   f<int>();
>   (lldb) expr f<int>()
>   hello world
>
>
>
>
> > In the context of the REPL & top-level, I'm not sure how we would find the "equivalent version".  But if it is supposed to be equivalent, is that important for functions the REPL & top-level are generating?
>
> I don't know about REPL, but top-level expressions can interact with (call) target functions. If a top-level expression defines a weak symbol that is also has a strong definition in the target (or maybe in another top level expression), the maybe one would expect that we call the strong definition.
>
> As to why, I guess the reason is that the only thing which enforces this "supposed" equivalence is the spectre of undefined behavior. The user will (unknowingly) do that anyway, things will go wrong and then they'll try to debug it...


Not sure what I was doing wrong, but I don't use templates a lot...

That one does work all the way, including calling the function.  That should be surprising.   It shouldn't with an unpatched lldb, right?

The way this one actually works is that when you run the expression:

  (lldb) expr --top-level -- template<typename T> void f() { (int)printf("hello world\n"); } template void f<int>();

clang does indeed make a function in the JIT'ted code for the expression:

  _Z1fIiEvv ---> void f<int>()

and that symbol has "hasWeakODRLinkage" set to true.

In unpatched lldb we just throw this symbol away, we don't put it into the symbol_map in the PersistentExpressionState, and so it is not available to future expressions.  So how did the second expression work?

Turns out, since the template definition is around in the AST, when you run the expression:

  (lldb) expr f<int>()

to call it, clang makes another copy of the function, with the same name obviously, and with hasLinkOnceODRLinkage set to true, but hasWeakODRLinkage set to false.  Then the expression that gets JITted explicitly calls the newly created version.  We never find or use a shared copy.

BTW, you also don't need to instantiate the template in the first expression.  This also works:

  (lldb) expr --top-level -- template <typename T> void f() { (void) printf("Hello world\n"); }
  (lldb) expr f<int>()
  Hello world

In that case, the second expression makes a copy of the function f<int>() and calls it directly.

We do this whole dance again every time we run this expression.  It looks like we don't look for instances of a template if we know how to make it, we just make it again.  That actually makes sense.  When you are compiling, you don't know if you will find an instantiation, so you have to make it.  Then you leave it to the linker to remove duplicates.

BTW, if you actually debug a program that has a template instantiation with this signature (even with exactly the same body, though I don't think we could know that), we also don't use that function in this operation.  Again, the compiler makes a copy in the module we're building for the expression, and we use that directly.

Ironically, if you just try to evaluate the function in a program that has the same template code shown above, you get:

  (lldb) expr f<int>()
  error: <user expression 0>:1:1: 'f' does not name a template but is followed by template arguments
  f<int>()
  ^~~~~~
  note: non-template declaration found by name lookup

That's because the DWARF only has "f<int>" and nowhere "f" so we don't know how to access extant template instantiations.  But that's a diversion from the main issue...

So it doesn't look to me like we were ever going to look past the instantiation of the template injected into the expression module, so in this case at least it doesn't matter whether we make the weakODRLinkage version available in future expressions or not.

In the case of swift "default argument value provider functions" they are not regenerated into the module that uses the struct or class, they are expected to be present in the module.  Since it doesn't get remade on demand, we have to export it from the expression.  Why they need to be "WeakODRLinkage" in this case I don't know, but presumably there was a reason for it...

Note that this is all about creating new code in a running program, and it almost seems like you really should use the new version you created if it is any different.  Or anyway, it's not entirely clear what the rule should be.  And anyway, the thing we have to do is not NOT export these symbols, it is really "figure out if they exist somewhere else and are equivalent" and use those, otherwise export these.  Since I don't yet have an example where there are two versions that would get used, I don't want to try to write code to do that blind.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78972