[10] RFC: Relaxing access restrictions for VM annotations

[John Rose]

On Feb 22, 2017, at 2:20 PM, Vladimir Ivanov <vladimir.x.ivanov at oracle.com> wrote:
> 
> Thanks for the feedback, John.

(You are kind; it wasn't very on-point.)

> The problem here is not related to bootstrapping (the checks for privileged classes can be just skipped), but because we need to make checks in the context of the class being loaded (in particular, determine the module the class belongs to).
> 
> I did a quick prototype of accessibility checks during parsing and ended up with the following:
>  http://cr.openjdk.java.net/~vlivanov/vm_annotations/webrev.01/
> 
> Diff between versions:
>  http://cr.openjdk.java.net/~vlivanov/vm_annotations/webrev.01_vs_00/

Yes, that's ugly.  The IK is lying in parts on the operating table and you need to ask it an urgent question.  So you make a note to ask the question later on when it is awake.  (And then if it gives the wrong answer, it's back to the table!)

OK, so the retry is not so ugly.  I would prefer if the retry logic were the *only* thing happening in the function that manages it.  In particular, the soundness of the technique (replaying the second time with a more precise annotation mask) will be easier to verify if the function doesn't have any other distractions in it.  I see you have some "FIXME" items to refactor stray code elsewhere, so +1 to that.

> It required to duplicate code from IK::set_package(), IK::module(), and Reflection::verify_class_access(). Also, I had to change the loading sequence and compute (possibly, create) package early in the parsing phase.
> 
> Overall, it looked much messier and more brittle than wrapping class file paring into re-parsing logic.

As we discussed on IM you can do more with those nifty little annotation masks, including location validity testing.

The stress test strategy looks good.  If there are residual bugs due to reparsing they will only affect those rare class files which attempt unsuccessfully to use the scoped annotations.

Reviewed, with suggestions.

— John

P.S.  Tentative suggestion:  Extend the logic to optionally track *all* annotations, with a flag -XX:+CheckInaccessibleAnnotations or some such.  Would require an extra growable array off to the side.  The benefit would be a debugging tool that would detect a general superset of the visibility anomalies that the JVM needs to detect.  This touches more on the language's binary compatibility story for annotations, not so much the JVM.  It's maybe worth a tracking bug so folks can vote for it.