Re: [jira] Commented: (STDCXX-761) [HP aCC 6.16] Out of bound access in new.cpp

[Martin Sebor]

Martin Sebor wrote:

Travis Vitek wrote:

Martin Sebor wrote:

Eric Lemings wrote:

Travis,

If you could, give the following patch a whirl (or quick review
at least).

Is there an easier way to silence the warning than by adding
all these loops? E.g., by assigning the address of the first
member to a local pointer? (Or is the compiler too smart for
that?)

That is exactly what the previous code did...

    size_t*       diffs    = diff.new_calls_;
    const size_t* st0_args = st0->new_calls_;
    const size_t* st1_args = st1->new_calls_;

The compiler complains because the member new_calls_ is an array of
length 2, but we iterate over 16 elements. Provided that there is no
padding between each of the array fields, this would work and we'd just
have to disable the warning.

Wouldn't that be the way to go then? I.e., wrap the loop
in a couple of #pragma diag_suppress/diag_default?

On second thought, I have to agree that the code is tricky and brittle.
If someone were to change the order of the rwt_free_store members or
add a new one in between the loop would silently break. So maybe the
right solution is as Brad proposed...

Btw., C99 added this _Pragma() operator.

[...]

I don't really like the macro, probably because it uses local variables
without taking them as parameters. Another option would be to use offset
pointer to member and a nested loop.

    typedef size_t (rwt_free_store::* checkpoint_field)[2];
    static const checkpoint_field fields [] =
    {

...or this, except, of course, for the braces which in local/class
scope go on the line above ;-)

You guys decide, either way is fine with me.

Btw., if you go with Brad's approach, there's no need for EXTENT().
We already have an RW_COUNTOF() macro in <testdefs.h>.

Oh, and another last thing: new.cpp uses a naming convention that's
inconsistent with the rest of the driver where non-member functions
with internal linkage (i.e., namespace-scope statics) start with
the _rw_ prefix, and extern functions with rw_. At some point we
need to change the externs but any new statics should use the
_rw_ convention.

Martin

        &rwt_free_store::new_calls_,
        &rwt_free_store::delete_calls_,
        &rwt_free_store::delete_0_calls_,
        &rwt_free_store::blocks_,
        &rwt_free_store::bytes_,
        &rwt_free_store::max_blocks_,
        &rwt_free_store::max_bytes_,
        &rwt_free_store::max_block_size_
    };

    static rwt_free_store diff;

    bool diff_0 = true;

    for (size_t f = 0; f < sizeof (fields) / sizeof (*fields); ++f)
    {
        size_t*       diffs    = (diff.*fields [f]);
        const size_t* st0_args = (st1->*fields [f]);
        const size_t* st1_args = (st0->*fields [f]);

        for (size_t i = 0; i < 2; ++i)
        {
          diffs [i] = st1_args [i] - st0_args [i];
          if (diffs [i])
            diff_0 = false;
        }
    }

Also, there is no need to use the _RWSTD_SIZE_T macro in .cpp
files. The macro is useful in library and test suite headers
to reduce namespace pollution (so we don't have to #include
<stddef.h>.

I was going to mention that.

Last thing: the code formatting convention calls for a space
before each open parenthesis. It might take some getting used
to but once you do, you'll never go back -- just ask Travis ;-)

Uh, yeah.

Travis