Bug 69777 - Give a warning when virtual function is devirtualized into a __cxa_pure_virtual call
Summary: Give a warning when virtual function is devirtualized into a __cxa_pure_virtu...
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 4.9.1
: P3 enhancement
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic
Depends on:
Blocks: new-warning
  Show dependency treegraph
 
Reported: 2016-02-11 23:06 UTC by Vadim Zeitlin
Modified: 2019-03-02 04:41 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail: 4.9.1, 5.5.0, 6.4.0, 7.2.1, 8.0
Last reconfirmed: 2017-12-30 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Vadim Zeitlin 2016-02-11 23:06:58 UTC
For the reasons of compatibility with old compilers which don't define some Windows COM interfaces in their headers, we define these interfaces ourselves, e.g. here: https://github.com/wxWidgets/wxWidgets/blob/WX_3_0_2/src/msw/textentry.cpp#L92

It seems that g++ 4.9.1 is smart enough to deduce that no classes deriving from a class declared in an anonymous namespace can exist outside of the current translation unit and so all IAutoCompleteDropDown pointers can only point to objects of the base IAutoCompleteDropDown type itself and then it proceeds with devirtualizing the calls to its methods to just directly call __cxa_pure_virtual() (i.e. instead of the usual indirect call I just see "call __cxa_pure_virtual" in the x86 disassembly).

This is technically correct, of course, and rather impressive, but as the generated code will always crash during run-time, couldn't g++ give a warning when devirtualizing a function into a pure virtual call? This would have helped me a lot when tracking down this crash (which was not totally obvious because the stack was shifted on entry into __cxa_pure_virtual() as the calling code temporarily changed esp register value).

I have a small (but still ~50 lines of code) example that can be used to reproduce this behaviour, please let me know if you'd like me to attach it.
Comment 1 Manuel López-Ibáñez 2016-02-14 22:48:16 UTC
(In reply to Vadim Zeitlin from comment #0)
> I have a small (but still ~50 lines of code) example that can be used to
> reproduce this behaviour, please let me know if you'd like me to attach it.

If it is the preprocessed file (*.i*) that triggers the bug, generated by adding -save-temps to the complete compilation command, then please do so. 50 lines is small enough. Otherwise https://gcc.gnu.org/bugs/minimize.html
Comment 2 Eric Gallager 2017-09-28 14:58:31 UTC
(In reply to Manuel López-Ibáñez from comment #1)
> (In reply to Vadim Zeitlin from comment #0)
> > I have a small (but still ~50 lines of code) example that can be used to
> > reproduce this behaviour, please let me know if you'd like me to attach it.
> 
> If it is the preprocessed file (*.i*) that triggers the bug, generated by
> adding -save-temps to the complete compilation command, then please do so.
> 50 lines is small enough. Otherwise https://gcc.gnu.org/bugs/minimize.html

Thus WAITING for it to be attached.
Comment 3 Eric Gallager 2017-12-30 14:25:35 UTC
(In reply to Eric Gallager from comment #2)
> (In reply to Manuel López-Ibáñez from comment #1)
> > (In reply to Vadim Zeitlin from comment #0)
> > > I have a small (but still ~50 lines of code) example that can be used to
> > > reproduce this behaviour, please let me know if you'd like me to attach it.
> > 
> > If it is the preprocessed file (*.i*) that triggers the bug, generated by
> > adding -save-temps to the complete compilation command, then please do so.
> > 50 lines is small enough. Otherwise https://gcc.gnu.org/bugs/minimize.html
> 
> Thus WAITING for it to be attached.

Closing due to the rule that bugs in WAITING for 3 months with no response can be closed. Feel free to reopen if attaching the requested example after all.
Comment 4 Vadim Zeitlin 2017-12-30 15:19:44 UTC
Sorry, I've somehow forgot to provide the example, but here it is in its most minimal form:

namespace {
    struct AnonB {
        virtual bool foo() const = 0;
        virtual ~AnonB() {}
    };
}

struct RealB {
    virtual bool foo() const = 0;
    virtual ~RealB() {}
};

AnonB* create()
{
    struct D : RealB {
        virtual bool foo() const { return true; }
    };

    return reinterpret_cast<AnonB*>(new D);
}

int main() {
    return create()->foo(); // please ignore the memory leak here
}

Compiling this without optimizations "works", while with -O2 or -O3 the program terminates due to a pure virtual function call and the code looks just like this:

(gdb) disassemble
Dump of assembler code for function main():
=> 0x0000000000400670 <+0>:     mov    edi,0x8
   0x0000000000400675 <+5>:     sub    rsp,0x8
   0x0000000000400679 <+9>:     call   0x400660 <_Znwm@plt>
   0x000000000040067e <+14>:    mov    QWORD PTR [rax],0x4008b0
   0x0000000000400685 <+21>:    mov    rdi,rax
   0x0000000000400688 <+24>:    call   0x400650 <__cxa_pure_virtual@plt>
End of assembler dump.

Again, I understand that the program invokes undefined behaviour and the compiler is perfectly in its right to do what it does. But in practice casts such as above are always required when using COM with its QueryInterface() and it would be nice if the compiler could warn about this devirtualization because it can never be intentional, AFAICS.

Also notice that giving the namespace a name (and replacing AnonB with NS::AnonB) also prevents the devirtualization from happening.

Finally, I've tested this with (the originally reported) 4.9, 5.5, 6.4 and 7.2 and the behaviour is almost the same for all of them, but 7.2 does give this warning

puredevirt.cpp: In function ‘int main()’:
puredevirt.cpp:24:25: warning: ‘<anonymous>’ is used uninitialized in this function [-Wuninitialized]
     return create()->foo();
            ~~~~~~~~~~~~~^~

I guess this already partially addresses my request because at least there is a warning, but its wording is not especially clear. Looking at the disassembly

(gdb) disassemble
Dump of assembler code for function main:
=> 0x0000555555554600 <+0>:     sub    rsp,0x8
   0x0000555555554604 <+4>:     mov    edi,0x8
   0x0000555555554609 <+9>:     call   0x5555555545e0 <_Znwm@plt>
   0x000055555555460e <+14>:    call   0x5555555545d0 <__cxa_pure_virtual@plt>
End of assembler dump.

it becomes clear that it's the object on which foo() is called which is not initialized (i.e. it doesn't even bother loading its address before calling __cxa_pure_virtual, which makes sense), but it's not really obvious from just the error message.
Comment 5 Eric Gallager 2017-12-31 02:28:54 UTC
(In reply to Vadim Zeitlin from comment #4)
> Sorry, I've somehow forgot to provide the example, but here it is in its
> most minimal form:
> 
> namespace {
>     struct AnonB {
>         virtual bool foo() const = 0;
>         virtual ~AnonB() {}
>     };
> }
> 
> struct RealB {
>     virtual bool foo() const = 0;
>     virtual ~RealB() {}
> };
> 
> AnonB* create()
> {
>     struct D : RealB {
>         virtual bool foo() const { return true; }
>     };
> 
>     return reinterpret_cast<AnonB*>(new D);
> }
> 
> int main() {
>     return create()->foo(); // please ignore the memory leak here
> }
> 
> Compiling this without optimizations "works", while with -O2 or -O3 the
> program terminates due to a pure virtual function call and the code looks
> just like this:
> 
> (gdb) disassemble
> Dump of assembler code for function main():
> => 0x0000000000400670 <+0>:     mov    edi,0x8
>    0x0000000000400675 <+5>:     sub    rsp,0x8
>    0x0000000000400679 <+9>:     call   0x400660 <_Znwm@plt>
>    0x000000000040067e <+14>:    mov    QWORD PTR [rax],0x4008b0
>    0x0000000000400685 <+21>:    mov    rdi,rax
>    0x0000000000400688 <+24>:    call   0x400650 <__cxa_pure_virtual@plt>
> End of assembler dump.
> 
> Again, I understand that the program invokes undefined behaviour and the
> compiler is perfectly in its right to do what it does. But in practice casts
> such as above are always required when using COM with its QueryInterface()
> and it would be nice if the compiler could warn about this devirtualization
> because it can never be intentional, AFAICS.
> 
> Also notice that giving the namespace a name (and replacing AnonB with
> NS::AnonB) also prevents the devirtualization from happening.
> 
> Finally, I've tested this with (the originally reported) 4.9, 5.5, 6.4 and
> 7.2 and the behaviour is almost the same for all of them, but 7.2 does give
> this warning
> 
> puredevirt.cpp: In function ‘int main()’:
> puredevirt.cpp:24:25: warning: ‘<anonymous>’ is used uninitialized in this
> function [-Wuninitialized]
>      return create()->foo();
>             ~~~~~~~~~~~~~^~
> 
> I guess this already partially addresses my request because at least there
> is a warning, but its wording is not especially clear. Looking at the
> disassembly
> 
> (gdb) disassemble
> Dump of assembler code for function main:
> => 0x0000555555554600 <+0>:     sub    rsp,0x8
>    0x0000555555554604 <+4>:     mov    edi,0x8
>    0x0000555555554609 <+9>:     call   0x5555555545e0 <_Znwm@plt>
>    0x000055555555460e <+14>:    call   0x5555555545d0
> <__cxa_pure_virtual@plt>
> End of assembler dump.
> 
> it becomes clear that it's the object on which foo() is called which is not
> initialized (i.e. it doesn't even bother loading its address before calling
> __cxa_pure_virtual, which makes sense), but it's not really obvious from
> just the error message.

Reopening; confirmed that the example only prints the -Wuninitialized warning, but I get different disassembly, although that's probably due to target differences. (my gdb doesn't disassemble it properly when optimized, but if I use -save-temps and examine the assembly, I can see that it still contains the call to __cxa_pure_virtual at least)
Comment 6 Eric Gallager 2019-03-02 04:41:18 UTC
this would be a new warning, so making it block the relevant meta-bug