RFR: JDK-8036003: Add --with-debug-symbols=[none|internal|external|zipped]

Wed Dec 9 04:12:39 UTC 2015

Hi,

On 9/12/2015 10:31 AM, Yasumasa Suenaga wrote:
> Hi,
>
> I've uploaded a new webrev:
>    http://cr.openjdk.java.net/~ysuenaga/JDK-8036003/webrev.06/
>
>
>> Can you reinstate the comment:
>>
>>   581   # This must be done after the toolchain is setup, since we're
>> looking at objcopy.
>>
>> as this constraint still exists.
>
> I've fixed.
>
>> I would have hoped that the deprecation macro enabled you to inform
>> the user what the replacement is as per the comments! This seems
>> unsatisfactory. :(
>
> I added one more argument to BASIC_DEPRECATED_ARG_ENABLE to show message.
> However, I'm not certain this change should be filed another issue or not.

This seems good to me - and small enough to not warrant another issue.

But ultimately it is up to the build folk.

I have no further comments.

Thanks,
David

>
> Thanks,
>
> Yasumasa
>
>
> On 2015/12/03 15:51, David Holmes wrote:
>> On 1/12/2015 12:34 AM, Yasumasa Suenaga wrote:
>>> Hi Magnus,
>>>
>>> I've uploaded new webrev:
>>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8036003/webrev.05/
>>
>> common/autoconf/jdk-options.m4
>>
>> Can you reinstate the comment:
>>
>>   581   # This must be done after the toolchain is setup, since we're
>> looking at objcopy.
>>
>> as this constraint still exists.
>>
>> It would be nice if the external and zipped cases could somehow be
>> shared so that we don't have to duplicate everything just to alter the
>> value of a single variable.
>>
>>   634   # --enable-zip-debug-info is deprecated.
>>   635   # Please use --with-debug-symbols=zipped .
>>   636   BASIC_DEPRECATED_ARG_ENABLE(zip-debug-info, zip_debug_info)
>>
>> I would have hoped that the deprecation macro enabled you to inform
>> the user what the replacement is as per the comments! This seems
>> unsatisfactory. :(
>>
>> I'm assuming there is no change to the default behaviour if none of
>> these options are specified. I'm unclear whether we will need to
>> coordinate this change with any internal processes that might
>> explicitly be using the deprecated options - I think Magnus should
>> have a better idea about that.
>>
>> Thanks,
>> David
>> -----
>>
>>>> If we do implement this flag, it should work as expected. If you
>>>> believe this is too hard to get right, I'm alright with fixing this as
>>>> a separate bug.
>>>
>>> Okay, I removed --enable-java-debug-symbols in this webrev.
>>> I will file to JBS and try to create patch for it after this issue.
>>>
>>>
>>>> That is, have you tried building with the four different values of
>>>> --with-native-debug-symbols and verified that the result is indeed as
>>>> you specified?
>>>
>>> Of course, I've checked all pattern of --with-native-debug-symbols.
>>> I ran readelf -S for libjvm, libnio, libsaproc, and java command.
>>> I checked debug section (.debug_info, and more) in ELF binaries.
>>>
>>>
>>>> Just by my quick glance I can already spot what I believe is a
>>>> mistake: for "none" you set STRIP_POLICY to min_strip, but it should
>>>> reasonably be no_strip.
>>>
>>> I've fixed in this webrev.
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> On 2015/11/30 21:26, Magnus Ihse Bursie wrote:
>>>>
>>>>
>>>> On 2015-11-26 15:19, Yasumasa Suenaga wrote:
>>>>> I uploaded a new webrev:
>>>>>   http://cr.openjdk.java.net/~ysuenaga/JDK-8036003/webrev.04/
>>>>>
>>>>> I implemented --with-native-debug-symbols and
>>>>> --enable-java-debug-symbols in it.
>>>>> And I deprecated --enable-debug-symbols and --enable-zip-debug-info .
>>>>
>>>> Hi Yasumasa,
>>>>
>>>> I apologize I have not been able to spend more time on this. I still
>>>> only had time to take a short glimpse, but I found a few issues anyway.
>>>>
>>>> First, the new -enable-java-debug-symbols is problematic. If you build
>>>> with "--enable-debug --disable-java-debug-symbols", then this implies
>>>> that you would get a build without java debug symbols. However, this
>>>> will not happen, since changing debug level wil enable java debug
>>>> symbols, but the new java-debug-symbols flag will not manage to
>>>> disable this.
>>>>
>>>> If we do implement this flag, it should work as expected. If you
>>>> believe this is too hard to get right, I'm alright with fixing this as
>>>> a separate bug.
>>>>
>>>> Second, all the flags we use to manage this internally makes me
>>>> nervous. :) Are you sure you're getting them right? That is, have you
>>>> tried building with the four different values of
>>>> --with-native-debug-symbols and verified that the result is indeed as
>>>> you specified? You'd need to check both libjvm.so and at least a jdk
>>>> library, like libjava.so or so, since they use different build system
>>>> and flags.
>>>>
>>>> Just by my quick glance I can already spot what I believe is a
>>>> mistake: for "none" you set STRIP_POLICY to min_strip, but it should
>>>> reasonably be no_strip. (I'd be surprised if strip was run if I
>>>> compiled without debug symbols).
>>>>
>>>> Other than that, I believe this patch is starting to get ready.
>>>>
>>>> /Magnus
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Yasumasa
>>>>>
>>>>>
>>>>> On 2015/11/25 23:52, Yasumasa Suenaga wrote:
>>>>>> Hi Magnus,
>>>>>>
>>>>>>> --with-java-debug-symbols=none,all
>>>>>>> (or "none,internal" for consistency).
>>>>>>
>>>>>> Can I change this option to --enable-java-debug-symbols ?
>>>>>> This value set to none or all. So I think that we should use
>>>>>> AC_ARG_ENABLE for it.
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Yasumasa
>>>>>>
>>>>>>
>>>>>> On 2015/11/25 23:35, Magnus Ihse Bursie wrote:
>>>>>>> On 2015-11-21 14:12, Yasumasa Suenaga wrote:
>>>>>>>>> My point with current proposal is that either it doesn't touch
>>>>>>>>> the zipping because we already have an option for that; or it
>>>>>>>>> should deprecate the existing option (now Erik has pointed out
>>>>>>>>> that capability).
>>>>>>>>
>>>>>>>> I'm using BASIC_DEPRECATED_ARG_ENABLE for debug-symbols and
>>>>>>>> zip-debug-info in new webrev:
>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8036003/webrev.03/
>>>>>>>>
>>>>>>>> I had thought this new option as a wrapper for existing options.
>>>>>>>> However, if current options should be deprecated, I will follow it.
>>>>>>>
>>>>>>> Hi Yasumasa,
>>>>>>>
>>>>>>> Sorry for the delay in my response.
>>>>>>>
>>>>>>> I had to go back and re-read the old discussions and my personal
>>>>>>> notes about this. It turned out that some of the complications that
>>>>>>> existed then is not relevant from Oracle's point of view anymore
>>>>>>> (more specifically, stripping to several distinct levels). With
>>>>>>> that complication gone, the basic case boils down to the three ways
>>>>>>> of doing this:
>>>>>>> 1) Compile without debug information ("none")
>>>>>>> 2) Compile with debug information and keep it ("internal")
>>>>>>> 3) Compile with debug information, copy it to an external file, and
>>>>>>> strip the binary ("external")
>>>>>>>
>>>>>>> So far, so good. I agree with David Holmes that the zipping
>>>>>>> complicates things somewhat. The problem here is two-fold: Firstly,
>>>>>>> I believe that the "external" choice will be relevant to nobody (as
>>>>>>> in, neither the community nor Oracle), and secondly, there have
>>>>>>> been discussions of handling the external debuginfo differently
>>>>>>> (namely, to move all debug info, unzipped, into a separate image,
>>>>>>> and zip them all at once). It's a bit unfortunate if we create an
>>>>>>> interface to configure that cannot grow properly with future
>>>>>>> changes.
>>>>>>>
>>>>>>> On the other hand, it does seem convenient to express the end
>>>>>>> result for the Oracle build as "zipped" (or possibly
>>>>>>> "external-zipped", to emphasize  the connection to "external"?).
>>>>>>>
>>>>>>> So I'm not sure what to think myself. But if David agrees that
>>>>>>> adding "zipped" (or "external-zipped") to the new argument, and
>>>>>>> deprecating the old, then I agree with it as well. :-)
>>>>>>>
>>>>>>> The second discussion here is the connection between java and
>>>>>>> native debug symbols.
>>>>>>>
>>>>>>> Fact: We do not currently have a story for setting java debug
>>>>>>> symbols. That's bad, and a regression as pointed out.
>>>>>>> Fact: That does not mean it has to be fixed in this patch.
>>>>>>>
>>>>>>> I do think that java and native debug symbols are different things
>>>>>>> and should be treated differently. But I also think we should have
>>>>>>> configure arguments that show their similarity. So, I propose that
>>>>>>> we have a
>>>>>>> --with-native-debug-symbols=none,internal,external,zipped
>>>>>>> as you propose, and then also a
>>>>>>> --with-java-debug-symbols=none,all
>>>>>>> (or "none,internal" for consistency).
>>>>>>>
>>>>>>> I think it would be nice if that flag could be implemented in the
>>>>>>> same patch, but since it is a different thing I understand if you
>>>>>>> want to leave it out. There is some added complication here, since
>>>>>>> we already set -g for java compilation depending on debug level,
>>>>>>> and the native debug symbol patch is obviously tricky as it is.
>>>>>>>
>>>>>>> /Magnus
>>>>