[8u] RFR 8248901: Signed immediate support in .../share/assembler.hpp is broken.

[Andrew Hughes]

On 15:47 Fri 25 Jun     , Andrew Dinn wrote:
> On 25/06/2021 14:54, Andrew Hughes wrote:
> > Sorry if I was unclear here. I'm not suggesting backporting the whole
> > of 8223140. I'm suggesting to follow 11u in adding this hunk:
> > 
> > --- a/src/hotspot/share/utilities/debug.hpp	Sat Jul 04 08:18:17 2020 +0800
> > +++ b/src/hotspot/share/utilities/debug.hpp	Mon Jul 06 21:29:51 2020 +0200
> > @@ -67,6 +67,9 @@
> >   // For backward compatibility.
> >   #define assert(p, ...) vmassert(p, __VA_ARGS__)
> > +#define precond(p)   assert(p, "precond")
> > +#define postcond(p)  assert(p, "postcond")
> > +
> >   #ifndef ASSERT
> >   #define vmassert_status(p, status, msg)
> >   #else
> > 
> > See:
> > 
> > https://hg.openjdk.java.net/jdk-updates/jdk11u/rev/d21f0f95e6b4
> > 
> > This should achieve the same as replacing precond with assert, but also provide
> > precond for future backports.
> 
> I'm happy for that specific change to be included given that it was included
> in jdk11u. However, cherry-picking bits of one patch and placing them in
> another patch seems like an anti-pattern to me.
>

I agree. It's not something I'm a fan of either. However, in cases
like this, where it's a generic helper function that is only part of
the original patch because that's where someone first thought to use
it, I think it's legitimate to make the backport that first uses it be
the one that introduces it in 8u.

I agree with you that backporting a patch just for such a macro would
be unacceptable. The only other alternative is to replace the
offending code in the backport. The best path to choose pretty much
depends on the scale of the change in question. With a couple of
one-liner macros like this, I definitely think just including them in
the backport is the best long-term option.

> >  From a search of the code, it appears only is_simm26, min_simm, max_simm
> > and min_simm16 are actually safe to be completely removed in 8u.
> > 
> > is_simm5, is_simm11, is_simm12, is_simm13 and min_simm13 are indeed all
> > used by the SPARC assembler code, as the 11u review already identified and
> > fixed. This is clearly not the case in trunk, as the SPARC port has been
> > removed.
> 
> Oh, indeed, that's my error. I did look at the jdk11u change set but then I
> completely forgot we needed the SPARC changes when I reviewed Zhengyu's
> patch.
> 
> > However, I can't approve this, knowing it will break SPARC. So, I
> > suggest either those functions are left untouched or (preferably) we
> > follow 11u in localising them to the only consumer, the SPARC port.
> Yes, I think the patch should be updated to include the jdk11u additions to
> the SPARC assembler.
>

I likely would not have spotted it myself, were it not for me doing
what I always tend to do with backports and comparing it with the 11u
version.

I believe our role as maintainers is not to contradict the expert
domain knowledge of reviewers like yourself, but to complement it from
the perspective of 8u as a whole, looking at the stability of
including the patch and the backporting decisions in the context of
past and potential future backports.

It is with that mind in that I questioned why the 11u differences were
not mentioned in the original review. I guess the general lesson from
this is we should be very wary of any backports that remove functions,
because that can only be guaranteed safe in the original context and
needs to be reanalysed when applied to a different codebase.

Regards,
-- 
Andrew :)
Pronouns: he / him or they / them
Senior Free Java Software Engineer
OpenJDK Package Owner
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