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

Tue Mar 22 16:52:48 UTC 2016

Thanks David.

I have another webrev which moved the delcaration of "replacement" 
inside of the #ifndef PRODUCT.
http://cr.openjdk.java.net/~mockner/8146632.03/

Max

On 3/18/2016 10:37 PM, David Holmes wrote:
> 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
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>