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

Fri Mar 18 14:03:17 UTC 2016

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.

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
>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>