This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Disable structor optimizations at -O0
ClosedPublic

Authored by labath on May 10 2018, 3:46 AM.

Details

Summary

Merging of complete and base structors can degrade debug quality as it
will leave the debugger unable to locate the full object structor. This
is apparent when evaluating an expression in the debugger which requires
constructing an object of class which has had this optimization applied
to it. When compiling the expression, we pretend that the class and
its methods have been defined in another compilation unit, so the
expression compiler assumes the structor definition must be available,
whereas in reality the complete structor may have been omitted and all
it's uses replaced by the base structor.

This improves debug quality on non-darwin platforms (darwin does not
have -mconstructor-aliases on by default, so it is spared these
problems) and enable us to remove some workarounds from LLDB which attempt to
mitigate this issue.

This is tested by strenghtening the check in the existing
ctor-dtor-alias test. In the other aliasing tests I add -O1 to compiler
options to make sure the aliasing kicks in.

PS: Of the three ways we can do structor optimizations, only the RAUW
actually causes problems as it makes the symbol completely disappear, so
technically it should be sufficient to weaken RAUW to one of the other two
for -O0, but disabling optimizations completely looked like a more
principled solution.

Diff Detail

Repository
rC Clang

Event Timeline

labath created this revision.May 10 2018, 3:46 AM

Can we suppress this optimization only when we can't emit an alias? An alias shouldn't degrade debugging experience, and it's good to emit less code at -O0.

labath updated this revision to Diff 146309.May 11 2018, 5:38 AM

Yes we can do that. Only the RAUW optimization is nasty, I've checked that the
debugger is able to use the alias with no problem.

Updated the patch the reflect that.

rjmccall accepted this revision.May 11 2018, 10:24 AM

LGTM, thanks.

This revision is now accepted and ready to land.May 11 2018, 10:24 AM

Thank you for the quick review.

This revision was automatically updated to reflect the committed changes.

I've reverted the patch as it has caused miscompiles on non-trivial inputs. I'll update the patch when I gain a better understanding of what is going on.

labath reopened this revision.May 17 2018, 7:19 AM
This revision is now accepted and ready to land.May 17 2018, 7:19 AM
labath updated this revision to Diff 147315.May 17 2018, 7:19 AM

Updating with a new version of the patch. The problem with the previous version
was that it caused us to use C5/D5 comdats very aggressively (at -O0), and this
is not always correct. This uses a more nuanced approach:

  • for local symbols we can use aliases because we don't have to worry about comdats or other compilation units.
  • for linkonce symbols, we must emit each structor separately. Theoretically it would be possible to emit a C5 comdat, but then we would need to make sure it contains both (in case of destructors, all three) symbols. As linkonce symbols are emitted lazily, I did not find an easy way to ensure that this is always the case.
  • for available_externally, I also emit the structor, as the code seemed to be worried about emitting an alias in this case.
  • other linkage types are not affected by the optimization level. They either get put into a comdat (weak) or get aliased (external).

I also add more thorough tests for this behavior.

Overall I am not sure if this extra complexity is worth the few extra aliases we
can emit this way at -O0. Let me know what you think.

labath requested review of this revision.May 17 2018, 7:21 AM

Well, internal and external types are important cases. I'm fine with this. It's a pity that we can't express what we want with aliases, though.

This revision was not accepted when it landed; it landed in state Needs Review.May 21 2018, 4:53 AM
This revision was automatically updated to reflect the committed changes.

This was reverted in r333482 because it was causing "definition with same mangled name as another definition" errors in some internal google builds.

It turned out this uncovered an (unrelated) issue in module importing. This has now been fixed (r336240), so I'm planning to resubmit this patch. Let me know if you have any concerns.