URGENT code review request for Solaris FDS fix (7175255)

[Daniel D. Daugherty]

On 6/19/12 9:37 PM, David Holmes wrote:
> On 20/06/2012 1:23 PM, Daniel D. Daugherty wrote:
>> Thanks for the quick review!
>
> You said it was urgent ;-)

Yes, but sometimes that still doesn't get me timely reviews. :-)
However, you have to be crazy to change HotSpot Makefiles so...
can you really blame people for not wanting to review this stuff?


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

You mean inside the $(ADD_GNU_DEBUGLINK) tool or inside the macro
that's used to call the tool? The tool is called from a couple of
different places. Inside the 64/ dir is just one of them...

Let's see how things evolve based on Kelly next reply...

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

Yes, I ran into those issues with getting the various FDS variables
in "all the right places" and with "all the same values". Pain in
the butt and needs to be rewritten, but that's another project.


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

OK, but I don't think lazy evaluation is coming into play
with my new TOPDIR variable or the way I'm setting GENERATED.


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

Exactly the opposite is true. Without the TOPDIR/GENERATED combo,
when you "cd 64", the "../generated" that the GENERATED variable
resolves to no longer works. When GENERATED is set to
"$(TOPDIR)/../generated", then it becomes an absolute path that
can be used anywhere...

However, I think where this is going to end up is something like:

- add TOPDIR only to add_gnu_debuglink.make
- set the ADD_GNU_DEBUGLINK variable using TOPDIR and a
   literal "../generated/add_gnu_debuglink"
- don't set GENERATED in add_gnu_debuglink.make at all
- drop the changes to fix_empty_sec_hdr_flags.make

I think that'll reduce the risk since this fix needs to
go back to HSX23.2 and HSX24.

Dan


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