RFR(M): 8068883: Remove disabling of warning "C4355: 'this' : used in base member initializer list"

Volker Simonis volker.simonis at gmail.com
Wed Jan 14 16:28:28 UTC 2015


Hi Kim,

thanks for your detailed comments. As I wrote, I'm not sure for myself
what's the best solution for this problem although I tend to see a
minor improvement in the proposed changes. Nevertheless, I won't be
upset if we decide to leave the code as it stands today as long as we
brought up and discussed all the different aspects of the problem.

Please find my further comments inline:


On Wed, Jan 14, 2015 at 1:26 AM, Kim Barrett <kim.barrett at oracle.com> wrote:
> On Jan 13, 2015, at 1:04 PM, Volker Simonis <volker.simonis at gmail.com> wrote:
>>
>> so here comes the change which re-enables the warning "C4355: 'this' :
>> used in base member initializer list".
>>
>> http://cr.openjdk.java.net/~simonis/webrevs/2015/8068883/
>> https://bugs.openjdk.java.net/browse/JDK-8068883
>>
>> This was previously discussed in the mail thread for "RFR(S): 8068739:
>> G1CollectorPolicy uses uninitialized field '_sigma' in the
>> constructor" (http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2015-January/011743.html).
>>
>> There are several places in the GC coding which disable the warning
>> Visual Studio warning "C4355: 'this' : used in base member initializer
>> list":
>>
>> […]
>>
>> However this warning is reasonable and we've recently found a bug
>> which was caused by the usage of the incompletely initialized this
>> pointer in the initializer list (see issue 8068739 -
>> https://bugs.openjdk.java.net/browse/JDK-8068739).
>>
>> Now that I've done this change I must confess that I'm not that
>> enthusiastic about it any more. […]
>>
>> Unfortunately, moving the corresponding field initializations from the
>> initializer list into the constructor body, required the creation of
>> several default constructors which is not that nice. On the other
>> side, I think the code got cleaner now and we are getting rid of the
>> pragmas which disabled the corresponding compiler warnings on Windows.
>> Notice that although the current implementation wasn't wrong, it still
>> handed over the this pointer from the initializer list several levels
>> of indirection down which makes it hard to detect from a first glance
>> if that usage is safe or not. But please judge yourself if it's worth
>> while doing this clean-up or not.
>
> Do we know whether relevant versions of the (Microsoft?) compiler
> actually issues the warning in question for the code under
> consideration? Looking at a couple of the cases under consideration
> here, I don't think warnings should be generated, based on the
> documentation for that warning.
>

Yes, the warning is still generated with VS 10. Just remove the pragma
from concurrentMark.cpp for example and you'll instantly get the
following build failure:

C:\Software\OpenJDK\jdk9-dev\hotspot\src\share\vm\gc_implementation\g1\concurrentMark.cpp(565)
: error C2220: warning treated as error - no 'object' file generated
C:\Software\OpenJDK\jdk9-dev\hotspot\src\share\vm\gc_implementation\g1\concurrentMark.cpp(565)
: warning C4355: 'this' : used in base member initializer list

> Of course, it might be that the warning documentation and
> implementation don't match, or I'm looking at the wrong documentation,
> or I'm not understanding it properly.  Here's what I'm looking at:
> http://msdn.microsoft.com/en-us/library/3c594ae3.aspx.
>

You're looking at the right place. For me it seems to be quite clear:
"The ... class member constructors are called before this constructor.
In effect, you've passed a pointer to an unconstructed object to
another constructor. If those other constructors access any members or
call member functions on this, the result will be undefined. You
should not use the this pointer until all construction has completed."

> In general, I don't like these sorts of changes. I think the code in
> question is (probably) valid, and that a heuristic of "use of
> <tt>this</tt> in <em>ctor-initializers</em> are invalid" is overly
> restrictive; the standard only says they are invalid before the base
> class initializers are complete.
>

I think that refers to the vtable. You won't be able to call any
virtual methods on "this" before the  base class initializers are
complete. But I think we can all agree on the fact that the access to
an uninitialized member trough the "this" pointer in the initializer
list is undefined. The canonical example for this is:

class Point {
  int x, y;
  public Point() : y(42), x(this.y) {}
};

There's no base class initializer involved here. Nevertheless, the
value of Point.x will be undefined, because the member initialization
happens in the declaration order of the class definition. So 'x' will
be initialized to the at that point in time undefined value of 'y'.
And that's because we are using 'this' before it was fully
initialized.

> I think problems like https://bugs.openjdk.java.net/browse/JDK-8068739
> can (and ought to) be detected via static analysis tools. (I know I've
> seen defect reports for this sort of thing from applying such tools to
> other code bases in previous jobs.) It is good that said bug was
> discovered and addressed (though I'm not entirely happy with the
> proposed fix, but didn't get around to commenting before it was
> approved and pushed). But that it wasn't already known would seem to
> me to indicate a defect in our use of static analysis tools, or in the
> static analysis tools we're using.
>

I agree that static analysis tools should be able to detect such
errors. But I'm not aware of any of these tools being run regularly on
every HotSpot check-in. Do you have such tools at Oracle?

The compiler can definitely not detect such errors because in the
general case it hands over the not fully initialized 'this' pointer to
a constructor of which the implementation is in a different
compilation unit. So a warning is the best he can do.

In the code we are speaking about, the "this" pointer is handed over
several levels deep and neither is it clear at the initial call site
(i.e. in the initializer list) if the called constructor/function uses
possibly uninitialized members trough "this" nor is it clear at the
actual use site (i.e. in the final constructor/function body) if it is
safe to use "this" for accessing members trough it.

While I also think that the code in question here is correct today,
I'd consider it at least hard to understand and error prone. If for
example somebody will access an uninitialized member through "this" in
one if the class member constructors, nobody would realize that.

> I think applying the kinds of transformations proposed here is not an
> improvement, particularly when it involves adding default constructors
> that leave an object in a "partially constructed" state that requires
> later (assignment-based) reconstruction or separate post-construction
> initialization. See, for example, the addition of a default
> CMMarkStack() constructor in order to allow ConcurrentMark::_markStack
> to be initialized in the constructor body after being (partially)
> constructed via default construction in the initializer list.
>

I agree that the proposed workaround isn't ideal either, but I think
it is safer.
Another possibility would be to use the previous initializations but
with "NULL" as argument instead of "thread". Of course that would
require the addition of corresponding setters to each class for
updating the corresponding fields later one. But that also seems kind
of overkill, right?

>
>> I've additionally also removed the Visual Studio compiler macro which
>> disable the warning "C4786:'identifier' : identifier was truncated to
>> 'number' characters in the debug information." because as far as I
>> could see, this was only relevant up to Visual Studio 6.0 and I hope
>> nobody will ever compile the HotSpot with that compiler any more(see
>> http://support.microsoft.com/kb/195386).
>
> This seems like a worthwhile change, independent of the other changes
> being proposed.  Could this be separated out.
>

Sure, if we decide not to push this patch we can still just remove
that pragma in a separate change.


Regards,
Volker


More information about the hotspot-gc-dev mailing list