RFR: 8146632: Add descriptive error messages for removed non-product logging flags.

[David Holmes]

On 19/03/2016 12:03 AM, Coleen Phillimore wrote:
>
>
> On 3/18/16 8:45 AM, David Holmes wrote:
>> On 18/03/2016 9:58 PM, Coleen Phillimore wrote:
>>> On 3/18/16 12:12 AM, David Holmes wrote:
>>>> On 17/03/2016 5:20 AM, Coleen Phillimore wrote:
>>>>> Okay, I didn't see your reply.  I also thought that storing a
>>>>> version in
>>>>> the table might be helpful for documentation purposes so we know in
>>>>> the
>>>>> future when to remove the line from the table.  I agree with your
>>>>> implementation choice to have an additional table rather than twisting
>>>>> the other table to cover this use case.
>>>>
>>>> I don't I'm afraid - it is yet another special case and no automatic
>>>> dropping of the message when we switch to JDK 10. Can't it be folded
>>>> into the AliasedLoggingFlag support? I'm really not seeing why the
>>>> conversion of the non-product flags to UL has to be handled
>>>> differently to the product ones ??
>>>
>>> The deprecation of non-product flags has always been different than
>>> product flags.  The AliasedLogging flags alias the old option with the
>>> new flag because they are "deprecated", ie. warned (tbd) but not
>>> removed.  Non-product flags can be removed without deprecation. This new
>>> table is just to improve the error message temporarily for non-product
>>> flags.
>>>
>>> It can't be handled with the aliased logging flag table because they
>>> convert the option.  It is the intent to remove these options.
>>>>
>>>> Aren't these just deprecated flags? We internally convert them to
>>>> their replacement form and issue a warning that they are going away?
>>>
>>> The intent was to make a nice error message for the non-product flags we
>>> removed to help internal users.  i thought you agreed to this in
>>> concept.
>>
>> Concept yes but I'm frustrated by the mechanism - we have too many
>> special cases with all this option processing, and too many tables.
>>
>> As you said above non-product flags can be removed without
>> deprecation, but all deprecation does is produce a nice error message
>> when you use the flag, and you want to add a nice error message for
>> this case so you have in fact turned it back into a deprecation!
>
> Deprecation is ignore and run the JVM.   This is going to exit the JVM
> with unrecognized option with a nicer message.

So it is a special case of "expired" which means it isn't really expired 
because the VM still has to know about it. Hence yet-another-kind-of-flag.

Seems to me this would all be a lot simpler if we treated the 
non-product flags the same as product and just aliased and deprecated 
them in 9, then expire in 10. There seems to be no benefit in what we 
are doing only added complexity.

David

> Coleen
>>
>> David
>>
>>> Coleen
>>>
>>>>
>>>>
>>>> David
>>>>
>>>>> http://cr.openjdk.java.net/~mockner/8146632.02/src/share/vm/runtime/arguments.cpp.udiff.html
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> I think Harold's refactoring makes sense and I think the #endif //
>>>>> PRODUCT
>>>>>
>>>>> }
>>>>> *+ #ifndef PRODUCT*
>>>>> *+ else if ((replacement =
>>>>> removed_develop_logging_flag_name(stripped_argname)) != NULL){*
>>>>> *+ jio_fprintf(defaultStream::error_stream(),*
>>>>> *+ "%s has been removed. Please use %s instead.\n", stripped_argname,
>>>>> replacement);*
>>>>> *+ return false;*
>>>>> *+ #endif*
>>>>> *+ }*
>>>>>
>>>>>
>>>>> I think this should be like this so the {} match inside of #ifndef
>>>>> PRODUCT:
>>>>>
>>>>> *+ #ifndef PRODUCT*
>>>>> *+ } else if ((replacement =
>>>>> removed_develop_logging_flag_name(stripped_argname)) != NULL) {*
>>>>> *+ jio_fprintf(defaultStream::error_stream(),*
>>>>> *+ "%s has been removed. Please use %s instead.\n", stripped_argname,
>>>>> replacement);*
>>>>> *+ return false;*
>>>>> *+ #endif*
>>>>> *+ }*
>>>>>
>>>>>
>>>>> Or refactor as Harold suggested.
>>>>>
>>>>> Coleen
>>>>>
>>>>> On 3/16/16 3:04 PM, Max Ockner wrote:
>>>>>> I did, but it blended in with the rest of the text
>>>>>>
>>>>>> My response: "Seems appropriate to report a specific error message
>>>>>> for
>>>>>> 9 and then remove it for 10. If it would help, we can store a Version
>>>>>> in the table to keep track of when each entry needs to be deleted,
>>>>>> like what is done in the table of obsolete flags. "
>>>>>>
>>>>>>
>>>>>> On 3/16/2016 2:24 PM, Coleen Phillimore wrote:
>>>>>>>
>>>>>>> You should also answer David's concern.
>>>>>>> Coleen
>>>>>>>
>>>>>>> On 3/16/16 2:05 PM, Max Ockner wrote:
>>>>>>>> webrev: http://cr.openjdk.java.net/~mockner/8146632.02/
>>>>>>>>  - Labeled #endif with // PRODUCT
>>>>>>>>  - refactored table lookup code to only do lookup once.
>>>>>>>>  - Added VerboseVerification to the table.
>>>>>>>>
>>>>>>>> Comments below.
>>>>>>>>
>>>>>>>> On 3/16/2016 1:48 AM, David Holmes wrote:
>>>>>>>>> Hi Max,
>>>>>>>>>
>>>>>>>>> On 16/03/2016 3:45 AM, Max Ockner wrote:
>>>>>>>>>> Hello again everyone!
>>>>>>>>>>
>>>>>>>>>> Please review this change which adds better error messages for
>>>>>>>>>> non-product flags that are now converted to Unified Logging.
>>>>>>>>>>
>>>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8146632
>>>>>>>>>> webrev: http://cr.openjdk.java.net/~mockner/8146632/
>>>>>>>>>>
>>>>>>>>>> Since these options have been removed, we want still want the
>>>>>>>>>> vm to
>>>>>>>>>> crash here, but now with an error message giving the correct
>>>>>>>>>> command
>>>>>>>>>> line option. The new message looks like this:
>>>>>>>>>>
>>>>>>>>>>  > TraceClassInitialization has been removed. Please use
>>>>>>>>>> -Xlog:classinit
>>>>>>>>>> instead."
>>>>>>>>>>
>>>>>>>>>> The entire output looks like this:
>>>>>>>>>>
>>>>>>>>>>  > TraceClassInitialization has been removed. Please use
>>>>>>>>>> -Xlog:classinit
>>>>>>>>>> instead.
>>>>>>>>>>  > Error: Could not create the Java Virtual Machine.
>>>>>>>>>>  > Error: A fatal exception has occurred. Program will exit.
>>>>>>>>>
>>>>>>>>> I'm concerned that this has introduced another variant of "flag
>>>>>>>>> deprecation". It begs the question as to when this new code should
>>>>>>>>> be removed. Maybe we need to add "replaced" as another type of
>>>>>>>>> flag
>>>>>>>>> change so we can report in 9 the flag has been replaced and
>>>>>>>>> then in
>>>>>>>>> 10 just report an "unknown option" error ?
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> David
>>>>>>>>>
>>>>>>>> Seems appropriate to report a specific error message for 9 and then
>>>>>>>> remove it for 10. If it would help, we can store a Version in the
>>>>>>>> table to keep track of when each entry needs to be deleted, like
>>>>>>>> what is done in the table of obsolete flags.
>>>>>>>>>> Tested with jtreg runtime tests. A new test checks for an
>>>>>>>>>> appropriate
>>>>>>>>>> error message for every develop flag that has so far been
>>>>>>>>>> converted to
>>>>>>>>>> logging.
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Max
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>
>