Re: MSVC 7.1 bad code generation

[Martin Sebor]

Farid Zaripov wrote:

  The 22.locale.money.put.cpp test fails on MSVC 7.1 (15s build type)
with buffer overrun error due to bad code generation.

Great detective work! Since our money_base class has no data members
the patch is fine.

Your analysis of the assembly looks right on the money. It seems MSVC
7.1 implementation of the empty base optimization (EBO) has a bug. It
might have something to do with it being second (or not first) in the
list of bases. If you can reproduce it in a simple test case and file
it as an External issue just for the record that would be great.

I assume the bug doesn't exist in MSVC 8.0 so we don't need to worry
about letting Microsoft know about it?

Martin

  Here the assembly code for moneypunct ctor:
-------------
    _EXPLICIT moneypunct (_RWSTD_SIZE_T __refs = 0)
        : _RW::__rw_facet (__refs), money_base () { }

004018C0 push ebp 004018C1 mov ebp,esp 004018C3 push ecx 004018C4 mov dword ptr [ebp-4],ecx 004018C7 mov eax,dword ptr [__refs] 004018CA push eax 004018CB mov ecx,dword ptr [this] 004018CE call __rw::__rw_facet::__rw_facet (412E20h) 004018D3 xor ecx,ecx 004018D5 mov edx,dword ptr [this] 004018D8 add edx,38h // the sizeof

(moneypunct) == 0x38
004018DB  mov         byte ptr [edx],cl           // here the place of
the buffer overrun

004018DD mov eax,dword ptr [this] 004018E0 mov dword ptr [eax],offset std::moneypunct<char,0>::`vftable' (488838h) 004018E6 mov eax,dword ptr [this] 004018E9 mov esp,ebp 004018EB pop ebp 004018EC ret 4 -------------

  When I commented the money_base () call the test succeeded and
assembly code has changed to:
-------------
    _EXPLICIT moneypunct (_RWSTD_SIZE_T __refs = 0)
        : _RW::__rw_facet (__refs)/*, money_base ()*/ { }

004018C0 push ebp 004018C1 mov ebp,esp 004018C3 push ecx 004018C4 mov dword ptr [ebp-4],ecx 004018C7 mov eax,dword ptr [__refs] 004018CA push eax 004018CB mov ecx,dword ptr [this] 004018CE call __rw::__rw_facet::__rw_facet (412E20h) 004018D3 mov ecx,dword ptr [this] 004018D6 mov dword ptr [ecx],offset std::moneypunct<char,0>::`vftable' (488838h) 004018DC mov eax,dword ptr [this] 004018DF mov esp,ebp 004018E1 pop ebp 004018E2 ret 4 -------------

  Here the same assembly, but in 12s configuration:

before change:
-------------
    const PunctT pun;

004018B1 push 1 004018B3 lea ecx,[esp+0B4h] 004018BA call __rw::__rw_facet::__rw_facet (40A770h)

004018BF  mov         byte ptr [esp+0E8h],bl            // 0xE8 - 0xB4
== 0x34, so here not buffer overrun,

// but maybe changed last 4-byte member of the __rw_facet // (I suppose is _C_pid)

004018C6  mov         dword ptr [esp+0B0h],offset

Punct<char,0>::`vftable' (43A258h) -------------

after change:
-------------
    const PunctT pun;

00401891 push 1 00401893 lea ecx,[esp+0B4h] 0040189A call __rw::__rw_facet::__rw_facet (40A720h) 0040189F mov dword ptr [esp+0B0h],offset Punct<char,0>::`vftable' (43A258h) -------------

  The money_base is empty class with default ctor and I think we could
remove explicit invoking
of the money_base() from the moneypunct ctor initialization list.

  I have not verified, but I suppose that the same problem might be with
messages class.

  The patch is below:

  ChangeLog:
  * _messages.h (messages): Removed explicit invoking of the
messages_base() ctor
  to avoid buffer overrun due to bad code generation on MSVC 7.1.
  * _moneypunct.h (moneypunct): Removed explicit invoking of the
money_base() ctor
  to avoid buffer overrun due to bad code generation on MSVC 7.1.

-------------
Index: loc/_messages.h
===================================================================
--- loc/_messages.h     (revision 575203)
+++ loc/_messages.h     (working copy)
@@ -82,7 +82,7 @@
                          allocator<char_type> > string_type;

_EXPLICIT messages (_RWSTD_SIZE_T __refs = 0)

-        : _RW::__rw_facet (__refs), messages_base () { }
+        : _RW::__rw_facet (__refs) { }

catalog open (const string& __fun, const locale& __loc) const {

Index: loc/_moneypunct.h
===================================================================
--- loc/_moneypunct.h   (revision 575203)
+++ loc/_moneypunct.h   (working copy)
@@ -66,7 +66,7 @@
     string_type;

_EXPLICIT moneypunct (_RWSTD_SIZE_T __refs = 0)

-        : _RW::__rw_facet (__refs), money_base () { }
+        : _RW::__rw_facet (__refs) { }

char_type decimal_point () const {

         return do_decimal_point ();
-------------

Farid.