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

[Dmitry Chuyko]

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.
>
>> I don't think we've been backporting all of the aarch64 back end work to 11u, just some of it, and we've (well, I've, so far) done the equivalent x64 work where needed too.
> So what is the threshold for a backport? I'd think it should be
> a significant user-visible bug (or performance enhancement.)
> There are other possible criteria, such as misleading and error-
> prone code, which this perhaps qualifies for. It's still almost
> a thousand lines of patch, though.
>
> 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.

-Dmitry