RE: [jira] [Updated] (STDCXX-1058) std::basic_ios<>::copyfmt() with registered callback (via std::ios_base::register_callback()) run-time SIGABRT

[Travis Vitek-4]

I approve the change, but with one caveat. The branching policy [1] indicates that you should commit your changes directly to the 4.2.x branch. They should be merged from 4.2.x to 4.3.x, and then from 4.3.x to trunk.

It has been a long time since I've worked actively on the stdcxx project, but it seems that if seeking feedback on a patch before committing you would send something to the list. It seems that trivial changes were committed to the appropriate branch without prior review. Unfortunately, the committers page [2] indicates that my memory is wrong.

Travis

[1] http://wiki.apache.org/stdcxx/Branching
[2] http://stdcxx.apache.org/index.html#committers


Index: 4.2.x/src/iostore.cpp
===================================================================
--- 4.2.x/src/iostore.cpp (revision 1055501)
+++ 4.2.x/src/iostore.cpp (working copy)
@@ -335,8 +335,14 @@
             // delete existing arrays, if any; _C_usr will only be deleted
             // if `rhs' contains no user data (see below)
             operator delete (_C_usr->_C_iarray);
+            _C_usr->_C_iarray = 0;
+            _C_usr->_C_isize = 0;
             operator delete (_C_usr->_C_parray);
+            _C_usr->_C_parray = 0;
+            _C_usr->_C_psize = 0;
             operator delete (_C_usr->_C_cbarray);
+            _C_usr->_C_cbarray = 0;
+            _C_usr->_C_cbsize = 0;
         }
         else if (ia || pa || cba || ptie) {
             // allocation may throw

[Martin Sebor-2]

On 05/16/2012 11:55 AM, Travis Vitek wrote:
> I approve the change, but with one caveat. The branching policy [1] indicates that you should commit your changes directly to the 4.2.x branch. They should be merged from 4.2.x to 4.3.x, and then from 4.3.x to trunk.

The test in the issue should be committed with the patch (after
adding the necessary asserts, being renamed to follow the naming
convention for regression tests, i.e., something like
regress/27.basic_ios.stdcxx-1058.cpp, and decorated with the ASF
license header).

>
> It has been a long time since I've worked actively on the stdcxx project, but it seems that if seeking feedback on a patch before committing you would send something to the list. It seems that trivial changes were committed to the appropriate branch without prior review. Unfortunately, the committers page [2] indicates that my memory is wrong.

I think the two are subtly inconsistent. At one point, I think we
wanted to commit to trunk first and then merge to branches. That
seems to be the safer way to work. I'm not sure why we set up the
branching policy to commit to the most restrictive branch first.
It might be in the archives. Or we can change it.

Other than that, the stdcxx index page is out of date. It doesn't
reflect that Stefan is both a committer and a PMC member, and it
still lists me as PMC chair. I viewed it as part of my job to
update the page when I chaired the project. But I wouldn't expect
the current chair to do it. So if we want to keep the site up to
date we'll need to decide who should update these things (anyone
with commit permissions can).

Martin

>
> Travis
>
> [1] http://wiki.apache.org/stdcxx/Branching
> [2] http://stdcxx.apache.org/index.html#committers
>
>
> Index: 4.2.x/src/iostore.cpp
> ===================================================================
> --- 4.2.x/src/iostore.cpp (revision 1055501)
> +++ 4.2.x/src/iostore.cpp (working copy)
> @@ -335,8 +335,14 @@
>               // delete existing arrays, if any; _C_usr will only be deleted
>               // if `rhs' contains no user data (see below)
>               operator delete (_C_usr->_C_iarray);
> +            _C_usr->_C_iarray = 0;
> +            _C_usr->_C_isize = 0;
>               operator delete (_C_usr->_C_parray);
> +            _C_usr->_C_parray = 0;
> +            _C_usr->_C_psize = 0;
>               operator delete (_C_usr->_C_cbarray);
> +            _C_usr->_C_cbarray = 0;
> +            _C_usr->_C_cbsize = 0;
>           }
>           else if (ia || pa || cba || ptie) {
>               // allocation may throw
>

[Stefan Teleman-2]

On Wed, May 16, 2012 at 2:58 PM, Martin Sebor <msebor@...> wrote:

> On 05/16/2012 11:55 AM, Travis Vitek wrote:
>>
>> I approve the change, but with one caveat. The branching policy [1]
>> indicates that you should commit your changes directly to the 4.2.x branch.
>> They should be merged from 4.2.x to 4.3.x, and then from 4.3.x to trunk.
>
>
> The test in the issue should be committed with the patch (after
> adding the necessary asserts, being renamed to follow the naming
> convention for regression tests, i.e., something like
> regress/27.basic_ios.stdcxx-1058.cpp, and decorated with the ASF
> license header).

I attached 27.ios_base.event_callback.stdcxx-1058.cpp to stdcxx-1058,
which will go in ${TOPDIR}/test/regress/.

--Stefan

--
Stefan Teleman
KDE e.V.
stefan.teleman@...

[Martin Sebor-2]

On 05/16/2012 09:23 PM, Stefan Teleman wrote:

> On Wed, May 16, 2012 at 2:58 PM, Martin Sebor<msebor@...>  wrote:
>> On 05/16/2012 11:55 AM, Travis Vitek wrote:
>>>
>>> I approve the change, but with one caveat. The branching policy [1]
>>> indicates that you should commit your changes directly to the 4.2.x branch.
>>> They should be merged from 4.2.x to 4.3.x, and then from 4.3.x to trunk.
>>
>>
>> The test in the issue should be committed with the patch (after
>> adding the necessary asserts, being renamed to follow the naming
>> convention for regression tests, i.e., something like
>> regress/27.basic_ios.stdcxx-1058.cpp, and decorated with the ASF
>> license header).
>
> I attached 27.ios_base.event_callback.stdcxx-1058.cpp to stdcxx-1058,
> which will go in ${TOPDIR}/test/regress/.

Great! A few suggestions:

The naming convention used for the tests is based on a few things:

1. the section number in the standard of the tested functionality
    (this is only used for sorting)
2. the name of class whose members are being tested (if any)
3. the name of the function or type (or member function or type)
    being tested (may be omitted for tests that exercise multiple
    members)
4. for regression tests, the issue number

Since this issue is about std::basic_ios::copyfmt() crashing (as
the subject says), its name should probably look something like:

   27.basic_ios.copyfmt.stdcxx-1056.cpp

Each test should exercise just the affected class, function, or
type. It's best to avoid relying on parts of the library that
aren't affected or subject of the test so that when they break
as a result of a some unrelated change in the future we don't
start seeing failures in unrelated tests. In this case, I would
suggest to avoid using fstream (and especially cerr) and rely
directly on basic_ios (define a derived class to access its
protected ctor). Most regression tests don't tend to produce
debugging output but if you find it useful please use stdio,
not iostreams.

Finally, regression tests should verify expected postconditions
by using the assert() macro. The exit status can be 0 on success
and non-zero on failure, but this is not done consistently.

Martin

>
> --Stefan
>

[Stefan Teleman-2]

On Thu, May 17, 2012 at 11:58 AM, Martin Sebor <msebor@...> wrote:

> On 05/16/2012 09:23 PM, Stefan Teleman wrote:
>>
>> On Wed, May 16, 2012 at 2:58 PM, Martin Sebor<msebor@...>  wrote:
>>>
>>> On 05/16/2012 11:55 AM, Travis Vitek wrote:
>>>>
>>>>
>>>> I approve the change, but with one caveat. The branching policy [1]
>>>> indicates that you should commit your changes directly to the 4.2.x
>>>> branch.
>>>> They should be merged from 4.2.x to 4.3.x, and then from 4.3.x to trunk.
>>>
>>>
>>>
>>> The test in the issue should be committed with the patch (after
>>> adding the necessary asserts, being renamed to follow the naming
>>> convention for regression tests, i.e., something like
>>> regress/27.basic_ios.stdcxx-1058.cpp, and decorated with the ASF
>>> license header).
>>
>>
>> I attached 27.ios_base.event_callback.stdcxx-1058.cpp to stdcxx-1058,
>> which will go in ${TOPDIR}/test/regress/.
>
>
> Great! A few suggestions:
>
> The naming convention used for the tests is based on a few things:
>
> 1. the section number in the standard of the tested functionality
>   (this is only used for sorting)
> 2. the name of class whose members are being tested (if any)
> 3. the name of the function or type (or member function or type)
>   being tested (may be omitted for tests that exercise multiple
>   members)
> 4. for regression tests, the issue number
>
> Since this issue is about std::basic_ios::copyfmt() crashing (as
> the subject says), its name should probably look something like:
>
>  27.basic_ios.copyfmt.stdcxx-1056.cpp
>
> Each test should exercise just the affected class, function, or
> type. It's best to avoid relying on parts of the library that
> aren't affected or subject of the test so that when they break
> as a result of a some unrelated change in the future we don't
> start seeing failures in unrelated tests. In this case, I would
> suggest to avoid using fstream (and especially cerr) and rely
> directly on basic_ios (define a derived class to access its
> protected ctor). Most regression tests don't tend to produce
> debugging output but if you find it useful please use stdio,
> not iostreams.
>
> Finally, regression tests should verify expected postconditions
> by using the assert() macro. The exit status can be 0 on success
> and non-zero on failure, but this is not done consistently.

OK I attached a new test case - 27.basic_ios.copyfmt.stdcxx-1058.cpp.

But, using a simple derived class from std::basic_ios doesn't trigger
the bug. It's only triggered when using std::fstream or
std::stringstream.

--Stefan

--
Stefan Teleman
KDE e.V.
stefan.teleman@...

[Martin Sebor-2]

...
> OK I attached a new test case - 27.basic_ios.copyfmt.stdcxx-1058.cpp.
>
> But, using a simple derived class from std::basic_ios doesn't trigger
> the bug. It's only triggered when using std::fstream or
> std::stringstream.

Here's what I meant:

   struct A: std::streambuf { };
   struct B: std::ios {
     A sb;
     B () { init (&sb); }
   } f0, f1;

Btw., in your new test, either the TEST_ASSERT() macro should abort
or the test should when before returning ret is non-zero.

I.e., every regression test should report failure by calling abort
(via the assert() macro).

Returning a non-zero exit status (in addition to making use of
assert()) is fine but it shouldn't be the sole mechanism for
reporting an error.

Also, please avoid #including <iostream> in tests unless
exercising the standard streams (std::cout et al.) The header
runs complicated code and can lead to unrelated failures.

Attached is a slightly modified test to show what I mean. (Also
fixes formatting -- please use 4 space indents and a space before
each open paren; curly brace goes on the same line as the statement
except for namespace scope declarations).

Martin

>
> --Stefan
>


[27.ios_base.event_callback.stdcxx-1058.cpp]

---

/**************************************************************************
 *
 * 27.ios_base.event_callback.stdcxx-1058.cpp - regression test for STDCXX-1058
 *
 * http://issues.apache.org/jira/browse/STDCXX-1058
 *
 * $Id$
 *
 **************************************************************************
 *
 * Licensed to the Apache Software  Foundation (ASF) under one or more
 * contributor  license agreements.  See  the NOTICE  file distributed
 * with  this  work  for  additional information  regarding  copyright
 * ownership.   The ASF  licenses this  file to  you under  the Apache
 * License, Version  2.0 (the  "License"); you may  not use  this file
 * except in  compliance with the License.   You may obtain  a copy of
 * the License at
 *
 * http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the  License is distributed on an  "AS IS" BASIS,
 * WITHOUT  WARRANTIES OR CONDITIONS  OF ANY  KIND, either  express or
 * implied.   See  the License  for  the  specific language  governing
 * permissions and limitations under the License.
 *
 **************************************************************************/
/*
 * This program should run to completion and exit with return status 0
 */

#include <cassert>
#include <ios>
#include <cstdio>

#define TEST_ASSERT(expr)                                               \
    (expr) ? ret = 0                                                    \
        : std::fprintf (stderr,"line %i: Assertion failed: %s\n",       \
                        __LINE__, #expr)

int x;

void testfun (std::ios_base::event ev, std::ios_base& iosobj, int index)
{
    x = index;

    switch (ev) {
    case std::ios_base::copyfmt_event:
        std::fprintf (stderr, "copyfmt_event\n");
        break;
    case std::ios_base::imbue_event:
        std::fprintf (stderr, "imbue_event\n");
        break;
    case std::ios_base::erase_event:
        std::fprintf (stderr, "erase_event\n");
        break;
    default:
       std::fprintf (stderr, "unknown (this should never happen)\n");
       break;
    }
}


int main ()
{
    struct A: std::streambuf { };
    struct B: std::ios {
        A sb;
        B () { init (&sb); }
    } f0, f1;

    int i = 101;
    int ret = 1;

    std::ios_base::event_callback e1 = &testfun;
    f0.register_callback (e1, i);

    x = 0;
    f0.imbue (f0.getloc ());
    TEST_ASSERT (x == i);

    x = 0;
    f0.copyfmt (f1);
    TEST_ASSERT (x == i);

    B s0, s1;

    s0.register_callback (e1, i);

    x = 0;
    s0.imbue (f0.getloc ());
    TEST_ASSERT (x == i);

    x = 0;
    s0.copyfmt (s1);
    TEST_ASSERT (x == i);

    assert (ret == 0);

    return ret;
}

---

[Stefan Teleman-2]

On Mon, Jun 11, 2012 at 12:35 PM, Martin Sebor <msebor@...> wrote:

> ...
>
>> OK I attached a new test case - 27.basic_ios.copyfmt.stdcxx-1058.cpp.
>>
>> But, using a simple derived class from std::basic_ios doesn't trigger
>> the bug. It's only triggered when using std::fstream or
>> std::stringstream.
>
>
> Here's what I meant:
>
>  struct A: std::streambuf { };
>  struct B: std::ios {
>    A sb;
>    B () { init (&sb); }
>  } f0, f1;
>
> Btw., in your new test, either the TEST_ASSERT() macro should abort
> or the test should when before returning ret is non-zero.
>
> I.e., every regression test should report failure by calling abort
> (via the assert() macro).
>
> Returning a non-zero exit status (in addition to making use of
> assert()) is fine but it shouldn't be the sole mechanism for
> reporting an error.
>
> Also, please avoid #including <iostream> in tests unless
> exercising the standard streams (std::cout et al.) The header
> runs complicated code and can lead to unrelated failures.
>
> Attached is a slightly modified test to show what I mean. (Also
> fixes formatting -- please use 4 space indents and a space before
> each open paren; curly brace goes on the same line as the statement
> except for namespace scope declarations).
>
> Martin

Done - new attachment 27.basic_ios.copyfmt.stdcxx-1058.cpp

--Stefan

--
Stefan Teleman
KDE e.V.
stefan.teleman@...