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

[Erik Joelsson]

Hello,

The change looks good to me too.

Regarding indentation, for the rest of the JDK, we indent rules with 
make ifeqs like this:

target: sources
<8 spaces>ifeq (something)
<tab+2 spaces>recipe line
<8 spaces>endif

Our reasoning is that it should still look like a recipe when glancing 
over it quickly (so 8 characters base indentation). After that we apply 
our standard 2 chars per logical level. Since make regards tab 
characters as special, we have to use space for non recipe lines.

The rest of our guidelines can be found here:
http://openjdk.java.net/groups/build/doc/code-conventions.html

/Erik

On 2014-02-14 17:45, Daniel D. Daugherty wrote:
> > 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
>>>>>
>>>
>