RFR: 8033580: Old debug information in IMPORT_JDK is not removed
Daniel D. Daugherty
daniel.daugherty at oracle.com
Tue Mar 25 14:32:25 UTC 2014
I'm fine if you leave your indentation as-is. This can be addressed
when the new build system style reaches the HotSpot universe...
Dan
On 3/25/14 3:41 AM, Erik Helin wrote:
> 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
>>>>>>>
>>>>>
>>>
>>
More information about the build-dev
mailing list