[lworld] RFR: 8279428: [lworld] Revalue ACC_PRIMITIVE to be 0x800 to align with draft JVMS

[Aggelos Biboudis]

On Thu, 6 Jan 2022 16:14:15 GMT, Srikanth Adayapalam <sadayapalam at openjdk.org> wrote:

>> src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/ClassWriter.java line 1551:
>> 
>>> 1549:             flags = adjustFlags(c.flags() & ~DEFAULT);
>>> 1550:             if ((flags & PROTECTED) != 0) flags |= PUBLIC;
>>> 1551:             flags = flags & (ClassFlags | ACC_PRIMITIVE) & ~STRICTFP;
>> 
>> As far as I can understand, `ACC_PRIMITIVE` was removed from here because it is driven by the relevant if-clause in relation to `PRIMITIVE_CLASS` in `adjustFlags` which makes the code clearer at this point. Is my assumption correct?
>
> Good question! Thanks for asking. 
> 
> Indeed there is a "problem" here - actually two problems, one old and one new. I put problem in quotes because, the bug is only latent at the moment and does not impact anything. but is nevertheless a bug and should be fixed.
> 
> Explanation. First the old bug:
> 
> In Flags.java we have the following:
> 
> 
> LocalClassFlags                   = FINAL | ABSTRACT | STRICTFP | ENUM | SYNTHETIC  | PRIMITIVE_CLASS,
> ClassFlags                        = LocalClassFlags | INTERFACE | PUBLIC | ANNOTATION,
> ``` 
> 
> This is problematic because PRIMITIVE_CLASS is defined to be 1 << 16 - while the class file representation of flags is only 16 bits. Which is why there is a mapping from PRIMITIVE_CLASS to ACC_PRIMITIVE in the first place.
> 
> So the definition of LocalClassFlags should have had the bit corresponding to 0x100 ie  (1 << 8) set by including a suitable symbolic constant in place of PRIMITIVE_CLASS.
> 
> The idea behind doing
> 
> flags = flags & ClassFlags
> 
> 
> in the old (and new code) is ensure that no bit other than well defined class flags bits are set in the flags being written out to file. By not having 0x100 (old ACC_PRIMITIVE) set in ClassFlags we are inadvertently masking ACC_PRIMITIVE bit and to avoid running into problems there, we or back ACC_PRIMITIVE into the ClassFlags just being &ing.
> 
> New problem:
> 
> In the new code, again LocalClassFlags is missing ACC_PRIMITIVE, but things work just fine but by accident - only because ACC_PRIMITIVE shares the same value with ACC_STRICT !
> 
> I'll post a revision that will make this clear and address the latent bug (which may become real bug if ACC_PRIMITIVE changes value again to have a value that is not already a known bit pattern in LocalClassFlags) 
> 
> The right

Thank you for the detailed explanation.

> In the new code, again LocalClassFlags is missing ACC_PRIMITIVE, but things work just fine but by accident - only because ACC_PRIMITIVE shares the same value with ACC_STRICT !

I am still a bit confused because I don't see any uses of ACC_STRICT but only STRICT_FP at that level (which is indeed 1<<11, 0x0800). So in my mind it shares the same value with STRICT_FP not ACC_STRICT. I will fully get it with your upcoming revision 👍 💯 

Thanks again for the explanation!

-------------

PR: https://git.openjdk.java.net/valhalla/pull/593