[8u] RFR 8231430: C2: Memory stomp in max_array_length() for T_ILLEGAL type

[Andrew Hughes]

On 20/02/2020 09:01, Aleksey Shipilev wrote:
> On 2/20/20 4:47 AM, Andrew Hughes wrote:
>> Given I don't see us backporting ConstantDynamic, could we not include
>> that small change to globalDefinitions.hpp in this fix, rather than
>> transposing its body into the if block in src/share/vm/opto/type.cpp?
> 
> Well, other backports we did substituted is_reference_type call with the explicit body. The rest of
> HS code uses the same pattern (before JDK-8230505), so it is not like we are doing something out of
> whack. The only valid reason I would see to import is_reference_type here is in case we would
> backport JDK-8230505 at some point later, which seems very unlikely.
> 
> We can still do that, if you insist:
>   https://cr.openjdk.java.net/~shade/8231430/webrev.8u.02/
> 

I'm afraid I don't follow your logic. If other backports have had to
make a similar substitution, surely that makes the case for just
backporting this little inline function to avoid doing it yet again in
future? I don't recall reviewing such backports. I'd probably have said
much the same about those.

Backporting is tedious enough as it is. I don't see the merit in
introducing a difference between the 8u & 11u versions of this fix, when
it can be easily resolved with a one-line function. It just creates work
for someone who encounters this difference later, and then has to work
around it, looking up the definition of is_reference_type in 11u, just
to find it matches what has been substituted in 8u.

Anyway, I'm happy with the revised webrev.

Thanks,
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222