[11u] RFR 8206895: aarch64: rework error-prone cmp instuction

[Andrew Dinn]

On 24/03/2021 14:15, Dmitry Chuyko wrote:
> Hi,
> 
> On 3/24/21 1:38 PM, Andrew Haley wrote:
>> .On 3/23/21 6:29 PM, Hohensee, Paul wrote:
>>> I'd say this patch is worth doing on its own, since it catches actual 
>>> argument size mismatches that would otherwise be silently accepted. 
>>> Doesn't seem super intrusive either: all it does is add formal 
>>> argument casts to "u1", and replace some cmp instructions with subs. 
>>> Effectively syntax-only changes, so low risk.
>> Hmm, interesting. But it's *not* syntax only, as I mention later.
>>
>> OK, but it's a lot of churn in an old maintenance release,
>> especially where there's no actual bug. Isn't churn a bad thing
>> of itself?
> I listed few issues that were already backported touching same code 
> areas. And I suppose the chance of getting more of them is high right 
> because this is a distributed change for a widely used unstruction. 
> There is no situation in currently backported code where we would rely 
> on non-backported argument checks though but it is an annoying thing to 
> track.

And in almost all of these cases sorting out the patch in the absence of 
this backport is pretty trivial. In a few cases it is not so trivial but 
those are *exactly* the cases where backporting this patch is also 
taking a risk.

So, this patch is not actually reducing risk. It is just importing that 
risk into the backports repo before we need to.

>> And this really is error-prone code:
>>
>> -    cmp(rscratch1, is_virtual ? 
>> DataLayout::virtual_call_type_data_tag : DataLayout::call_type_data_tag);
>> +    cmp(rscratch1, u1(is_virtual ? 
>> DataLayout::virtual_call_type_data_tag : 
>> DataLayout::call_type_data_tag));
>>
>> because it silently truncates its argument.
>>
>> It's what checked_cast was invented for,
> 
> Looks like something to fix in the upstream too.
Well, perhaps so. At which point we can then backport a worthwhile fix 
instead of reshuffling deck chairs like this patch does.

regards,


Andrew Dinn
-----------
Red Hat Distinguished Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill