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

[David Holmes]

Objections withdrawn. Code Reviewed.

Thanks,
David

On 19/03/2016 7:49 AM, David Holmes wrote:
> 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
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>