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

[Coleen Phillimore]

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.

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