RFR: 8033580: Old debug information in IMPORT_JDK is not removed

[Daniel D. Daugherty]

 > http://cr.openjdk.java.net/~ehelin/8033580/webrev.03/

make/Makefile
     No comments.

Makefile style question:
     Since all this ifeq(...) logic is around Makefile rules,
     should the rules themselves be indented by one tab or by
     one tab and some number of spaces.

     I scrolled around the Makefile and I don't really see
     consistent indenting of the rule lines themselves...

     If you change the indenting, I don't see a reason for
     another round of review.

Dan


On 2/14/14 6:21 AM, Erik Helin wrote:
> Dan,
>
> On 2014-02-14 00:29, Daniel D. Daugherty wrote:
>>  > http://cr.openjdk.java.net/~ehelin/8033580/webrev.01/
>>
>> make/Makefile
>>
>> +      ifeq ($(OSNAME), bsd)
>> +            $(RM) -rf $(EXPORT_SERVER_DIR)/libjvm.dylib.dSYM
>>
>> The above needs to be:
>>
>>         ifeq ($(OS_VENDOR), Darwin)
>>               $(RM) -rf $(EXPORT_SERVER_DIR)/libjvm.dylib.dSYM
>>
>> I don't think that BSD uses .dylib stuff; that's MacOS X specific.
>
> You are right, thanks for the correction. Please see new webrev at:
> http://cr.openjdk.java.net/~ehelin/8033580/webrev.03/
>
> Again, same testing as for prior patches were run.
>
> Thanks,
> Erik
>
>> Dan
>>
>>
>> On 2/13/14 1:59 PM, Erik Helin wrote:
>>> Hi Dan,
>>>
>>> thanks for reviewing! See my replies inline.
>>>
>>> On 02/13/2014 12:40 AM, Daniel D. Daugherty wrote:
>>>>  > http://cr.openjdk.java.net/~ehelin/8033580/webrev.00/
>>>>
>>>> make/Makefile
>>>>      277   ifeq ($(ZIP_DEBUGINFO_FILES),1)
>>>>      278     ifeq ($(OSNAME), windows)
>>>>      279           $(RM) -f $(EXPORT_SERVER_DIR)/jvm.map
>>>> $(EXPORT_SERVER_DIR)/jvm.pdb
>>>>      280     else
>>>>      281           $(RM) -f $(EXPORT_SERVER_DIR)/libjvm.debuginfo
>>>>      282     endif
>>>>
>>>>      On MacOS X, the debuginfo will be in libjvm.dylib.dSYM so
>>>>      you'll need a MacOS X specific rule. You don't need an
>>>>      update for the JVM_VARIANT_CLIENT version because MacOS X
>>>>      doesn't support the Client VM, but if it did...
>>>
>>> Thanks for catching this! I've uploaded a new webrev at:
>>> http://cr.openjdk.java.net/~ehelin/8033580/webrev.01/
>>>
>>> The same testing was done as for the first patch.
>>>
>>> On 02/13/2014 12:40 AM, Daniel D. Daugherty wrote:
>>>> So the above change handles libjvm, but what about the
>>>> other libraries exported by HotSpot? libjsig, libjvm_db,
>>>> and libjvm_dtrace come to mind...
>>>
>>> As we discussed, I would like to implement this for libjvm first and
>>> then take care of the other libraries in a separate patch.
>>>
>>> Thanks,
>>> Erik
>>>
>>>> Dan
>>>>
>>>>
>>>> On 2/12/14 8:03 AM, Erik Helin wrote:
>>>>> Hi all,
>>>>>
>>>>> this patch changes how old debug information copied from 
>>>>> IMPORT_JDK is
>>>>> treated.
>>>>>
>>>>> When running the copy_*_jdk target, HotSpot's makefiles copies the
>>>>> entire IMPORT_JDK folder, including additional files (such as 
>>>>> unzipped
>>>>> debug information). The export_*_jdk targets will then, via the
>>>>> generic_export target, copy the build artifacts via implicit rules to
>>>>> the correct destination in hotspot/build/JDK_IMAGE_DIR.
>>>>>
>>>>> The bug arises when IMPORT_JDK contains unzipped debug info
>>>>> (libjvm.debuginfo) and the make target produces zipped debug info
>>>>> (libjvm.diz), or vice versa. hotspot/build/JDK_IMAGE_DIR will then
>>>>> contain both libjvm.debuginfo and libjvm.diz, but only one of them
>>>>> will match libjvm.so.
>>>>>
>>>>> Finally, the JPRT make targets jprt_build_* just zips
>>>>> hotspot/build/$(JDK_IMAGE_DIR). The zipped JPRT bundle will end up
>>>>> having different zipped and unzipped debug info, since the IMPORT_JDK
>>>>> in JPRT contains libjvm.debuginfo and we build libjvm.diz by default.
>>>>>
>>>>> This patch removes the debug info that we did *not* build. If we 
>>>>> build
>>>>> libjvm.diz, then libjvm.debuginfo will be removed (if it exists).
>>>>> Correspondingly, if we build libjvm.debuginfo, then libjvm.diz 
>>>>> will be
>>>>> removed (if it exists).
>>>>>
>>>>> Patch:
>>>>> http://cr.openjdk.java.net/~ehelin/8033580/webrev.00/
>>>>>
>>>>> Bug:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8033580
>>>>>
>>>>> Testing:
>>>>> - Building in JPRT
>>>>> - Building Linux 32-bit locally on a Linux 64-bit machine
>>>>> - Building Linux 64-bit locally on a Linux 64-bit machine
>>>>>
>>>>> For all of the above, verify that only the correct debug info is
>>>>> present in the output.
>>>>>
>>>>> Thanks,
>>>>> Erik
>>>>
>>