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

Thu Dec 10 23:47:12 UTC 2015

Hi build folks,

Could you review it?

>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8036003/webrev.06/

Thanks,

Yasumasa

On 2015/12/09 13:12, David Holmes wrote:
> 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
>>>>>