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

Tue Mar 22 17:31:06 UTC 2016

Hi Max,

These changes look good.

Harold

On 3/22/2016 12:52 PM, Max Ockner wrote:
> 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
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>