Handle vbase destructors more intelligently

[sei-eschwartz]

See #61

[dberlin]

(One of my teams built the initial MS ABI support for Clang, so some of this stuff is stuck in my head) FWIW:

In the MS ABI, for a given class, there are possibly 4 types of destructor:

base destructor vbase destructor scalar deleting destructor vector deleting destructor

For purposes of inferencing, some interesting facts: the complete destructor does not accept a VTT parameter when there are virtual bases (IE if there is a VTT parameter, there are no virtual bases)

For virtual destructors, only one entry is reserved in the vftable, and will always point to the vector deleting destructor if it exists (the vector deleting destructor is the most general, and can be used to do all the other things). Both scalar and vector take bitfields saying what gets deleted and which things are arrays. IIRC (been a while), the scalar deleting destructor will call the vector deleting one with isarray bit set to 0 and the vector one will do the deletion. This often confuses people because they see vector delete functions called as a result of scalar deletes.

I do not remember what happens if you forbid the vector delete operator on a class (IE does it still generate a vector deleting destructor) :)

Beyond that: Because a non-base destructor can be emitted in a translation unit that lacks a definition for the destructor, non-base destructors must always delegate to or alias the base destructor.

[sei-eschwartz]

Thanks! This is very helpful.

It looks like some of this is documented here

We should definitely add a rule to detect virtual bases from the complete destructor! That sounds like an easy win.

[dberlin]

Yes, some of it is documented, but not all. One thing to keep in mind (specifically about MSVC 6.0 where this started) is that once we built an ABI fuzzer, as you would expect, we found plenty of bugs not just in our own stuff, but in how the older MS compilers were implementing their ABI in weirder cases. We reported them, and Microsoft has fixed most of them at this point. But it also means there are things that will not be completely consistent over every version even when they should be.

On Mon, Jun 8, 2020 at 5:19 AM sei-eschwartz [email protected] wrote:

Thanks! This is very helpful.

It looks like some of this is documented here https://github.com/llvm-mirror/clang/blob/master/lib/CodeGen/MicrosoftCXXABI.cpp#L175

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/cmu-sei/pharos/issues/75#issuecomment-640567998, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACPI24LDRZRC4HWXOOC7W3RVTJMRANCNFSM4MR4SJWA .

[sei-eschwartz]

Well, if you have any other insights or suggestions we're all ears.

@sei-ccohen I just looked at our test cases and we don't define a custom delete operator for any of them, which probably partially explains why it took so long for us to see this issue.

[dberlin]

I'm happy to help however i can. I'm analyzing a large program so i've started also poking around at the profiling of SWI (it currently takes a few days with SWI to do inference)

A few things to start:

1. I put the fuzzer we used for random c++ hierarchy/structure generation here: https://github.com/dberlin/superfuzz It was built to expose layout issues mostly, but may be helpful.

(Obviously, we had two compilers and were trying to make one look like the other - this is a little easier)

2. Though it's not for the MS ABI, there is also a C++ ABI testsuite in clang's test repository that covers the IA64 ABI per-section, and thus has very small testcases for each section of the ABI that should expose the basic interesting case differences. See https://github.com/llvm/llvm-test-suite/tree/master/ABI-Testsuite

3. There is a vtable dumper for Microsoft vtables in object/archive files here: https://github.com/llvm-mirror/llvm/commit/96171327dbe19cd0ebc277aa1607c417079e6821#diff-3e9be69e62123984263654b04f16ef34

This was tremendously helpful in trying to understand what the heck was going on :)

On Mon, Jun 8, 2020 at 7:59 AM sei-eschwartz [email protected] wrote:

Well, if you have any other insights or suggestions we're all ears.

@sei-ccohen https://github.com/sei-ccohen I just looked at our test cases and we don't define a custom delete operator for any of them, which probably partially explains why it took so long for us to see this issue.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/cmu-sei/pharos/issues/75#issuecomment-640684591, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACPI2YJSMR3MKXSPDQEBHTRVT4FZANCNFSM4MR4SJWA .

[sei-eschwartz]

Thanks! These look extremely helpful.

Improving the prolog performance is definitely not for the faint of heart. We've cleaned up a lot of bad/inefficient code over the last few releases. Some of the remaining inefficiencies are inherent to the design of incremental tabling which we use very heavily. We're working with the developer of SWI prolog on ways to address this, but it's not easy!

Anyway, if you want to look at profiling, I highly recommend using webstat. If you can share your sample(s) we can take a look at them as well (though no promises when). Also, if your samples complete within a few days and are not small, we would probably chalk that up as a win...