RFR: JDK-8230776: Javac throws AssertionError in jvm.Gen.visitExec

[Jan Lahoda]

Hi Vicente,

I think it looks great overall.

Basically my only comment is for PoolReader. Given the performance of 
this code is potentially important, would it make sense to make the 
validity checks as fast as possible? E.g. having something like:
readIfNeeded(int index, BitSet expectedTags) {

...
if (!expectedTags.get(currentTag)) {
     throw reader.badClassFile("unexpected.const.pool.tag.at", 
tag(index), offset(index));
}

And having static final pre-created BitSets used to pass into this method?

Thanks,
     Jan

On 28. 08. 20 0:01, Vicente Romero wrote:
> Hi Jan,
> 
> Please review another iteration of the patch at [1], this one is a bit 
> more complex as it addresses several very related bugs together. These 
> bugs are [2-6]. The common factor is that javac interrupts compilation 
> with an exception or assertion error while compiling a source file that 
> extends a fuzzed class file.
> 
> Thanks,
> Vicente
> 
> [1] http://cr.openjdk.java.net/~vromero/8230776/webrev.01/
> [2] https://bugs.openjdk.java.net/browse/JDK-8231311
> [3] https://bugs.openjdk.java.net/browse/JDK-8230964
> [4] https://bugs.openjdk.java.net/browse/JDK-8230963
> [5] https://bugs.openjdk.java.net/browse/JDK-8230919
> [6] https://bugs.openjdk.java.net/browse/JDK-8230776
> 
> 
> On 8/26/20 4:26 AM, Jan Lahoda wrote:
>> Hi Vicente,
>>
>> I am sorry, but I don't think this is a fix on a good place.
>>
>> The root cause here is that the classfile for Test has an invalid 
>> method descriptor for the Test's constructor (returns float instead of 
>> void). See JVMS 4.6., descriptor_index. I think it would be much 
>> better to validate method descriptors, and reject classfiles with 
>> descriptors not adhering to JVMS. The user would then get a much more 
>> helpful error, saying something like:
>> ---
>> Sub.java:1: error: cannot access Test
>> class Sub extends Test {}
>>                   ^
>>   bad class file: 
>> /home/jlahoda/src/jdk/jdk/build/linux-x86_64-server-release/nb-jtreg/work/classes/tools/javac/T8230776/AssertionErrorTest.d/Test.class 
>>
>>     method descriptor invalid for <init>
>>     Please remove or make sure it appears in the correct subdirectory 
>> of the classpath.
>> 1 error
>> ---
>>
>> What do you think?
>>
>> Jan
>>
>> On 25. 08. 20 19:23, Vicente Romero wrote:
>>> Please review fix for [1] at [2]. The fix is ending the compilation 
>>> at the code generation phase if a statement cant be generated and 
>>> there is no way for the compilation to continue.
>>>
>>> Thanks,
>>> Vicente
>>>
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8230776
>>> [2] http://cr.openjdk.java.net/~vromero/8230776/webrev.00/
>