Treat hasWeakODRLinkage symbols as external in REPL computations

[jingham]

Summary

The way both the REPL and the --top-level mode of the expression parser work is that they compile and JIT the code the user provided, and then look through the resultant IR and find any llvm::Functions produced by the compilation. Then it iterates over those functions and if they are marked "external", it adds them to a table of functions that it then makes available to subsequent expressions.

The test we were using for "is external" was hasExternalLinkage || hasLinkOnceODRLinkage. However, there's a third form of external function: hasWeakODRLinkage that can sometimes be produced, which is also intended to be an exported symbol. This patch just includes that predicate to the test we were doing.

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.

OTOH, the swift compiler uses this linkage type when it makes the "default argument provider" for a function with default arguments. So I can easily write a swift REPL test for this. But if somebody knows how to make a C function in the lldb expression parser with a WeakODRLinkage, show me and I'll happily add a test for it here.

[davide]

Saleem is the right person to review this change.

[davide]

Weak functions can actually be overridden -- I'm really wondering whether this causes issues here. Do you have an example [even if it's in swift] of a symbol that's not exported correctly?

[davide]

I'm really worried about this change -- and we should think about it very carefully because it might cause the LLVM verifier to freak out -- or even worse, causing random crashes when executing.

[labath]

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...

[jingham]

In 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.

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?

[labath]

In D78972#2008129, @jingham wrote:

In 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...

[jingham]

In D78972#2010033, @labath wrote:

In D78972#2008129, @jingham wrote:

In 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.

[labath]

In D78972#2011571, @jingham wrote:

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

Yep, I realized that is not the best example after posting that comment.

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.

You're describing the "normal" linkonce semantics. Things get more complex once you start having explicit instantiations: template void f<int>(); forces a compiler to emit an instantiation even though it is not needed (this results in the weak_odr linkage. extern template void f<int>(); inhibits instantiation -- the compiler will assume you have an explicit instantiation elsewhere. The fact that the explicit instantiation is "weak" allows for some interesting effects, but it's tricky to demonstrate them, as they all involve UB. Once can see some of that with normal compilation like this:

$ head a.cc b.cc c.cc
==> a.cc <==
#include <cstdio>

template<typename T> void f() { printf("Hello T\n"); }

template void f<int>(); // weak

==> b.cc <==
#include <cstdio>

extern "C" void _Z1fIiEvv() { printf("Hello _Z1fIiEvv\n"); } // strong

==> c.cc <==
#include <cstdio>

template<typename T> void f() { printf("Hello T\n"); }

extern template void f<int>(); // inhibit instantiation

int main() {
  f<int>();
}
$ c++ a.cc c.cc
$ ./a.out 
Hello T
$ c++ a.cc c.cc b.cc
$ ./a.out 
Hello _Z1fIiEvv

I don't know whether any of this is relevant for top-level expressions.

Incidentally it looks like we have a problem with function template specializations in top-level expressions, though I don't think that is related to this patch in any way:

(lldb) expr --top-level --
Enter expressions, then terminate with an empty line to evaluate:
  1: template<typename T> void f() { (void) printf("Hello T\n"); } 
  2: template<> void f<int>() { (void) printf("Hello int\n"); } 
  3: void g() { f<int>(); } 
(lldb) p g()
Hello int
(lldb) p f<int>()
Hello T

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.

Yep, at this point I am pretty lost about what should be the correct behavior, and whether the change is detectable with c++. However, I don't like the fact that the change has no test coverage.

[jingham]

In D78972#2012796, @labath wrote:

In D78972#2011571, @jingham wrote:

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

Yep, I realized that is not the best example after posting that comment.

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.

You're describing the "normal" linkonce semantics. Things get more complex once you start having explicit instantiations: template void f<int>(); forces a compiler to emit an instantiation even though it is not needed (this results in the weak_odr linkage. extern template void f<int>(); inhibits instantiation -- the compiler will assume you have an explicit instantiation elsewhere. The fact that the explicit instantiation is "weak" allows for some interesting effects, but it's tricky to demonstrate them, as they all involve UB. Once can see some of that with normal compilation like this:

$ head a.cc b.cc c.cc
==> a.cc <==
#include <cstdio>

template<typename T> void f() { printf("Hello T\n"); }

template void f<int>(); // weak

==> b.cc <==
#include <cstdio>

extern "C" void _Z1fIiEvv() { printf("Hello _Z1fIiEvv\n"); } // strong

==> c.cc <==
#include <cstdio>

template<typename T> void f() { printf("Hello T\n"); }

extern template void f<int>(); // inhibit instantiation

int main() {
  f<int>();
}
$ c++ a.cc c.cc
$ ./a.out 
Hello T
$ c++ a.cc c.cc b.cc
$ ./a.out 
Hello _Z1fIiEvv

I don't know whether any of this is relevant for top-level expressions.

Thanks for the explanation!

I think if you are bothering to create a template in --top-level your intent was to create something new. So the current setup, which will always emit an instantiation from the template that was injected into the Expression ASTContext, means subsequent experiments will use this version, which seems closest to that intent. That is certainly what happens today. If I build either version of the example above and do:

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

That behavior is unsurprisingly not affected by this patch.

I guess another use case might be "change all uses of this template function throughout the program to point to this new body". But can we even do that reliably? The linker has already resolved all the extant references to whichever of these symbols it decided should win. We'd have to interpose on that choice after the fact, which we don't do anywhere else, and seems tricky to get right.

So I think the current behavior is the best we can do.

Incidentally it looks like we have a problem with function template specializations in top-level expressions, though I don't think that is related to this patch in any way:

(lldb) expr --top-level --
Enter expressions, then terminate with an empty line to evaluate:
  1: template<typename T> void f() { (void) printf("Hello T\n"); } 
  2: template<> void f<int>() { (void) printf("Hello int\n"); } 
  3: void g() { f<int>(); } 
(lldb) p g()
Hello int
(lldb) p f<int>()
Hello T

The f<int>() specialization here emitted with external linkage, which makes sense. So that does not seem related. We need to be a smarter linker but it doesn't look like we remember the linkage of symbols made in expressions so we don't have the data to do this right as it stands.

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.

Yep, at this point I am pretty lost about what should be the correct behavior, and whether the change is detectable with c++. However, I don't like the fact that the change has no test coverage.

Well, there will be a swift test. I do have a case there where this is detectible. That's not entirely satisfactory, but it doesn't look like we can come up with something in C++ which we can actually use to test this.

[compnerd]

WeakODR requires that the symbol actually be discarded if not referenced. This will preserve the symbol even if unreferenced will it not? One approach might be to just create a DenseMap and check for any references and mark is as preserved otherwise just drop it. However, it _is_ ODR, which means that if it ever gets instantiated, we are well within our rights to give you the second definition rather than the first.

[labath]

In D78972#2035420, @compnerd wrote:

WeakODR requires that the symbol actually be discarded if not referenced. This will preserve the symbol even if unreferenced will it not? One approach might be to just create a DenseMap and check for any references and mark is as preserved otherwise just drop it. However, it _is_ ODR, which means that if it ever gets instantiated, we are well within our rights to give you the second definition rather than the first.

The question I am stuck at here is "referenced by who". E.g. does a subsequent top-level expression count? If so, then we can't drop it ever, because we don't know if they use will try to reference it in the future?

The reason I am stuck is because it's not clear to me which "model" is our expression evaluation try to emulate. The model closest to the current implementation (and also most reasonable sounding model, for me) seems to be to treat each expression as if it was defined in a separate translation unit, and then "linked" together. In that world, I think we should basically treat all non-private (static) linkage types as "external", as all of these are visible in other TUs (with slightly differing semantics, which are also hard to nail down because of the strangeness of the debugger expression evaluation model).

OTOH, in this model, it should be possible to declare the same "static" variable/function twice, but this currently fails with:

(lldb) expr --top-level -- static void myfunc() {}
(lldb) expr --top-level -- static void myfunc() {}
error: <user expression 4>:1:13: redefinition of 'myfunc'
static void myfunc() {}
            ^
<user expression 3>:1:13: previous definition is here
static void myfunc() {}
            ^

If that is intentional (i.e., not a bug) then a closer model would be to treat all expressions as belonging to a single translation unit, and the notion of externality does not make much sense (we could treat pretty much all linkage types the same way).

[jingham]

For expression evaluation in a REPL (which we only have for Swift, but if somebody wants to make such a thing for C++ we wouldn't be displeased) the model is compiling into a single translation unit. I think that's the only thing that makes sense. You don't want to have to artificially make everything public in the REPL so that you could use it in a subsequent REPL session.

--top-level was introduced mostly because C doesn't allow inner functions, and so without a way to say "compile this expression in no function context" you couldn't define new functions. That limits the utility of the expression parser in a seemingly artificial way, and you ended up having to write any code you wanted to reuse when you were using the expression parser for injecting code to do investigations in the inferior (like heap.py) awkward. That doesn't go one way or the other to how we want to use this for more than one evaluation. We could certainly discriminate between REPL and top-level in IRExecutionUnit and give them different behaviors. But unless there's a good reason to make them different, I'd rather not.

[labath]

I don't think we need to have different behavior for repl and --top-level. I'm mainly just confused about what is the right behavior to aim for.

So, assuming we want these to behave as if everything was in a single TU, my next question is: What is the purpose of the external flag? In this setup, it seems like everything should be "external" (i.e., made available to subsequent expressions)...

[jingham]

In D78972#2041680, @labath wrote:

I don't think we need to have different behavior for repl and --top-level. I'm mainly just confused about what is the right behavior to aim for.

So, assuming we want these to behave as if everything was in a single TU, my next question is: What is the purpose of the external flag? In this setup, it seems like everything should be "external" (i.e., made available to subsequent expressions)...

Eh, this got lost in my queue... I think the thing you are trying to avoid is if the compiler generates some static helper function in when building one of the actual TU's (as opposed to our fictitious TU) and then we we export that we might confuse it when it goes to emit the same thing in the next TU, since it isn't expecting to find it as an external symbol. Since I don't know all the games of this sort any of the languages we support is likely to play, I'd rather only export the things that were clearly meant to be exported.

[labath]

Right -- I can understand that. I'm not sure whether this is a real issue -- I'd expect that these compiler-internal symbols would normally have private (not "internal") linkage (and hence be caught by line 330) -- but I'm not insisting on exposing all of the internal linkage symbols either.

However, I think the phrase "meant to be exported" brings us back to the beginning, full circle. Objects with "weak" linkage are also "meant" to be exported (== externally visible in a normal compilation), just like their weak_odr counterparts. I think that the real bug here is that we've started special-casing individual linkage types, and that it's only a matter of time before we find ourselves needing to add another linkage type to this list. Llvm has a bunch of functions to check for the properties of linkage types, and it seems to me like we should be able to pick one of them. So, if we want to just expose the functions that would be visible during a normal compilation, the function to use is hasLocalLinkage() (negation of it, that is)

[jingham]

Switched to !hasLocalLinkage.

[jingham]

In D78972#2353643, @labath wrote:

Right -- I can understand that. I'm not sure whether this is a real issue -- I'd expect that these compiler-internal symbols would normally have private (not "internal") linkage (and hence be caught by line 330) -- but I'm not insisting on exposing all of the internal linkage symbols either.

However, I think the phrase "meant to be exported" brings us back to the beginning, full circle. Objects with "weak" linkage are also "meant" to be exported (== externally visible in a normal compilation), just like their weak_odr counterparts. I think that the real bug here is that we've started special-casing individual linkage types, and that it's only a matter of time before we find ourselves needing to add another linkage type to this list. Llvm has a bunch of functions to check for the properties of linkage types, and it seems to me like we should be able to pick one of them. So, if we want to just expose the functions that would be visible during a normal compilation, the function to use is hasLocalLinkage() (negation of it, that is)

The REPL is weird since it formally has to compile individual expressions as "compile units" but must also pretend they aren't individual compile units, but rather a weird kind of streaming single compile unit (weird because they also allow redefinitions that wouldn't be legal if the thing were one compile unit). So the compiler's normal notion of "localLinkage" isn't necessarily related to what the REPL needs to export. OTOH, localLinkage also works, so I'm happy to use it for now till we come up with a reason not to.

lldb/source/Expression/IRExecutionUnit.cpp

void IRExecutionUnit::GetRunnableInfo(Status &error, lldb::addr_t &func_addr,
   m_execution_engine_up->setProcessAllSections(true);
 
   m_execution_engine_up->DisableLazyCompilation();
 
   for (llvm::Function &function : *m_module) {
     if (function.isDeclaration() || function.hasPrivateLinkage())
       continue;
 
-    const bool external =
-        function.hasExternalLinkage() || function.hasLinkOnceODRLinkage();
+    const bool external = !function.hasLocalLinkage();
 
     void *fun_ptr = m_execution_engine_up->getPointerToFunction(&function);
 
     if (!error.Success()) {
       // We got an error through our callback!
       return;
     }