RFR 8175383: JVM should throw NCDFE if ACC_MODULE and CONSTANT_Module/Package are set

[John Rose]

On Mar 1, 2017, at 1:20 PM, harold seigel <harold.seigel at oracle.com> wrote:
> 
> This sounds like a good enhancement for JDK-10.
>> 
>>> To summarize the desired behavior:
>>> 
>>>   For class file versions < 53 throw CFE for any bad constant pool
>>>   entry regardless of access_flags.
>>> 
>>>   For class file versions >= 53 throw NCDFE for a bad constant pool
>>>   entry of 19 or 20 and ACC_Module set in access_flags.
>>> 
>>>   For class file versions >= 53 throw CFE for a bad constant pool
>>>   entry of 19 or 20 and ACC_Module not set in access_flags.
>>> 
>>>   For class file versions >= 53 throw CFE for a bad constant pool
>>>   entry other than 19 or 20 regardless of access_flags.
>>> 
>>> The reasoning behind this is that constant pool entries 19 and 20 are
>>> only valid when ACC_Module is set.  So, if ACC_Module is set then
>>> constant pool entries 19 and 20 are technically valid and CFE should not
>>> be thrown.  Instead, a NCDFE is thrown because of ACC_Module.
>>> 
>>> I moved the check for ACC_MODULE out of verify_legal_class_modifiers()
>>> for the following reason.  Suppose there's a CONSTANT_Module entry in
>>> the constant pool and suppose verify_legal_class_modifiers() throws a
>>> CFE for a reason unrelated to ACC_MODULE.  That could be considered a
>>> bug because the CFE should be thrown with a message describing the bad
>>> CONSTANT_Module entry, not the bad access_flags.  If it does not matter
>>> which CFE gets thrown then the ACC_MODULE check could be moved back into
>>> verify_legal_class_modifiers().
>> 
>> I prefer the ACC_MODULE check where it was so that we don't duplicate the logic. I'm not aware of anything that gives precedence to one CFE over another, or how anyone could legitimately complain about the behaviour either way.
> If no one objects then I'll move the ACC_MODULE check back into verify_legal_class_modifiers().  It will simplify the change.

+1 on keeping the code simple.  If the simple logic breaks something we can make the code more complex.

The bug here is that we want to throw a NCDFE instead of a CFE for modules, but modules are likely to contain odd constants..

The basic constraints are:  1. the JVM class-file loader must (somehow) refuse to "load" ACC_MODULE files (as a JVMS condition), 2. the JVM should not be required to perform semantic processing of 19 and 20 constants (because YAGNI, and unused code makes bugs).

(Maybe in the future we will make CP tags 19 and 20 semantically significant in normal class files.  When that day comes, we must fully process them.  But this is not that day.  This day we throw!)

That implies we should try to fail-fast as soon as we see 19 or 20 CP constants.  If some external spec. forces us to fail later, after getting to where we can test for ACC_MODULE, then we have to keep going through the class files, using only minimal logic to skip (not store) the strange constants, as we do with annotations.

I think the bug JDK-8175383 should give the reasons (either JVMS language or use case) why the more convenient CFE would be a bad outcome.  In other words, why are we doing this?

— John