[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-hotspot-runtime-dev
Subject: Re: RFR: 8146632: Add descriptive error messages for removed non-product logging flags.
From: Max Ockner <max.ockner () oracle ! com>
Date: 2016-03-23 14:18:09
Message-ID: 56F2A5A1.7020306 () oracle ! com
[Download RAW message or body]
Thanks everyone!
On 3/23/2016 7:43 AM, David Holmes wrote:
> Looks fine.
>
> Thanks,
> David
>
> On 23/03/2016 2:52 AM, 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
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>
>>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic