RFR(M) : 7104565 : trim jprt build targets

[Vladimir Kozlov]

On 4/9/13 7:33 PM, David Holmes wrote:
> Hi David,
>
> 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.

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

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.

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

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

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