URGENT code review request for Solaris FDS fix (7175255)

[Daniel D. Daugherty]

Thanks for the quick review!


On 6/19/12 7:58 PM, Kelly O'Hair wrote:
>   130         ( set -e ; \
>   131           cd 64 ; \
>   132           $(QUIETLY) $(ADD_GNU_DEBUGLINK) \
>   133             $(LIBJVM_DB_DEBUGINFO) $(LIBJVM_DB) ; \
>   134         )
> Would be better as:
>   130         ( cd 64&&  $(ADD_GNU_DEBUGLINK) $(LIBJVM_DB_DEBUGINFO) $(LIBJVM_DB) )

The "set -e" was your idea in another FDS fix. :-)
You wanted the sub-shell to die if something went wrong
which would return an error to the outer make.

I do like dropping the "$(QUIETLY)" though.


> And making GENERATED a full path scares me. That variable is used all over the place.
> Are we sure all uses are ok being full paths?

There's good news and bad news about the GENERATED variable in
HotSpot Makefiles. :-) But you know this already... somewhere in
the back of your mind... you remember seeing this insanity...

The good news is that Linux and BSD pretty much do this:

   27 TOPDIR                    = $(shell echo `pwd`)
   28 GENERATED                 = $(TOPDIR)/../generated

in most of their HotSpot Makefiles. Solaris, on the other hand,
did not do this before, but I'm adding it in a limited way now.
Why do I say "limited way"? Well... that's the bad news...

Most of the HotSpot Makefiles set the GENERATED variable all
on their own... And since vm.make includes most of the other
HotSpot Makefiles, you get GENERATED set to various values
as the various Makefiles are included... Has this made you
ill yet??

What does all this mean? Well it means that GENERATED will be
set to a full path for when add_gnu_debuglink.make and when
fix_empty_sec_hdr_flags.make are included. Which means that both
ADD_GNU_DEBUGLINK and FIX_EMPTY_SEC_HDR_FLAGS will be set to
full paths which means they can be called from anywhere.
As soon as one of the other HotSpot Makefiles that sets
GENERATED to "../generated" is included, then GENERATED goes
back and in the context of that HotSpot Makefile (and beyond),
the paths are relative again. Insane you say? You betcha, but
please remember that I didn't write this GENERATED variable
insanity... I'm just making it work.

FIX_EMPTY_SEC_HDR_FLAGS isn't called from anywhere where
we did a "cd" so that change isn't strictly necessary. I
could drop it without worry. Until someone else does something
with it from some sub-directory somewhere and wonders why
the heck the command can't be found. Yes, that's what happened
to me today...

> And shouldn't this 64 directory name be in a variable to make it more obvious?
> Maybe LIBJVM_DIRNAME64=64?

To be consistent, I should name it XLIBJVM_DIRNAME to match all
the other 64/... variable names. Yes, I used a literal "64"
because I'm getting tired of this FDS and Makefile crap. I can
fix that also. Do you want all the variables that use 64/...
to switch to this new variable? Seems like a lot of changes,
but if I don't, then it'll be inconsistent. Your call.


> Is this a last minute change that is expected to be extremely low risk? Doesn't feel low risk. :^(  Sorry.

To make it more low risk, I could drop the changes to
fix_empty_sec_hdr_flags.make. That would make the GENERATED
variable a full-path for the include of add_gnu_debuglink.make
and for the setting of ADD_GNU_DEBUGLINK. That would be it
for a full-path GENERATED value on Solaris.

I've updated my monster ck_fds_jprt_job script to look into
.diz ZIP archives and verify that they don't use path elements.
The HSX-24 and HSX-23.2 test jobs that I ran earlier today
pass the latest ck_fds_jprt_job script without error.

I haven't done "hotspot + jdk" control jobs or "hotspot +
everything else" control jobs yet. But since the bug was
with the contents of the ".diz" file and did not show up
until the .diz files were extracted by the intall repo, I'm
not too worried about the control jobs. I will run them
since I want to update my database of FDS test results, but
I don't think they are critical.

To address your review, I plan to:

- drop the "$(QUIETLY)"
- drop the changes to fix_empty_sec_hdr_flags.make
- add the XLIBJVM_DIRNAME=64 variable and use it in
   the new code or in all uses of 64/... (Kelly's choice)

Please let me know if this resolves your issues with the changes.

And, thanks again for the quick review!

Dan



> -kto
>
> On Jun 19, 2012, at 6:21 PM, 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