Queries and patch for JDK-8034854: outer_class_info_index of synthetic class is not zero - approved

Tue Feb 25 06:28:52 PST 2014

On 25/02/14 09:01, Jan Lahoda wrote:
> Hi Vicente,
>
> Updated patch:
> http://cr.openjdk.java.net/~jlahoda/8034854/webrev.02/

Hi Jan,

It's OK for me,

Vicente

>
> Thanks for the comments,
>     Jan
>
> On 02/21/2014 06:34 PM, Vicente-Arturo Romero-Zaldivar wrote:
>> On 21/02/14 16:29, Jan Lahoda wrote:
>>> On 02/20/2014 10:06 PM, Alex Buckley wrote:
>>>> Interesting reasons. Does reuse of an existing top-level or anonymous
>>>> class choose from a pool of all classes (inc. user-defined ones) or 
>>>> only
>>>> synthetic classes?
>>>
>>> User-defined anonymous classes are reused for private constructor tags
>>> if available, but, on a closer look, user-defined anonymous classes
>>> don't seem to be reused in other cases. User-defined top-level classes
>>> (if available) appear to be reused for targets <= 1.4 to hold the
>>> desugared code for class literals.
>>>
>>>>
>>>> I'm still not clear if "one class is synthesized per top-level type"
>>>> means "one member class" or "one anonymous class". I guess the
>>>
>>> Internally, they look somewhat like a member class with an empty name
>>> ("anonymous member class"). But I don't think the internal
>>> representation should affect the content of the attributes in this 
>>> case.
>>>
>>>> ill-formed answer currently given by javac is "one member-anonymous
>>>> class", because the non-zero outer_class_info_index implies it's a
>>>> member class while the zero inner_name_index implies it's an anonymous
>>>> class - yuk. I recommend making them truly anonymous.
>>>
>>> Thanks.
>>>
>>> A webrev that updates tests according to comments received so far:
>>> http://cr.openjdk.java.net/~jlahoda/8034854/webrev.01/
>>
>> Hi Jan,
>>
>> I would modify the proposed test by including all the cases for which
>> javac generates synthetic classes, also it would be preferable if the
>> test is included in just one file.
>>
>> Thanks,
>> Vicente
>>
>>>
>>> Jan
>>>
>>>>
>>>> Alex
>>>>
>>>> On 2/20/2014 12:34 PM, Jan Lahoda wrote:
>>>>> As far as I was able to determine, these are the cases where and why
>>>>> the
>>>>> auxiliary classes are generated/used:
>>>>> -as tags for access constructors for private constructors. An 
>>>>> existing
>>>>> anonymous innerclass is reused as a tag, if available, otherwise at
>>>>> most
>>>>> one class is synthesized per top-level type.
>>>>> -for target levels whose ldc instruction does not support 
>>>>> references to
>>>>> classes, desugared code for class literals (a "getter" and cache for
>>>>> the
>>>>> Class objects) is placed into a "cache" class. Depending on the
>>>>> circumstances, a top-level class or an existing anonymous 
>>>>> innerclass is
>>>>> reused if possible, otherwise at most one class is synthesized per
>>>>> top-level type.
>>>>> -for switch-over-enum, a lazy map between enum constants and
>>>>> ordinals is
>>>>> placed into a cache class. An existing anonymous innerclass is 
>>>>> reused,
>>>>> if available, otherwise at most one class is synthesized per 
>>>>> top-level
>>>>> type.
>>>>> -for interfaces, to hold their assertions enabled status. At most one
>>>>> class is synthesized per top-level type.
>>>>>
>>>>> Jan
>>>>>
>>>>> On 02/20/2014 08:14 PM, Alex Buckley wrote:
>>>>>> It would make sense to consider the full range of reasons why these
>>>>>> auxiliary classes are generated. You indicated one reason - tags for
>>>>>> accessing private ctors - and it makes sense to generate a "true"
>>>>>> anonymous class there (outer_class_info_index=0, 
>>>>>> inner_name_index=0).
>>>>>> But perhaps other reasons would justify auxiliary classes with
>>>>>> meaningful "owners" - again, your word - and there it would be
>>>>>> sensible
>>>>>> to consider them as member classes rather than anonymous classes.
>>>>>>
>>>>>> Alex
>>>>>>
>>>>>> On 2/20/2014 3:11 AM, Jan Lahoda wrote:
>>>>>>> Hi Alex,
>>>>>>>
>>>>>>> Thanks for the comments.
>>>>>>>
>>>>>>> I was briefly considering filling some inner_name for the synthetic
>>>>>>> classes, but using zeroing outer_class_info_index seemed somewhat
>>>>>>> cleaner, safer (no risk of name clashes or misinterpretation of the
>>>>>>> name) and simpler. But if generating an inner_name for the 
>>>>>>> synthetic
>>>>>>> classes would be (strongly) preferred, I can investigate it.
>>>>>>>
>>>>>>> Jan
>>>>>>>
>>>>>>> On 02/19/2014 08:13 PM, Alex Buckley wrote:
>>>>>>>> Hi Jan,
>>>>>>>>
>>>>>>>> The requirement that outer_class_info_index must agree with
>>>>>>>> inner_name_index w.r.t. an anonymous class was added in JVMS7
>>>>>>>> because we
>>>>>>>> saw class files where they disagreed and it simply made no sense.
>>>>>>>> The
>>>>>>>> requirement was conditioned on 51.0 class files because we didn't
>>>>>>>> want
>>>>>>>> to break pre-7 class files with insensible InnerClasses.
>>>>>>>>
>>>>>>>> The auxiliary classes generated by javac appear to have a 
>>>>>>>> meaningful
>>>>>>>> "owner" - your word - so it would seem appropriate to have a
>>>>>>>> non-zero
>>>>>>>> outer_class_info_index. Just generate a random name for
>>>>>>>> inner_name_index. (The 4.7.6 text assumes the "original simple 
>>>>>>>> name"
>>>>>>>> can
>>>>>>>> be derived from source code, but that's not applicable for 
>>>>>>>> synthetic
>>>>>>>> classes.) This change could reasonably affect all target levels,
>>>>>>>> since
>>>>>>>> no-one should be relying on the value of inner_name_index for 
>>>>>>>> these
>>>>>>>> auxiliary classes.
>>>>>>>>
>>>>>>>> OTOH, your proposal to represent the auxiliary classes as true
>>>>>>>> anonymous
>>>>>>>> classes in InnerClasses is attractive because it exposes even less
>>>>>>>> information than at present. This change could reasonably 
>>>>>>>> affect all
>>>>>>>> target levels too, since no-one should be relying on the value of
>>>>>>>> outer_class_info_index for these auxiliary classes.
>>>>>>>>
>>>>>>>> Alex
>>>>>>>>
>>>>>>>> On 2/19/2014 4:34 AM, Jan Lahoda wrote:
>>>>>>>>> Hello,
>>>>>>>>>
>>>>>>>>> I have a few questions about JDK-8034854 and a possible
>>>>>>>>> patch/fix for
>>>>>>>>> it. The bug URL:
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8034854
>>>>>>>>>
>>>>>>>>> The problem is that while JVMS 7, 4.7.6. (The InnerClasses
>>>>>>>>> Attribute)
>>>>>>>>> mandates that:
>>>>>>>>>   If a class file has a version number that is greater than or
>>>>>>>>> equal to
>>>>>>>>>   51.0, and has an InnerClasses attribute in its attributes 
>>>>>>>>> table,
>>>>>>>>> then
>>>>>>>>>   for all entries in the classes array of the InnerClasses
>>>>>>>>> attribute,
>>>>>>>>>   the value of the outer_class_info_index item must be zero if 
>>>>>>>>> the
>>>>>>>>> value
>>>>>>>>>   of the inner_name_index item is zero.
>>>>>>>>> javac in some cases produces non-zero "outer_class_info_index"
>>>>>>>>> even if
>>>>>>>>> "inner_name_index" is zero. This happens for synthetically
>>>>>>>>> generated
>>>>>>>>> auxiliary classes. These classes are generated for a number of
>>>>>>>>> reasons,
>>>>>>>>> for example to be used as tags when accessing private 
>>>>>>>>> constructors.
>>>>>>>>> The
>>>>>>>>> synthetic classes internally have an empty name, so the generated
>>>>>>>>> "inner_name_index" is zero, but their owner is a class, so 
>>>>>>>>> they get
>>>>>>>>> the
>>>>>>>>> non-zero "outer_class_info_index".
>>>>>>>>>
>>>>>>>>> I've sketched out a simple fix for this problem, which ensures 
>>>>>>>>> that
>>>>>>>>> "outer_class_info_index" is zero for classes that have empty 
>>>>>>>>> name:
>>>>>>>>> http://cr.openjdk.java.net/~jlahoda/8034854/webrev.00/
>>>>>>>>>
>>>>>>>>> After this change, the generated synthetic classes look a lot 
>>>>>>>>> like
>>>>>>>>> anonymous classes defined in an initializer of the given class
>>>>>>>>> (based on
>>>>>>>>> the InnerClasses attribute and the EnclosingMethod attribute). 
>>>>>>>>> That
>>>>>>>>> seems reasonable to me.
>>>>>>>>>
>>>>>>>>> My questions are:
>>>>>>>>> -does the fix above make sense?
>>>>>>>>> -the change affects all target levels. It seems to me that the 
>>>>>>>>> new
>>>>>>>>> behavior makes sense even for pre-7 classfiles, but I'll gladly
>>>>>>>>> limit
>>>>>>>>> the new behavior to only some minimal target level if desired.
>>>>>>>>>
>>>>>>>>> Any comments welcome.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>     Jan
>>