On Mon, 12 Aug 2024 17:23:15 GMT, Nizar Benalla <nbena...@openjdk.org> wrote:
> The test is inspired from [FFM API's > TestNulls](https://github.com/openjdk/jdk/blob/master/test/jdk/java/foreign/TestNulls.java), > I customized their Null checking framework it to work with ClassFile API. > > The framework for for testing an API in bulk, so that all methods are > reflectively called with some values replaced with nulls, so that all > combinations are tried. > > This test reveals some inconsistencies in the ClassFile API, whether it's a > method with nullable arguments`[1]`, nulls are passed to`[2]` or its > implementation handles nulls `[3]` `[4]`. > > > `[1]:` > [ModuleRequireInfo#of](https://github.com/openjdk/jdk/blob/25e03b52094f46f89f2fe8f20e7e5622928add5f/src/java.base/share/classes/java/lang/classfile/attribute/ModuleRequireInfo.java#L89) > `[2]:` > [Signature$ClassTypeSig#of](https://github.com/openjdk/jdk/blob/7ad61605f1669f51a97f4f263a7afaa9ab7706be/src/java.base/share/classes/java/lang/classfile/Signature.java#L150) > `[3]:` > [CatchBuilderImpl#catching](https://github.com/openjdk/jdk/blob/7ad61605f1669f51a97f4f263a7afaa9ab7706be/src/java.base/share/classes/jdk/internal/classfile/impl/CatchBuilderImpl.java#L55) > `[4]:` > [CodeBuilder#loadConstant](https://github.com/openjdk/jdk/blob/7ad61605f1669f51a97f4f263a7afaa9ab7706be/src/java.base/share/classes/java/lang/classfile/CodeBuilder.java#L615) > > > > `test/jdk/jdk/classfile/TestNullHostile.java#EXCLUDE_LIST` has the list of > methods that are still not null hostile, they are ignored by the test, as > making them null hostile either breaks some tests (listed below) or breaks > the build with a `NullPointerException` or `BootstapMethodError`. I will > paste the relevant methods from that list here. > > Tests broken by these methods are : > > > jdk/classfile/VerifierSelfTest.java > jdk/classfile/TestRecordComponent.java > jdk/classfile/TestNullHostile.java > jdk/classfile/OpcodesValidationTest.java > jdk/classfile/ClassPrinterTest.java > java/lang/invoke/MethodHandlesGeneralTest.java > java/lang/invoke/MethodHandleProxies/WrapperHiddenClassTest.java > java/lang/invoke/MethodHandleProxies/WithSecurityManagerTest.java > java/lang/invoke/MethodHandleProxies/BasicTest.java > > > Full list of methods > > > //the implementation of this method in CatchBuilderImpl handles nulls, is > this fine? > "java.lang.classfile.CodeBuilder$CatchBuilder/catching(java.lang.constant.ClassDesc,java.util.function.Consumer)/0/0", > > // making this method null-hostile causes a BootstapMethodError during the > the build > // java.lang.BootstrapMethodError: bootstrap method initiali... **Edit: the entire list has now been checked** I now have some tests that pick up on the bug in `Utf8Entry::equalsString`. I have added `Objects::requireNonNull` in a few places, will do an other scan when I'm done to remove a few that may be redundant, as the null check is sometimes implicit. Classes/Interfaces that I haven't tested yet are the following AttributeMapper AttributeMapper.AttributeStability BufWriter ClassBuilder ClassFileElement ClassFileTransform ClassHierarchyResolver.ClassHierarchyInfo ClassModel ClassReader ClassSignature ClassTransform CodeBuilder CodeBuilder.CatchBuilder CodeBuilder.BlockCodeBuilder CodeElement CodeTransform CompoundElement FieldBuilder FieldTransform MethodBuilder MethodElement MethodSignature MethodTransform Opcode Opcode.Kind Signature.ArrayTypeSig Signature.BaseTypeSig Signature.TypeArg Signature.TypeArg.Bounded Signature.TypeArg.Bounded.WildcardIndicator Signature.TypeArg.Unbounded Signature.ClassTypeSig Signature.ThrowableSig Signature.TypeParam Signature.TypeVarSig Signature.RefTypeSig TypeAnnotation.TargetInfo TypeAnnotation.TypePathComponent TypeAnnotation.TypePathComponent.Kind TypeAnnotation.TypeArgumentTarget TypeAnnotation.OffsetTarget TypeAnnotation.CatchTarget TypeAnnotation.LocalVarTargetInfo TypeAnnotation.LocalVarTarget TypeAnnotation.ThrowsTarget TypeAnnotation.FormalParameterTarget TypeAnnotation.EmptyTarget TypeAnnotation.TypeParameterBoundTarget TypeAnnotation.SupertypeTarget TypeAnnotation.TypeParameterTarget TypeAnnotation.TargetType AnnotationDefaultAttribute BootstrapMethodsAttribute ModuleAttribute.ModuleAttributeBuilder ModuleExportInfo ModuleHashInfo ModuleHashesAttribute ModuleMainClassAttribute ModuleOpenInfo ModulePackagesAttribute ModuleProvideInfo ModuleRequireInfo ModuleResolutionAttribute ModuleTargetAttribute ClassPrinter.MapNode ClassPrinter.ListNode ClassPrinter.LeafNode ClassPrinter.Node ClassRemapper CodeLocalsShifter CodeRelabeler Thanks for keeping track of this PR, I will do one more push tomorrow and open it. I shaved 500 lines from the test I was using, I can probably make it more concise with more time. Adding null checks breaks some tests (sometimes even the build), some are method handles related but the rest are limited to CF. I have filed [JDK-8339344](https://bugs.openjdk.org/browse/JDK-8339344) but more Bugs/RFEs will need to be filed. I will modify the PR's body with some questions on which methods can be added to `TestNullHostile##EXCLUDE_LIST` and which need to be updated. Also, I noticed a recent push (#20779) removing `CodeBuilder.loadConstant(Opcode, ConstantDesc)` which had been a major source of pain today. Will merge in a bit. An other want I want to discuss is CodeBuilder.loadConstant(ConstantDesc value). It [handles nulls ](https://github.com/openjdk/jdk/blob/ad40a122d632d65052b71125c0dfd58c54e3a521/src/java.base/share/classes/java/lang/classfile/CodeBuilder.java#L614), and adding a null check causes a `BootstrapMethodError` during the build ------------- PR Comment: https://git.openjdk.org/jdk/pull/20556#issuecomment-2321858976 PR Comment: https://git.openjdk.org/jdk/pull/20556#issuecomment-2325417395 PR Comment: https://git.openjdk.org/jdk/pull/20556#issuecomment-2326579191 PR Comment: https://git.openjdk.org/jdk/pull/20556#issuecomment-2326627971
