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

Tue Mar 25 09:41:40 UTC 2014

Daniel, Erik

thanks for the reviews and sorry taking so long to respond.

About the indentation: I understand the convention used in the new build 
system and I think it makes a lot of sense, but the file 
hotspot/make/Makefile clearly does not use this convention :)

I would prefer to use the same kind of indentation as the rest of the 
file, even though as Dan noted, the file is not consistent with itself. 
It seems like most of the file is using 2 spaces for ifeq (something) 
and then just <tab> or <tab+2 spaces> for recipes depending on logical 
level.

Dan and Erik, are you fine with keeping the indentation as it is or do 
you prefer that I update the change to use the convention of the new 
build system?

Thanks,
Erik

On 02/17/2014 09:09 AM, Erik Joelsson wrote:
> 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
>>>>>>
>>>>
>>
>