[lldb] Don't emit artificial constructor declarations as global functions

[teemperor]

Summary

When we have a artificial constructor DIE, we currently create from that a global function with the name of that class.
That ends up causing a bunch of funny errors such as "must use 'struct' tag to refer to type 'Foo' in this scope" when
doing Foo f. Also causes that constructing a class via Foo() actually just calls that global function.

The fix is that when we have an artificial method decl, we always treat it as handled even if we don't create a CXXMethodDecl
for it (which we never do for artificial methods at the moment).

Fixes rdar://55757491 and probably some other radars.

[teemperor]

• Enabled test on Linux. That's only failing on my box (Thanks Pavel)

[teemperor]

• Document dedicated test a bit better.

[vsk]

Great find. The changes in this patch makes sense to me locally, but I'm having trouble picking up on the context necessary to confidently 'lgtm'. + @JDevlieghere & @labath to get some more experienced people.

I'd love to see the big switch in ParseTypeFromDWARF broken up into small, well-commented functions bit-by-bit -- when that's done, I think I'll have a much better chance at reviewing changes. If folks agree that that's a reasonable refactor, I'd be happy to send a few patches (perhaps starting with the DW_TAG_subroutine_type handling).

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

1010

nit: we don't ?

[shafik]

So if I look at ClangASTContext::AddMethodToCXXRecordType(...) it has the following:

if (is_artificial)
  return nullptr; // skip everything artificial

but why? if I look at the godbolt w/ an AST dump it is not creating as a global function and it would seem that we would want this behavior as well.

[jingham]

In D68130#1686530, @shafik wrote:

So if I look at ClangASTContext::AddMethodToCXXRecordType(...) it has the following:

if (is_artificial)
  return nullptr; // skip everything artificial

but why? if I look at the godbolt w/ an AST dump it is not creating as a global function and it would seem that we would want this behavior as well.

IIRC, it is because we can't count on which artificial functions get generated in the debug information in any given CU. It depends on what gets used. If we write artificial functions into the AST it makes it harder to unique instances of the class. And it doesn't matter whether we do or not because the compiler will generate them into the Scratch Context when it needs them.

[shafik]

In D68130#1686549, @jingham wrote:

....
IIRC, it is because we can't count on which artificial functions get generated in the debug information in any given CU. It depends on what gets used. If we write artificial functions into the AST it makes it harder to unique instances of the class. And it doesn't matter whether we do or not because the compiler will generate them into the Scratch Context when it needs them.

That makes sense and it holds up to some basic experiments.

@teemperor you should add a detailed comment to note that is why we skip artificial memebers.

This is a quick examples where it does not generate the constructor:

struct Foo {
  // Triggers that we emit an artificial constructor for Foo.
  virtual ~Foo() = default;
};

void f( Foo &f) {
}

[labath]

In D68130#1686184, @vsk wrote:

Great find. The changes in this patch makes sense to me locally, but I'm having trouble picking up on the context necessary to confidently 'lgtm'. + @JDevlieghere & @labath to get some more experienced people.

I am not very familiar with this code either, but it seems fairly safe and straightforward to me...

I'd love to see the big switch in ParseTypeFromDWARF broken up into small, well-commented functions bit-by-bit -- when that's done, I think I'll have a much better chance at reviewing changes. If folks agree that that's a reasonable refactor, I'd be happy to send a few patches (perhaps starting with the DW_TAG_subroutine_type handling).

+100 to that. I've tried to reduce the function down a bit by splitting the dwarf parsing stuff into the separate ParsedTypeAttributes class, but the amount of code left is still far too much for a single function. If it's getting in the way, feel free to undo/refactor the attribute parsing code...

[teemperor]

So, are there any concerns about this (beside the typo which I'll fix pre-commit) and Shafik's request for documentation (which will be another NFC commit)?

[stella.stamenova]

Unfortunately, this is failing on the windows bot:

http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/9961

[teemperor]

Yeah, seems like constructing objects in expressions isn't implemented on Windows. I'm not sure if there is a reliable way to test that constructors aren't shadowed by these global functions if constructors themselves don't work on Windows, but I filed llvm.org/pr43707 for the underlying bug and x-failed the tests. Will push in a few minutes.

[stella.stamenova]

In D68130#1713620, @teemperor wrote:

Yeah, seems like constructing objects in expressions isn't implemented on Windows. I'm not sure if there is a reliable way to test that constructors aren't shadowed by these global functions if constructors themselves don't work on Windows, but I filed llvm.org/pr43707 for the underlying bug and x-failed the tests. Will push in a few minutes.

Thanks!

Btw, I also see TestCallOverriddenMethod failing on Ubuntu but there's no official bot for that.

FAIL: test_call_on_temporary_dwarf (TestCallOverriddenMethod.ExprCommandCallOverriddenMethod)

Test calls to overridden methods in derived classes.

---

Traceback (most recent call last):

File "/home/e2admin/vstsdrive/_work/115/s/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1748, in test_method
  return attrvalue(self)
File "/home/e2admin/vstsdrive/_work/115/s/lldb/packages/Python/lldbsuite/test/decorators.py", line 111, in wrapper
  func(*args, **kwargs)
File "/home/e2admin/vstsdrive/_work/115/s/lldb/packages/Python/lldbsuite/test/commands/expression/call-overridden-method/TestCallOverriddenMethod.py", line 71, in test_call_on_temporary
  self.expect("expr Base().foo()", substrs=["1"])
File "/home/e2admin/vstsdrive/_work/115/s/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 2309, in expect
  inHistory=inHistory)
File "/home/e2admin/vstsdrive/_work/115/s/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 2068, in runCmd
  msg if (msg) else CMD_MSG(cmd))

AssertionError: False is not True : Command 'expr Base().foo()
Error output:
error: Execution was interrupted, reason: signal SIGABRT.
The process has been returned to the state before expression evaluation.
' returns successfully
Config=x86_64-/home/e2admin/vstsdrive/_work/115/b/bin/clang-10

[stella.stamenova]

Oh, and TestCallOverridenMethod still has some failures on Windows:

http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/9996/steps/test/logs/stdio

[teemperor]

Sorry, seems like I forgot to delete these tests when I extracted them into their own test function.

About the ubuntu failure: I think I have a similar failure on my Arch Linux machine when constructing an object in another test, so I fear that the whole object construction in the expression parser seems to be not working properly. I'll write a separate test for constructing different kind of objects and then we see what platform actually survives those tests. In the meanwhile I can also X-Fail that test on Linux.

[labath]

It doesn't seem to be failing here http://lab.llvm.org:8014/builders/lldb-x86_64-debian.

[teemperor]

Well, I'm fine with x-failing it on Linux, even though I guess at some point someone (i.e., probably me) has to figure out why this stuff is broken in the expression parser.

[labath]

In D68130#1715433, @teemperor wrote:

Well, I'm fine with x-failing it on Linux, even though I guess at some point someone (i.e., probably me) has to figure out why this stuff is broken in the expression parser.

I said doesn't. What I was trying to say is that if you xfail it on linux, the test fill start "failing" on all the configurations where if was succeeding before.

That said, I am quite surprised that this behaves nondeterministically, as the test is very hermetic. If I had to guess, I would actually say that this hermeticity is the problem. Synthesizing the default constructor and calling it probably generates some calls into the c++(abi) library. Depending on the system configuration (static/dynamic linking, -Wl,--as-needed, etc), the necessary external functions may or may not be available. My guess would be that ensuring the test creates at least one other (unrelated) object will ensure the c++ runtime support functions are all there and this test succeeds consistently. However, as it is succeeding on the machines I have around right now, I don't have a way to verify this hypothesis.

[teemperor]

Whoops, I meant skipIf'ing, not x-failing.

Anyway, I can reproduce the SIBABRT fail on my Arch Linux CI, so it seems that Stella's ubuntu machine isn't at fault here but instead some other issue. If I disable unwind-on-error I get this:

lldb version 10.0.0 ([e-mail]:Teemperor/llvm-project revision 94df949a1f120014e9c715fc43d87398ed3a880a)
  clang revision 94df949a1f120014e9c715fc43d87398ed3a880a
  llvm revision 94df949a1f120014e9c715fc43d87398ed3a880a
warning: (x86_64) /home/teemperor/work/ci/build/lldb-test-build.noindex/commands/expression/call-overridden-method/TestCallOverriddenMethod.test_call_on_temporary_dwarf/a.out 0x7fffffff00000101: DW_AT_specification(0x000000d5) has no decl
inlinable function call in a function with debug info must have a !dbg location
  call void @_ZN4BaseC2Ev(%class.Base* %this1) #4
inlinable function call in a function with debug info must have a !dbg location
  call void @_ZN4BaseC2Ev(%class.Base* %this1) #4
LLVM ERROR: Broken module found, compilation aborted!

[labath]

That is interesting. Can you send me the binary produced by this test? I'd like to compare it with what I get locally. (I can also send you mine if you're interested)

lldb/packages/Python/lldbsuite/test/commands/expression/call-overridden-method/TestCallOverriddenMethod.py

def test(self):
 
         # Test call to overridden method in derived class (this will fail if the
         # overrides table is not correctly set up, as Derived::foo will be assigned
         # a vtable entry that does not exist in the compiled program).
         self.expect("expr d.foo()", substrs=["2"])
 
         # Test calling the base class.
         self.expect("expr realbase.foo()", substrs=["1"])
+
+        # Test with locally constructed instances.
+        self.expect("expr Base().foo()", substrs=["1"])
+        self.expect("expr Derived().foo()", substrs=["2"])

lldb/packages/Python/lldbsuite/test/commands/expression/call-overridden-method/main.cpp

 class Base {
 public:
   virtual ~Base() {}
   virtual int foo() { return 1; }
 };
 
 class Derived : public Base {
 public:
   virtual int foo() { return 2; }
 };
 
 int main() {
   Base realbase;
+  realbase.foo();
   Derived d;
   Base *b = &d;
   return 0; // Set breakpoint here
 }

lldb/packages/Python/lldbsuite/test/commands/expression/ignore-artificial-constructors/TestIgnoreArtificialConstructors.py

+from lldbsuite.test import lldbinline
+from lldbsuite.test import decorators
+
+lldbinline.MakeInlineTest(__file__, globals(), None)

lldb/packages/Python/lldbsuite/test/commands/expression/ignore-artificial-constructors/main.cpp

+struct Foo {
+  // Triggers that we emit an artificial constructor for Foo.
+  virtual ~Foo() = default;
+};
+
+int main() {
+  Foo f;
+  // Try to construct foo in our expression.
+  return 0; //%self.expect("expr Foo()", substrs=["(Foo) $0 = {}"])
+}

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

if (attrs.name) {
                           m_ast.AddMethodToCXXRecordType(
                               class_opaque_type.GetOpaqueQualType(),
                               attrs.name.GetCString(), attrs.mangled_name,
                               clang_type, attrs.accessibility, attrs.is_virtual,
                               is_static, attrs.is_inline, attrs.is_explicit,
                               is_attr_used, attrs.is_artificial);
 
                       type_handled = cxx_method_decl != NULL;
+                      // Artificial methods are always handled even when don't
+                      // create a new declaration for them.
+                      type_handled |= attrs.is_artificial;
 
-                      if (type_handled) {
+                      if (cxx_method_decl) {
                         LinkDeclContextToDIE(
                             ClangASTContext::GetAsDeclContext(cxx_method_decl),
                             die);
 
                         ClangASTMetadata metadata;
                         metadata.SetUserID(die.GetID());
 
                         if (!object_pointer_name.empty()) {