URGENT code review request for Solaris FDS fix (7175255)

[David Holmes]

On 20/06/2012 1:23 PM, Daniel D. Daugherty wrote:
> Thanks for the quick review!

You said it was urgent ;-)

> On 6/19/12 8:06 PM, David Holmes wrote:
>> It would be nice if the cd into the 64 directory could be handled
>> internally to the link logic rather than occurring at the top-level (I
>> say this as someone who will need to hand merge this into another
>> workspace ;-) ).
>
> I'm not quite sure what you mean by this: "the cd into the 64 directory
> could be handled internally to the link logic rather than occurring at
> the top-level".
>
> This isn't "at the top-level". It's in the dtrace.make subsidiary
> Makefile and it only happens for the context of the subshell.
>
> The problem is that ADD_GNU_DEBUGLINK and the ZIP_EXE need to be
> done in the 64 directory, but nothing else in that crazy Makefile
> logic does need to be done in the 64 directory.

I "simply" meant doing the "cd 64" inside the ADD_GNU_DEBUGLINK logic 
rather than in the makefile that invokes that logic. That said I can 
live with Kelly's shorter form.

>> Also in make/solaris/makefiles/add_gnu_debuglink.make I don't
>> understand the logic change:
>>
>> GENERATED = ../generated
>>
>> becomes
>>
>> TOPDIR = $(shell echo `pwd`)
>> GENERATED = $(TOPDIR)/../generated
>>
>> but at what time is "pwd" evaluated? If we have: /out/lib/64 and
>> originally we started in lib then GENERATED==lib/../generated ie
>> out/generated. If we have now done a cd into 64 then: pwd =
>> /out/lib/64 and so GENERATED==/out/lib/64/../generated ie
>> /out/lib/generated. I may well be missing something but this doesn't
>> seem right.
>
> Please see the very long answer that I gave Kelly about the GENERATED
> variable and HotSpot Makefiles. In particular, the TOPDIR followed
> by the setting of GENERATED is used in the Linux and BSD HotSpot
> makefiles (most of them anyway)...
>
> I'll wait to see what you can clarify about the "top-level" comment
> above before I try to resolve this any further...

I read the long answer and yes it is complex. As I recently had to 
untangle this for other reasons you (ie the hotspot developer cursed to 
modify the build system) need to be aware that we effectively have three 
levels of "make" invocations when building hotspot:
- "top level" Makefile or <os>/Makefile
- buildtree.make sub-make
- make of the makefiles generated by  buildtree.make

determining which files get included at each level, and which variables 
get passed through the submakes is complex. Lazy evaluation of variables 
adds to the complexity.

That all said, it still seems to me that the logic for GENERATED is not 
correct if you have done a cd into the 64 subdirectory.

Cheers,
David
> Dan
>
>
>
>>
>> David
>> -----
>>
>> On 20/06/2012 11:21 AM, Daniel D. Daugherty wrote:
>>> Greetings,
>>>
>>> This is an URGENT code review request for a Solaris specific Full Debug
>>> Symbols (FDS) fix. Due to a Makefile logic error, the full debug symbol
>>> files and related '_g' symlinks are created in the wrong sub-directory
>>> for a couple of the dtrace libraries. The incorrect paths have a double
>>> "64/" sub-directory, e.g.:
>>>
>>> solaris-<arch>/jre/lib/<arch>/client/64/64/libjvm_db.debuginfo
>>>
>>> These are the correct symlink paths:
>>>
>>> solaris-<arch>/fastdebug/jre/lib/<arch>/client/64/libjvm_g_db.debuginfo
>>> solaris-<arch>/fastdebug/jre/lib/<arch>/client/64/libjvm_g_dtrace.debuginfo
>>>
>>> solaris-<arch>/fastdebug/jre/lib/<arch>/server/64/libjvm_g_db.debuginfo
>>> solaris-<arch>/fastdebug/jre/lib/<arch>/server/64/libjvm_g_dtrace.debuginfo
>>>
>>>
>>> and these are the correct debug info file paths:
>>>
>>> solaris-<arch>/jre/lib/<arch>/client/64/libjvm_db.debuginfo
>>> solaris-<arch>/jre/lib/<arch>/client/64/libjvm_dtrace.debuginfo
>>> solaris-<arch>/jre/lib/<arch>/server/64/libjvm_db.debuginfo
>>> solaris-<arch>/jre/lib/<arch>/server/64/libjvm_dtrace.debuginfo
>>> solaris-<arch>/fastdebug/jre/lib/<arch>/client/64/libjvm_db.debuginfo
>>> solaris-<arch>/fastdebug/jre/lib/<arch>/client/64/libjvm_dtrace.debuginfo
>>>
>>> solaris-<arch>/fastdebug/jre/lib/<arch>/server/64/libjvm_db.debuginfo
>>> solaris-<arch>/fastdebug/jre/lib/<arch>/server/64/libjvm_dtrace.debuginfo
>>>
>>>
>>> where "<arch>" is "i586" or "sparc". The 64-bit Solaris platforms
>>> ("amd64"
>>> and "sparcv9") don't have this issue because they don't have the "64/"
>>> sub-directories.
>>>
>>> This fix is targeted at HSX-24/JDK8 and HSX-23.2/JDK7u6 and will resolve
>>> an issue that is preventing Oracle's Release Engineering scripts from
>>> running properly.
>>>
>>> Here is the webrev URL for the HSX-24/JDK8 version:
>>>
>>> http://cr.openjdk.java.net/~dcubed/fds_revamp/7175255-webrev/0/
>>>
>>> The HSX23.3/JDK7u6 version is the same except for the changes to
>>> make/solaris/makefiles/defs.make which are not needed in HSX23.2.
>>>
>>> Thanks, in advance, for any reviews!
>>>
>>> Dan