Page MenuHomePhabricator

List implicit operator== after implicit destructors in a vtable.
ClosedPublic

Authored by rsmith on Jan 16 2020, 6:37 PM.

Details

Summary

We previously listed first declared members, then implicit operator=,
then implicit operator==, then implicit destructors. Per discussion on
https://github.com/itanium-cxx-abi/cxx-abi/issues/88, put the implicit
equality comparison operators at the very end, after all special member
functions.

Diff Detail

Event Timeline

rsmith created this revision.Jan 16 2020, 6:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 16 2020, 6:37 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I see two reasonable approaches here: we can rely on the implicit members being added in the right order by Sema, as this patch seems to do, or we can broaden the special treatment of implicit virtual functions in the v-table layout code. The latter is more complex (we'd basically need to collect and sort all the implicit virtual functions), but it reliably isolates the ABI from implementation decisions like the order in which Sema processes implicit members and whether some lookup might have triggered an implicit declaration at a point we didn't expect.

I know that Sema used to have a lot of issues with implicitly declaring special members, but I also know that that was bound up with how we tracked special state for the class, and that code has changed a lot. If you feel that that's an unlikely problem now, and you're comfortable with potentially having to generate implicit declarations of special members in different orders to satisfy different ABIs, I can accept this approach. (It would be good to know what order MSVC will emit these members in, though.)

I know that Sema used to have a lot of issues with implicitly declaring special members, but I also know that that was bound up with how we tracked special state for the class, and that code has changed a lot. If you feel that that's an unlikely problem now, and you're comfortable with potentially having to generate implicit declarations of special members in different orders to satisfy different ABIs, I can accept this approach.

I'm not particularly overjoyed by having an ABI decision determined by the order in which Sema chooses to implicitly declare these members. I think it's unlikely to have stability problems (we explicitly force declaration of virtual implicit members at the end of the class before they can have been lazily declared), but avoiding a reliance on Sema's behavior would be nice. It'd be hard to avoid relying on Sema getting the order of implicit operator== functions right, though, as I don't think we want vtable layout to be responsible for figuring out the correspondence between implicit operator== functions and defaulted operator<=> functions. Please take a look at this alternative approach and see what you think.

rsmith updated this revision to Diff 238881.Jan 17 2020, 1:45 PM
  • Explicitly put implicit virtual functions in the right order, rather
rjmccall accepted this revision.Jan 17 2020, 3:07 PM

It wouldn't be a bad idea to track that correspondence in the AST just on the general principle that it's useful information (for diagnostics, tooling, etc.) that's otherwise hard to recreate. But I agree that it's also not problematic to expect Sema to add them in the proper order, and if we don't have an ABI that needs the correspondence directly (e.g. because it wants to emit the operators in pairs), it's fine to rely on it.

Putting the operators at the end makes sense to me. It seems likely that an arbitrary implementation is always more likely to need to process the implicit members first.

I'm much happier with this implementation; LGTM.

This revision is now accepted and ready to land.Jan 17 2020, 3:07 PM
This revision was automatically updated to reflect the committed changes.

It is also failing on the other 32-bit arm bots, example http://lab.llvm.org:8011/builders/clang-cmake-armv7-quick/builds/13052/steps/ninja%20check%201/logs/FAIL%3A%20Clang%3A%3Avirtual-compare.cpp
Looking at the failure

// CHECK-SAME: @_ZThn8_N1AD1Ev

and the output on an Arm machine

$_ZThn4_N1AD1Ev = comdat any

$_ZThn4_N1AD0Ev = comdat any

$_ZThn4_N1AaSERKS_ = comdat any

It might be a 32-bit/64-bit specific expectation.

This test is still failing on the arm bots and also with my downstream ARM compiler validation.

Hi Richard,

http://lab.llvm.org:8011/builders/llvm-clang-win-x-armv7l builder has been broken by this patch during few days (failed "Clang::virtual-compare.cpp" test).
Sorry, but I'm going to revert these changes.

Hi Richard,

http://lab.llvm.org:8011/builders/llvm-clang-win-x-armv7l builder has been broken by this patch during few days (failed "Clang::virtual-compare.cpp" test).
Sorry, but I'm going to revert these changes.

Thanks for the revert; fixed and re-landed.