Page MenuHomePhabricator

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

Authored by teemperor on Sep 27 2019, 5:04 AM.

Details

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.

Diff Detail

Event Timeline

teemperor created this revision.Sep 27 2019, 5:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2019, 5:04 AM
teemperor updated this revision to Diff 222140.Sep 27 2019, 5:23 AM
teemperor edited the summary of this revision. (Show Details)
  • Enabled test on Linux. That's only failing on my box (Thanks Pavel)
teemperor updated this revision to Diff 222162.Sep 27 2019, 6:29 AM
  • Document dedicated test a bit better.
vsk added a subscriber: labath.Sep 27 2019, 10:42 AM

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 added a subscriber: shafik.Sep 27 2019, 2:21 PM

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.

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.

....
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) {
}
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...

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)?

aprantl accepted this revision.Oct 14 2019, 9:29 AM
This revision is now accepted and ready to land.Oct 14 2019, 9:29 AM
This revision was automatically updated to reflect the committed changes.

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.

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

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.

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.

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 added a comment.EditedTue, Oct 29, 3:38 AM

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 (git@github.com: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!

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)