Fwd: code review request for initial JDK FDS support (7071907)

[Daniel D. Daugherty]

On 4/10/12 7:54 AM, keith mcguigan wrote:
>
> Hi Dan,
>
> I think it looks good.

Thanks!


> The new form of the 'find' commands in Release.gmk could use a 
> comment, though.  It took me a few minutes to realize that EXE_SUFFIX 
> was empty for linux/solaris -- it looks like there's a redundancy in 
> the logic (both  NOT name *.debuginfo AND name *.exe).  I suppose 
> there is a redundancy, but only for windows; that new clause is needed 
> only for linux/solaris.

These two blocks of make logic make me grind my teeth:

  869 $(JRE_BIN_LIST):
  870         $(RM) $@
  871 ifeq ($(PLATFORM), windows)
  872         $(FIND) $(JRE_IMAGE_DIR)/bin -type f -name \*.exe \
  873            -o -name \*.dll | $(EGREP) -v -i "$(MSVCRNN_DLL)" > $@
  874 else
  875         $(FIND) $(JRE_IMAGE_DIR)/lib -type f -name 
\*.$(LIB_SUFFIX) >> $@
  876         $(FILE) `$(FIND) $(JRE_IMAGE_DIR)/bin -type f ! -name 
'*.debuginfo' -name \*$(EXE_SUFFIX)` \
  877             | $(EGREP) 'ELF' | $(CUT) -d':' -f1 >> $@
  878 endif

1132 # Get list of binary (COFF or Elf) files in the jdk
1133 JDK_BIN_LIST=$(TEMPDIR)/jdk-bin-files.list
1134 $(JDK_BIN_LIST):
1135 ifeq ($(PLATFORM), windows)
1136         $(FIND) $(JDK_IMAGE_DIR)/jre/bin -type f -name \*.exe \
1137            -o -name \*.dll | $(EGREP) -v -i "$(MSVCRNN_DLL)" > $@
1138         $(FIND) $(JDK_IMAGE_DIR)/bin -type f -name \*.exe \
1139            -o -name \*.dll | $(EGREP) -v -i "$(MSVCRNN_DLL)" >> $@
1140 else
1141         $(RM) $@
1142         $(FIND) $(JDK_IMAGE_DIR)/jre/lib -type f -name 
\*.$(LIB_SUFFIX) >> $@
1143         $(FILE) `$(FIND) $(JDK_IMAGE_DIR)/jre/bin -type f ! -name 
'*.debuginfo' -name \*$(EXE_SUFFIX)` \
1144             | $(EGREP) 'ELF' | $(CUT) -d':' -f1 >> $@
1145         file `$(FIND) $(JDK_IMAGE_DIR)/bin -type f ! -name 
'*.debuginfo' -name \*$(EXE_SUFFIX)` \
1146             | $(EGREP) 'ELF' | $(CUT) -d':' -f1 >> $@
1147 endif


I originally made more changes here, but I backed them out and went
with the minimal changes needed to make FDS work.

It's strange that the non-Windows code (lines 876, 1143 and 1144) above
uses $(EXE_SUFFIX) which is empty on non-Windows, but it doesn't use it
on Windows. Line 876 and line 1143 could probably be rewritten to:

     $(FILE) `$(FIND) $(JRE_IMAGE_DIR)/bin -type f ! -name '*.debuginfo'` \

but I didn't want to field extra questions about why I changed that.
I'm going to add a comment above lines 876 and 1143:

# The FILE command reports .debuginfo files as "ELF", but we don't want
# those files in the JRE_BIN_LIST file. EXE_SUFFIX is empty on non-Windows.

Also, these lines work by accident:

  872         $(FIND) $(JRE_IMAGE_DIR)/bin -type f -name \*.exe \
  873            -o -name \*.dll | $(EGREP) -v -i "$(MSVCRNN_DLL)" > $@
1136         $(FIND) $(JDK_IMAGE_DIR)/jre/bin -type f -name \*.exe \
1137            -o -name \*.dll | $(EGREP) -v -i "$(MSVCRNN_DLL)" > $@
1138         $(FIND) $(JDK_IMAGE_DIR)/bin -type f -name \*.exe \
1139            -o -name \*.dll | $(EGREP) -v -i "$(MSVCRNN_DLL)" >> $@

There need to be '\(' and '\)' to bind the two '-name' options together.
Right now the '-type f' binds to the first '-name' which is then or'ed
with the second '-name'. A non-file object with '.dll' extension would
get on the list...

Dan




> -- 
> - Keith
>
> Daniel D. Daugherty wrote:
>> Greetings,
>>
>> Coming soon to a JDK repo near you! Full Debug Symbols!
>>
>> OK, to just a subset of libraries and programs... on Linux and 
>> Solaris...
>> If you're a Windows fan, the JDK repo has had Full Debug Symbols support
>> since way back in JDK1.4.1... Now we're trying get Linux and Solaris
>> caught up...
>>
>> Runtime Team, we don't have much in the JDK repo, but I tried to cover
>> our few libraries and programs. Let me know if I missed anything...
>>
>> Serviceability Team, all of your demos, libraries and programs are
>> covered... for some reason, updating those seemed like reliving old
>> times and I didn't think you'd mind... :-)
>>
>> Here is the webrev URL:
>>
>> http://cr.openjdk.java.net/~dcubed/fds_revamp/7071907-webrev/0-jdk8-jdk/
>>
>> Thanks, in advance, for any review comments.
>>
>> Dan
>>
>> P.S.
>> For those of you that are keeping track of all the FDS
>> changesets, not everything has hit the various master
>> repos yet. As a reminder, FDS has to hit the closed
>> install repo first. The open root and jdk repos along
>> with the closed deploy repo are in the second wave. And
>> the hotspot repo, being more Mercurial than his fellow
>> ghosts, will make his appearance in his own good time
>> (and via a different set of repos)...
>>
>> Apologies to Dickens, of course... :-)
>>
>>
>>