Re: [Mingw-w64-public] Possible bug in 64-bit std::thread-enabled 4.7.0 build

[K. F. <kfr...@gm...>]

Hi Kai!

Thanks for your response.

I don't follow what you are saying -- please see my comments at
the bottom.

On Sat, Nov 5, 2011 at 4:29 AM, Kai Tietz <kti...@go...> wrote:
> 2011/11/5 K. Frank <kfr...@gm...>:
>> Hello List (and Ruben)!
>>
>> I think I have come across a bug in Ruben's 64-bit 4.7.0 build, otherwise
>> known as "The Mighty Ruben's Wacky Build" ("TMRWB") (TM).
>> ...
>> The bug is triggered by logically incorrect, but I believe well-defined code.
>> I will explain as well as I can.
>> ...
>> Here is the code for the test program:
>>
>> ====================
>>
>> compiled as:  g++ -g -o stdout_stream_error stdout_stream_error.cpp
>>
>> ====================
>>
>> #include <iostream>
>> #include <streambuf>
>> #include <string>
>>
>> #include <cstdio>
>>
>> class StdoutStream : public std::basic_streambuf<char>
>> {
>> public:
>>  StdoutStream(std::ostream &stream) : m_stream (stream) {
>>    m_old_buf = stream.rdbuf();
>>    stream.rdbuf (this);
>>  }
>>  ~StdoutStream() {
>>    // output anything that is left
>>    if (!m_string.empty())  printf ("%s", m_string.c_str());
>>    m_stream.rdbuf (m_old_buf);
>>  }
>>
>> protected:
>>  virtual int_type overflow(int_type v) {
>>    if (v == '\n') {
>>      printf ("%s", m_string.c_str());
>>      m_string.erase (m_string.begin(), m_string.end());
>>    }
>>    else  m_string += v;
>>    return v;
>>  }
>>

*****  The only use of std::streamsize is here  *****
*****  (and in the return statement).                *****

>>  virtual std::streamsize xsputn (const char *p, std::streamsize n) {
>>    m_string.append (p, p + n);
>>    unsigned pos = 0;
>>    // size_t pos = 0;  // this is the correct declaration
>>    while (pos != std::string::npos) {  // 32-bit unsigned compared
>> against 64-bit size_t
>>    // while (pos != ((unsigned) -1)) {  // this works
>>      printf ("xsputn: top of loop, pos = %d\n", pos);
>>      pos = m_string.find ('\n');
>>      if (pos != std::string::npos) {  // 32-bit unsigned compared
>> against 64-bit size_t
>>      // if (pos != ((unsigned) -1)) {  // this works
>>        std::string tmp(m_string.begin(), m_string.begin() + pos + 1);
>>        printf ("%s", tmp.c_str());
>>        m_string.erase (m_string.begin(), m_string.begin() + pos + 1);
>>      }
>>      printf ("xsputn: bottom of loop, pos = %d\n", pos);
>>    }
>>    printf ("xsputn: before return, n = %d, pos = %d\n", n, pos);
>>    return n;
>>  }
>>
>> private:
>>  std::ostream &m_stream;
>>  std::streambuf *m_old_buf;
>>  std::string m_string;
>> };
>>
>> int main (int argc, char *argv[]) {
>>
>>  printf ("hello...\n");
>>
>>  StdoutStream *out = new StdoutStream (std::cout);
>>
>>  if (out) {  // to avoid unused variable warning
>>    // prints neither message to text1
>>    // std::cout << "Message 1 (with endl)..." << std::endl;
>>    // std::cout << "Message 2 (with endl)..." << std::endl;
>>
>>    // prints only "Message 1" to text1
>>    try {
>>      std::cout << "Message 1 (with '\\n')...\n";
>>      std::cout << "Message 2 (with '\\n')...\n";
>>    }
>>    catch (...) {  // no exception thrown -- not the ptoblem
>>      printf ("exception caught...\n");
>>    }
>>
>>    // prints both messages to text1
>>    // std::cout << "Message 1 (with two '\\n's)...\nMessage 2 (with
>> two '\\n's)...\n";
>>  }
>>
>>  printf ("goodbye!\n");
>>
>>  return 0;
>> }
>>
>> ====================
>>
>> The code is, I believe, logically incorrect in the member function xsputn()
>> in that the local variable "pos" should be declared as
>>
>>   size_t pos;
>>
>> rather than
>>
>>   unsigned pos;
>>
>> This is because (under the 64-bit compiler) unsigned is 32 bits, while
>> size_t is 64 bits.
>>
>> However, I would expect this error to cause the loop never to terminate.
>> Instead, the function exits, but according to gdb, not through its return
>> statement!
>>
>> In fact, it seems to me that according to gdb, the return statement has
>> been somehow optimized away.  (This is just speculation on my part.)
>> ...
>> I can also explain why I think this is a real bug, and not incorrect code
>> invoking "undefined behavior."

A quick comment on this.  Kai raises the issue of signed / unsigned
conversions (because std::streamsize is signed).  I've raised the issue
of narrower (32-bit) / wider (64-bit) unsigned conversions.  As I explain
below, I don't think std::streamsize enters into this, but even if it did, I
still believe that the compiler is not behaving properly.

As I read the standard, neither the signed / unsigned nor the narrower /
wider conversions invoke undefined behavior.  Narrower / wider integral
are, I believe, well specified by the standard.  I believe that signed /
unsigned integral conversions can be implementation-defined (based
on how the implementation / hardware represents negative integers,
and how it implements sign extension), but not undefined.

No matter how the conversions are implemented, as I look at the code,
I think that the behavior should be well defined and the function should
either enter an infinite loop or run correctly.  Exiting the loop apparently
early (no matter what the conversions are) seems to me to be a compiler
bug.

>> ...
>> Thanks for anyone's thoughts.
>>
>> K. Frank
>
> Hmm, this looks to me indeed as logical wrong code, but by different
> reason as you are assuming.  The underlying issue here is that
> streamsize is derived from ptrdiff_t, which is a signed type.  But you
> are using here an 'unsigned'.
> The expanding rule from unsigned to signed with higher precision
> remains unsigned valued.  For 32-bit we have here for both types same
> precision, and therefore indeed the sign check is done as intended.

Here's what I don't understand about your comment:  In the code above,
std::streamsize is used explicitly in two places:

    virtual std::streamsize xsputn (const char *p, std::streamsize n) {

as the return type of xsputn, and as the type of the argument n.  It is
also used implicitly, if you will, in the return statement of xsputn:

       return n;

But n is never modified in the code, and is only used, as I understand it,
in a fully legal pointer-arithmetic expression:

   m_string.append (p, p + n);

(p is a char*).

So I just don't see any signed / unsigned mismatch between std::streamsize
and any other integral type.

> If you are using here instead of 'unsigned' a 'signed' n, everything
> works as expected.

I don't follow what the concrete change to the code would be.  I'm happy
to make such a change and see if it fixes the problem, but I don't see
what specific change you are proposing.

If you mean changing the type of the argument n to be unsigned:

    virtual std::streamsize xsputn (const char *p, unsigned n) {

I don't think that I would be allowed to do that because the signature of
xsputn is defined by the iostream library (and hence by the standard).

(Of course, when I say I think there is an error in the compiler, I mean
the whole compiler / library package.  The compiler could be okay, but
there could be an error in the library code.  To clarify, I do believe that
the compiler is generating incorrect code, but if a coding error in the
library were invoking undefined behavior, the compiler would technically
be within its rights to do so.)

> Regards,
> Kai

Further thoughts would be greatly appreciated.


K. Frank