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

Pavel Labath via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Fri May 15 01:35:44 PDT 2020


labath added a comment.

I just realized a different problem with this approach. Since the constructor DIE is only emitted when it is actually used, we could run into problems/inconsistencies even within a single library. We currently parse only one instance of a class description -- assuming all of them to be equivalent. This means that if we happen to pick the class description without the constructor as *the* definition for the class, then the we would still fall back to the (incorrect) constructor that clang generates for us. In this test case that could be demonstrated by having another compile unit, which only uses `ClassWithImplicitCtor`, but never constructs it, and then arranging it such that this file is picked for the class definition (probably involves putting this file first on the link line, or ensuring the first breakpoint hit is in that file).

Fixing that is going to be tricky. The best I can come up with right now, is injecting a constructor declaration into a class even if DWARF does not describe one. Then, if the user calls it, we go searching for the constructor symbol in the module. If we find one, we use it. If we don't, we bail out of the expression.

I think that approach would result in the most correct behavior, and it would also solve the structural equivalence issues that Greg is worried about (all classes would have the default constructor). However, it does have some serious drawbacks: The presence of a user-specified constructor (which is how this will appear to clang) disables certain ways of initializing the class. So for instance, the user would not be able to do this,

  code:
  struct Simple { int a, b, c; };
  lldb:
  p Simple() # value-initialized to zero
  p Simple{47} # compound initialization, a=47, b=c=0
  p Simple{1, 2, 3} # compount initalization with all members

That is kind of correct, since we're actually not able to differentiate the above class, from a class like this `struct SeeminglySimple { int a = random(), b = random(), c = random(); };`. However, I have a feeling it would annoy a lot of people. The best we could do is make the third expression work by synthesizing an `(int, int, int)` constructor initializing all members, as that will not use the default initializers (if they existed).

I have also considered the idea of trawling through all definitions of the class, in search of the default constructor. However:

- it's possible that no compile unit uses it, but the class still contained default initializers. Then, we will still get the initialization wrong.
- it won't help with partial compound initialization (second expression)
- it will slow us down (but maybe not so much with indexes, as we know the constructor name)

So, it does seem like some help from the compiler is called for. Maybe the compiler could attach a `DW_AT_default_value` with an empty expression to the members which have default initializers, as a way of saying "this member has a default initializer, but I can't/won't tell you what it is" ? Then we could search for this attribute and refuse constructing classes which have it. That would enable us to keep `p Simple{}` working, while preventing `p SeeminglySimple{}`.

In D79811#2037636 <https://reviews.llvm.org/D79811#2037636>, @dblaikie wrote:

> In D79811#2036112 <https://reviews.llvm.org/D79811#2036112>, @labath wrote:
>
> > This example also demonstrates what I believe *is* a bug in the compiler. The inherited constructor will get `DW_AT_name(A)`:
> >
> >   0x0000003f:   DW_TAG_structure_type
> >                   DW_AT_calling_convention	(DW_CC_pass_by_value)
> >                   DW_AT_name	("B")
> >                   DW_AT_byte_size	(0x01)
> >                   DW_AT_decl_file	("/home/pavelo/ll/build/opt/<stdin>")
> >                   DW_AT_decl_line	(1)
> >  
> >   0x00000048:     DW_TAG_inheritance
> >                     DW_AT_type	(0x0000005f "A")
> >                     DW_AT_data_member_location	(0x00)
> >  
> >   0x0000004e:     DW_TAG_subprogram
> >                     DW_AT_name	("A")
> >                     DW_AT_declaration	(true)
> >                     DW_AT_artificial	(true)
> >                     DW_AT_external	(true)
> >
> >
> > That doesn't sound correct to me. Gcc emits the name as `B`, which seems to be much better. Looping in @dblaikie for thoughts.
>
>
> Agreed - sounds like a bug/something to fix in Clang there - if you'd like to fix it/file a bug/etc.


Yeah, I guess should do that, before we start needing to work around this bug for compatibility purposes. :)


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

https://reviews.llvm.org/D79811





More information about the lldb-commits mailing list