[Lldb-commits] [PATCH] D79811: WIP: Reenable creation of artificial methods in AddMethodToCXXRecordType(...)

Wed May 13 10:17:30 PDT 2020

shafik marked an inline comment as done.
shafik added a comment.

In D79811#2033399 <https://reviews.llvm.org/D79811#2033399>, @labath wrote:

> In general, I think this is a good direction to go in. We can run into the same kinds of problems even with non-artificial functions -- either the application can have real ODR violations, or we can introduce ODR violations during expression eval by playing fast-and-loose with anonymous namespaces.
>
> For all of those cases we need a more general approach to handling the "I have two versions of a class, and they don't really match" problem.
>
> As for a test case where this would fail, I haven't had first hand experience with this, but I would guess it involves two shared libraries, one which contains the definition of this artificial constructor, and one which doesn't. And then pulling both of these class definitions into the same expression...

Yes, in some offline conversations with @clayborg that was the scenario he had seen this come up before. I was not able to synthesize a test case that ran into this issue but that does not mean it is not possible. Perhaps I just wasn't creative enough ;-)

================
Comment at: lldb/test/API/lang/cpp/constructors/main.cpp:11
+  int x=10;
+  ClassForSideEffect cse = ClassForSideEffect(100);
 };
----------------
labath wrote:
> Just for my own education, how would this fail without the patch? During expression evaluation, we would synthesize a call to the default constructor of `ClassForSideEffect`, instead of the one taking int=100? Or is it something else?
So it would not initialize `x` to `10` and it would not call `ClassForSideEffect(100)` it would just default initialize it. I don't quite understand why, I plan to dig into that some more but I wanted to get the conversation rolling.

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

https://reviews.llvm.org/D79811