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

[David Holmes]

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!

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