RFR(M) : 7104565 : trim jprt build targets

[David Holmes]

On 10/04/2013 2:09 PM, David Holmes wrote:
> On 10/04/2013 1:09 PM, Vladimir Kozlov wrote:
>> On 4/9/13 7:33 PM, David Holmes wrote:
>>> A few comments/concerns:
>>>
>>> jprt.properties:
>>>
>>> So the JPRT GC "_default" tests were redundant? Don't they test that
>>> ergonomics based selection works okay versus forced GC selection?
>>
>> It is redundant because forced selection use all gc collectors.
>
> Ok.
>
>>>
>>> makefile related:
>>>
>>> The bug says that the debug targets are unused but a fix for that is not
>>> what has been implemented. Please update the CR to reflect that debug
>>> lives and jvmg/profiled are what are now gone.
>>>
>>> Why is there no jvmgminimal1_warn target?
>>>
>>> ---
>>>
>>> As discussed in the CR I have concerns about replacing all ifdef
>>> DEBUG/FASTDEBUG with ASSERT as they do not mean the same thing. In
>>> particular:
>>>
>>> I don't agree with the removal of the FASTDEBUG define as used here:
>>>
>>> ./os/solaris/dtrace/generateJvmOffsets.cpp:#if defined(DEBUG) ||
>>> defined(FASTDEBUG)
>>> ./os/solaris/dtrace/generateJvmOffsets.cpp:#endif /* defined(DEBUG) ||
>>> defined(FASTDEBUG) */
>>> ./os/bsd/dtrace/generateJvmOffsets.cpp:#if defined(DEBUG) ||
>>> defined(FASTDEBUG)
>>> ./os/bsd/dtrace/generateJvmOffsets.cpp:#endif /* defined(DEBUG) ||
>>> defined(FASTDEBUG) */
>>>
>>> the code this guards couldn't care less about ASSERT, it cares about a
>>> symbol issue that is related to doing debug/fastdebug builds. (I wonder
>>> if this is still an issue though - what different build settings do we
>>> use for a debug build ??)
>>
>> For both debug/fastdebug builds ASSERT is set. So this replacement is
>> correct.
>
> It is equivalent based on the present setting of flags, but code added
> for debugging purposes and code added for assertions are quite distinct
> things. I think many people will be surprised to find that we have:
>
> #ifdef ASSERT
> #define DEBUG_ONLY(code) code
> #define NOT_DEBUG(code)
>
> and not
>
> #if defined(DEBUG) || defined(FASTDEBUG)
> #define DEBUG_ONLY(code) code
> #define NOT_DEBUG(code)
>
>
>> Yes, using ASSERT here could be confusing and we could add new
>> -DAVOID_C1_DEBUG_LINK_ERROR but we have the comment which explains the
>> problem and we know that ASSERT is set in such case.
>>
>> I agree that we need to investigate this case and fix the problem
>> instead of using this workaround.
>>
>>>
>>> Ditto for src/share/vm/classfile/stackMapFrame.hpp - that code is not
>>> assert related but debugging related. Further you have now added that
>>
>> It is assert related! VM will assert (I hope) somewhere where we check
>> values in wrong place on stack or VM dies (SEGV) which is also good
>> since it is incorrect to use invalid values.
>
> Even if ultimately this code relies on assertions to flag errors it is
> still fundamentally additional debugging code. It is an extra level of
> checking/verification that we did not want to do for either product
> builds or fastdebug builds. But now it is in fastdebug. Is that a
> problem? can't say for sure as I don't do anything that relies on
> fastdebug being fast. If no one cares then maybe we can kill fastdebug
> too? Afterall it already has all that DEBUG_ONLY code in it so not as
> fast as one might assume.

Which only makes sense if above I had said:

#if defined(DEBUG)
#define DEBUG_ONLY(code) code
#define NOT_DEBUG(code)

rather than

#if defined(DEBUG) || defined(FASTDEBUG)
#define DEBUG_ONLY(code) code
#define NOT_DEBUG(code)

David
-----

>>> code to fastdebug builds - which seems wrong (or at least potentially
>>> impacts the "fast" part!). Also as Vladimir has indicated in the bug
>>> report this code would now be getting tested for the first time! - so
>>> potential bug tail there.
>>
>> I don't understand this argument. It would be good if it helps to find
>> problems here. Why do you want to hide problems?
>
> This last part wasn't an argument (sorry if it wasn't clear) just an
> observation. It wouldn't be obvious to the readers that this changeset
> will result in previously untested code now being tested. (Heads up to
> gatekeepers!)
>
>>> The changes in src/share/vm/shark/llvmHeaders.hpp need to be confirmed
>>> by the shark folk (if they haven't already). (And of course depend on
>>> -DDEBUG being eradicated.)
>>
>> Oracle is not supporting shark! They are on this mailing alias. We
>> discussing this for long time already. We hear nothing from them. If
>> they find problems later they will send a patch and we use it.
>
> As long as this shouldn't break their build (which it shouldn't). But
> based on that view you could have simply left this untouched.
>
> Thanks,
> David
>
>> Thanks,
>> Vladimir
>>
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>
>>> On 10/04/2013 9:01 AM, David Chase wrote:
>>>>
>>>> On 2013-04-09, at 5:26 PM, Vladimir Kozlov
>>>> <vladimir.kozlov at oracle.com> wrote:
>>>>> One note: in frame_sparc.cpp remove 'jvmg' in the line you modified
>>>>> since we will not build jvmg.
>>>>
>>>> done
>>>>
>>>>> And question: did you use mercurial commands to remove and rename
>>>>> jvm.make and debug.make? We need to use hg commands to preserve
>>>>> changes history for jvmg.make (it has more changesets then old
>>>>> debug.make).
>>>>>
>>>>> I just want to confirm because sometimes webrev may not show
>>>>> correctly what was done to sources. I mean you need to do:
>>>>>
>>>>> hg remove debug.make
>>>>> hg rename jvmg.make debug.make
>>>>>
>>>>> and then edit debug.make
>>>>
>>>> To be sure:
>>>>
>>>> 10009  hg revert make/{solaris,bsd,linux}/makefiles/jvmg.make
>>>> 10010  zip -9 ../safemakes.zip
>>>> make/{solaris,bsd,linux}/makefiles/debug.make
>>>> 10011  hg revert make/{solaris,bsd,linux}/makefiles/debug.make
>>>> 10012  hg remove make/{solaris,bsd,linux}/makefiles/debug.make
>>>> 10013  for i in solaris bsd linux ; do hg rename
>>>> make/$i/makefiles/{jvmg,debug}.make; done
>>>> 10014  unzip -l ../safemakes.zip
>>>> 10015  unzip ../safemakes.zip
>>>>
>>>> And the new-and-improved diffs:
>>>>
>>>> http://cr.openjdk.java.net/~drchase/7104565/webrev.06-new/
>>>>
>>>>