RFR: 8079093: Remove FakeRttiSupport workaround for gcc -Wtypes-limit
Kim Barrett
kim.barrett at oracle.com
Wed Jun 3 18:41:20 UTC 2015
On Jun 3, 2015, at 12:59 PM, Volker Simonis <volker.simonis at gmail.com> wrote:
>
> On Wed, Jun 3, 2015 at 6:26 PM, Kim Barrett <kim.barrett at oracle.com> wrote:
>> On Jun 3, 2015, at 9:34 AM, Volker Simonis <volker.simonis at gmail.com> wrote:
>>>
>>> Hi Kim,
>>>
>>> as far as I can see, TagType is a template parameter. What if this
>>> parameter is of type unsigned?
>>>
>>> Wouldn't we get a warning that the comparison '0 <= tag' is always
>>> true? So why not cast the tag in the comparison as well '0 <=
>>> (intx)tag’.
>>
>> Remember when we were discussing the addition of -Wtype-limits, I said
>> I had some code that would be tripped up by it due to a gcc bug fixed
>> in 4.8? This is that code.
>>
>> But even with the restriction of that warning flag to gcc4.8 and
>> later, at the time I still needed the workaround. This was because
>> the the Mac build tools we were using included gcc4.2, which
>> unconditionally turned on the buggy version of -Wtype-limits checking,
>> with no way to turn it off (-W[no-]type-limits didn't show up until
>> gcc4.3). This is no longer a problem, now that we've updated the
>> minimum supported build tools for the Mac, so the workaround can be
>> removed.
>>
>>> You also changed 'tag < (sizeof(uintx) * BitsPerByte' to 'tag <
>>> BitsPerWord' but I don't think that is safe because it is not
>>> guaranteed that sizeof(TagType)==BitsPerWord. Currently
>>> FakeRttiSupport is only instantiated with TagType=Name where Name is
>>> an enum but that enum can very well be less than 32-bit wide as the
>>> C++ standard only requires that en enum is beeing backed up by an
>>> underlying integral type which is big enough to hold all the enum
>>> values. So I'd suggest to better keep the old assertion.
>>
>> I don't understand this comment.
>>
>> The change is to replace the expression
>>
>> (sizeof(uintx) * BitsPerByte)
>>
>> with the equivalent value'd (but simpler and more descriptive)
>> expression
>>
>> BitsPerWord
>>
>> The purpose of validate_tag is to ensure the tag value is in the range
>> [0, BitsPerWord) so that the shift in tag_bit is safe.
>
> I thought you want to check that the tag value is in the range [0,
> sizeof(TagType)*BitsPerByte) which is a stronger check because
> sizeof(TagType)*BitsPerByte can be smaller than BitsPerWord. If that's
> not the case your change is fine.
>
> Thank you and best regards,
> Volker
Thanks.
>
>>
>> Nothing here cares about the size of TagType, only about the runtime
>> value of the tag. It might be that the set of possible values for a
>> given instantiation of TagType guarantee (some of) the range checks
>> will pass, but that's not a problem, so long as the compiler doesn't
>> inappropriately complain.
>>
>>> Regards,
>>> Volker
>>>
>>>
>>> On Thu, Apr 30, 2015 at 7:14 AM, Kim Barrett <kim.barrett at oracle.com> wrote:
>>>> Please review this removal of a workaround to avoid spurious compiler
>>>> warnings when building on MacOSX.
>>>>
>>>> With the move to a newer minimum compiler version for MacOSX, the
>>>> workaround is no longer needed.
>>>>
>>>> CR:
>>>> https://bugs.openjdk.java.net/browse/JDK-8079093
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~kbarrett/8079093/webrev/
>>>>
>>>> Testing:
>>>> JPRT
More information about the hotspot-gc-dev
mailing list