RFR(M) : 7104565 : trim jprt build targets

[David Holmes]

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.

>> 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/
>>>
>>>