RFR: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries

classic Classic list List threaded Threaded
43 messages Options
123
Reply | Threaded
Open this post in threaded view
|

RFR: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries

Severin Gehwolf
Hi,

Could I please get reviews for this enhancement? It adds a debug
symbols stripping plug-in to jlink for Linux and Unix platforms. It's
the first platform specific jlink plugin and the approach taken for
keeping it contained is to use a plugin specific ResourceBundle.
Discussion for this happened in [1].

The test uses a native library which should never get debug symbols
stripped during the test library build. As such, tiny modifications
were needed to make/common/TestFilesCompilation.gmk. Hence, build-dev
being on the list for this RFR. The test currently only runs on Linux
and requires objcopy to be available. Otherwise the test is being
skipped.

Example usage of this plugin is described in the bug.

webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/04/webrev/
Bug: https://bugs.openjdk.java.net/browse/JDK-8214796

Testing: test/jdk/tools/jlink test/jdk/jdk/modules tests on Linux
x86_64 (with good and broken objcopy) and the newly added test. It's
currently running through jdk/submit too.

Thoughts?

Thanks,
Severin

[1] http://mail.openjdk.java.net/pipermail/jigsaw-dev/2019-January/014109.html

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries

Erik Joelsson
Hello Severin,

There is a macro for automatically finding all source dirs for a module.
So in Gensrc-jdk.jlink.gmk, I think it would be better expressed using
that macro, like this:

JLINK_RESOURCE_DIRS := $(wildcard $(addsuffix
/jdk/tools/jlink/resources, $(call FindModuleSrcSdirs, jdk.jlink)))

The above could/should even be inlined.

Otherwise build changes look ok.

/Erik

On 2019-02-07 09:09, Severin Gehwolf wrote:

> Hi,
>
> Could I please get reviews for this enhancement? It adds a debug
> symbols stripping plug-in to jlink for Linux and Unix platforms. It's
> the first platform specific jlink plugin and the approach taken for
> keeping it contained is to use a plugin specific ResourceBundle.
> Discussion for this happened in [1].
>
> The test uses a native library which should never get debug symbols
> stripped during the test library build. As such, tiny modifications
> were needed to make/common/TestFilesCompilation.gmk. Hence, build-dev
> being on the list for this RFR. The test currently only runs on Linux
> and requires objcopy to be available. Otherwise the test is being
> skipped.
>
> Example usage of this plugin is described in the bug.
>
> webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/04/webrev/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8214796
>
> Testing: test/jdk/tools/jlink test/jdk/jdk/modules tests on Linux
> x86_64 (with good and broken objcopy) and the newly added test. It's
> currently running through jdk/submit too.
>
> Thoughts?
>
> Thanks,
> Severin
>
> [1] http://mail.openjdk.java.net/pipermail/jigsaw-dev/2019-January/014109.html
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries

Severin Gehwolf
Hi Erik,

On Thu, 2019-02-07 at 09:39 -0800, Erik Joelsson wrote:

> Hello Severin,
>
> There is a macro for automatically finding all source dirs for a module.
> So in Gensrc-jdk.jlink.gmk, I think it would be better expressed using
> that macro, like this:
>
> JLINK_RESOURCE_DIRS := $(wildcard $(addsuffix
> /jdk/tools/jlink/resources, $(call FindModuleSrcSdirs, jdk.jlink)))
>
> The above could/should even be inlined.

I've considered this. It seems, though, that FindModuleSrcDirs comes
from make/common/Modules.gmk which isn't included in
make/gensrc/Gensrc-jdk.jlink.gmk. Given that it has already caused
problems with multiple includes of Modules.gmk (JDK-8213736) I was
reluctant to include it here too. Without the new include the above
won't work.

The approach I've taken here seems to be the lesser evil. Thoughts?

Thanks,
Severin

> Otherwise build changes look ok.
>
> /Erik
>
> On 2019-02-07 09:09, Severin Gehwolf wrote:
> > Hi,
> >
> > Could I please get reviews for this enhancement? It adds a debug
> > symbols stripping plug-in to jlink for Linux and Unix platforms. It's
> > the first platform specific jlink plugin and the approach taken for
> > keeping it contained is to use a plugin specific ResourceBundle.
> > Discussion for this happened in [1].
> >
> > The test uses a native library which should never get debug symbols
> > stripped during the test library build. As such, tiny modifications
> > were needed to make/common/TestFilesCompilation.gmk. Hence, build-dev
> > being on the list for this RFR. The test currently only runs on Linux
> > and requires objcopy to be available. Otherwise the test is being
> > skipped.
> >
> > Example usage of this plugin is described in the bug.
> >
> > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/04/webrev/
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8214796
> >
> > Testing: test/jdk/tools/jlink test/jdk/jdk/modules tests on Linux
> > x86_64 (with good and broken objcopy) and the newly added test. It's
> > currently running through jdk/submit too.
> >
> > Thoughts?
> >
> > Thanks,
> > Severin
> >
> > [1] http://mail.openjdk.java.net/pipermail/jigsaw-dev/2019-January/014109.html
> >

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries

Mandy Chung
In reply to this post by Severin Gehwolf
Hi Severin,

Using the plugin specific ResourceBundle is good.  Thanks for making
the change.

I see that Alan's comment [1] on the plugin options and I assume
you will investigate the plugin options and bring back a proposal.
Did I miss the discussion on these options?

Option:
--strip-native-debug-symbols=<defaults|options[:objcopy-cmd=<path/to/objcopycmd>][:debuginfo-file-ext=<ext>][:include-debug-syms=true]>

Examples: --strip-native-debug-symbols=defaults

I suggest the above be simplified and drop the "=defaults".  Simply:
    --strip-native-debug-symbols

Examples:
--strip-native-debug-symbols=options:objcopy-cmd=/usr/bin/objcopy:debuginfo-file-ext=dbg:include-debug-syms=true


If include-debug-syms=true then it will run
   objcopy --only-keep-debug foo foo.<ext> to create debug symbols file
   objcopy --add-gnu-debuglink=foo.dbg foo

So what about simplifying the options to:
 
--strip-native-debug-symbols=keep-debug-symbols=dbg,objcopy-path=/usr/bin/objcopy

We could also drop the word "symbols":

 
--strip-native-debug=[keep-debug=<debug-file-ext>,][objcopy-path=<path-of-objcopy>]

By default, `--strip-native-debug` will strip native debug symbols
and don't keep debug symbols.

keep-debug=<debug-file-ext> creates the debug symbols file.
<debug-file-ext> specifies the file extension.  It would be
nice to use the default `debuginfo` extension and simply
accept `keep-debug` option.

`objcopy-cmd` may be okay too.  Others may have opinion.

I think we should agree on the plugin options first before
doing the code review.

W.r.t. the test:

 > The test currently only runs on Linux
 > and requires objcopy to be available. Otherwise the test is being
 > skipped.

We can create a fake objcopy utility for testing purpose
such that the test will validate if the options are passed
properly to the test utility.

 > webrev:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/04/webrev/

I haven't reviewed the new files.  Just notice that very long
lines in the new files that you may want to fix the formatting
that will help further side-by-side review.

The --list-plugin output is very very long.  The existing plugins
didn't set a good example to keep the well formatted (I recorded
that they were cleaned up at one point to keep 80 chars width).

One other question: should this plugin be moved to linux/classes
rather than unix/classes given that that's the platform it
intends to support?

Mandy
[1]
http://mail.openjdk.java.net/pipermail/jigsaw-dev/2019-January/014099.html

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries

Erik Joelsson
In reply to this post by Severin Gehwolf

On 2019-02-07 11:09, Severin Gehwolf wrote:

> Hi Erik,
>
> On Thu, 2019-02-07 at 09:39 -0800, Erik Joelsson wrote:
>> Hello Severin,
>>
>> There is a macro for automatically finding all source dirs for a module.
>> So in Gensrc-jdk.jlink.gmk, I think it would be better expressed using
>> that macro, like this:
>>
>> JLINK_RESOURCE_DIRS := $(wildcard $(addsuffix
>> /jdk/tools/jlink/resources, $(call FindModuleSrcSdirs, jdk.jlink)))
>>
>> The above could/should even be inlined.
> I've considered this. It seems, though, that FindModuleSrcDirs comes
> from make/common/Modules.gmk which isn't included in
> make/gensrc/Gensrc-jdk.jlink.gmk. Given that it has already caused
> problems with multiple includes of Modules.gmk (JDK-8213736) I was
> reluctant to include it here too. Without the new include the above
> won't work.
>
> The approach I've taken here seems to be the lesser evil. Thoughts?

I appreciate your concern, but JDK-8213736 was a problem in Main.gmk,
which is part of where Modules.gmk gets bootstrapped. In any normal
makefile (as in where stuff is just being built), the bootstrapping is
done and including Modules.gmk is completely fine. Any
<phase>-<module>.gmk file certainly qualifies here.

/Erik

> Thanks,
> Severin
>
>> Otherwise build changes look ok.
>>
>> /Erik
>>
>> On 2019-02-07 09:09, Severin Gehwolf wrote:
>>> Hi,
>>>
>>> Could I please get reviews for this enhancement? It adds a debug
>>> symbols stripping plug-in to jlink for Linux and Unix platforms. It's
>>> the first platform specific jlink plugin and the approach taken for
>>> keeping it contained is to use a plugin specific ResourceBundle.
>>> Discussion for this happened in [1].
>>>
>>> The test uses a native library which should never get debug symbols
>>> stripped during the test library build. As such, tiny modifications
>>> were needed to make/common/TestFilesCompilation.gmk. Hence, build-dev
>>> being on the list for this RFR. The test currently only runs on Linux
>>> and requires objcopy to be available. Otherwise the test is being
>>> skipped.
>>>
>>> Example usage of this plugin is described in the bug.
>>>
>>> webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/04/webrev/
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8214796
>>>
>>> Testing: test/jdk/tools/jlink test/jdk/jdk/modules tests on Linux
>>> x86_64 (with good and broken objcopy) and the newly added test. It's
>>> currently running through jdk/submit too.
>>>
>>> Thoughts?
>>>
>>> Thanks,
>>> Severin
>>>
>>> [1] http://mail.openjdk.java.net/pipermail/jigsaw-dev/2019-January/014109.html
>>>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries

Alan Bateman
In reply to this post by Severin Gehwolf
On 07/02/2019 17:09, Severin Gehwolf wrote:

> Hi,
>
> Could I please get reviews for this enhancement? It adds a debug
> symbols stripping plug-in to jlink for Linux and Unix platforms. It's
> the first platform specific jlink plugin and the approach taken for
> keeping it contained is to use a plugin specific ResourceBundle.
> Discussion for this happened in [1].
>
> The test uses a native library which should never get debug symbols
> stripped during the test library build. As such, tiny modifications
> were needed to make/common/TestFilesCompilation.gmk. Hence, build-dev
> being on the list for this RFR. The test currently only runs on Linux
> and requires objcopy to be available. Otherwise the test is being
> skipped.
>
> Example usage of this plugin is described in the bug.
>
> webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/04/webrev/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8214796
>
I agree with Mandy that the CLI options need examination. The proposed
`--strip-native-debug-symbols` seems reasonable but it will be
inconsistent with the existing `--strip-debug` option. Mandy - what you
would think about renaming the existing option to something that makes
it clear that it strips the debug attribute from class files? (we would
of course need to do something to keep the existing option working).

The need to specify the argument as "defaults" or "options" is a bit
annoying. It might be time to replace hasArguments in the plugin
interface to allow for optional arguments. The plugin interface is not
exported/documented/supported so we have flexibility to change it. If we
do this then it should mean the usages reduce down to:

--strip-native-debug-symbols
--strip-native-debug-symbols objcopy=<path-to-objcopy>:...

I see the plugin has moved to src/jdk.jlink/unix in this iteration. It
might be better to start out in src/jdk.jlink/linux - we can always move
to the unix tree in the event that there is interest/support on other
platforms.

Mandy points out the issue with wrapping in the usage output. I agree
that the`jlink --list-plugins` needs to be readable/tidy esp. in this
case as there are many sub-options to read about.

The implementation will need a bit of a tidy up too. The readFileBytes
method can be replaced with Files.readAllBytes. The BAIS usage can be
replaced with Files.write. Ignoring exceptions thrown in stripBinary
will also need consideration.

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries

Mandy Chung


On 2/8/19 2:08 AM, Alan Bateman wrote:
> I agree with Mandy that the CLI options need examination. The proposed
> `--strip-native-debug-symbols` seems reasonable but it will be
> inconsistent with the existing `--strip-debug` option. Mandy - what you
> would think about renaming the existing option to something that makes
> it clear that it strips the debug attribute from class files? (we would
> of course need to do something to keep the existing option working).

Renaming it to make it clear is good.  How about:

--strip-debug-attribute
--strip-native-debug-symbols

--strip-debug will invoke both --strip-debug-attribute and
--strip-native-debug-symbols, if supported.

Typically we want to strip both.  If only stripping debug attribute,
then it can use --strip-debug-attribute.

What do you think?

> The need to specify the argument as "defaults" or "options" is a bit
> annoying. It might be time to replace hasArguments in the plugin  > interface to allow for optional arguments.

Hmm... I thought it allows optional arguments.  LegalNoticeFilePlugin
accepts an optional argument.

On the other hand, pluginToMaps will put an empty map if hasArguments
return false.  The plugin processing code (PluginsHelper) is quite
complicated that I can't quite tell without spending time.

In any case I think a plugin should support optional arguments.

> The plugin interface is not
> exported/documented/supported so we have flexibility to change it. If we
> do this then it should mean the usages reduce down to:
>
> --strip-native-debug-symbols
> --strip-native-debug-symbols objcopy=<path-to-objcopy>:...

objcopy is fine.

It would also be nice to allow `keep-debuginfo` taking an optional
file extension.

--strip-native-debug-symbols keep-debuginfo
--strip-native-debug-symbols keep-debuginfo=dbg

>
> I see the plugin has moved to src/jdk.jlink/unix in this iteration. It
> might be better to start out in src/jdk.jlink/linux - we can always move
> to the unix tree in the event that there is interest/support on other
> platforms.

Agree.

> Mandy points out the issue with wrapping in the usage output. I agree
> that the`jlink --list-plugins` needs to be readable/tidy esp. in this
> case as there are many sub-options to read about.

I file JDK-8218685 for --list-plugins tidy up.


Mandy
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries

Alan Bateman
On 08/02/2019 18:53, Mandy Chung wrote:

>
> Renaming it to make it clear is good.  How about:
>
> --strip-debug-attribute
> --strip-native-debug-symbols
>
> --strip-debug will invoke both --strip-debug-attribute and
> --strip-native-debug-symbols, if supported.
>
> Typically we want to strip both.  If only stripping debug attribute,
> then it can use --strip-debug-attribute.
>
> What do you think?
This make sense and I'm just wondering if we need to extend this to
debug symbols that are in external files. This suggests exploring
whether it should imply --exclude-files too.


> :
>
> Hmm... I thought it allows optional arguments. LegalNoticeFilePlugin
> accepts an optional argument.
I don't think --dedup-legal-notices works without an option. It would be
generally useful to allow optional values.

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries

Severin Gehwolf
In reply to this post by Erik Joelsson
Hi Erik,

On Thu, 2019-02-07 at 17:01 -0800, Erik Joelsson wrote:

> On 2019-02-07 11:09, Severin Gehwolf wrote:
> > Hi Erik,
> >
> > On Thu, 2019-02-07 at 09:39 -0800, Erik Joelsson wrote:
> > > Hello Severin,
> > >
> > > There is a macro for automatically finding all source dirs for a module.
> > > So in Gensrc-jdk.jlink.gmk, I think it would be better expressed using
> > > that macro, like this:
> > >
> > > JLINK_RESOURCE_DIRS := $(wildcard $(addsuffix
> > > /jdk/tools/jlink/resources, $(call FindModuleSrcSdirs, jdk.jlink)))
> > >
> > > The above could/should even be inlined.
> > I've considered this. It seems, though, that FindModuleSrcDirs comes
> > from make/common/Modules.gmk which isn't included in
> > make/gensrc/Gensrc-jdk.jlink.gmk. Given that it has already caused
> > problems with multiple includes of Modules.gmk (JDK-8213736) I was
> > reluctant to include it here too. Without the new include the above
> > won't work.
> >
> > The approach I've taken here seems to be the lesser evil. Thoughts?
>
> I appreciate your concern, but JDK-8213736 was a problem in Main.gmk,
> which is part of where Modules.gmk gets bootstrapped. In any normal
> makefile (as in where stuff is just being built), the bootstrapping is
> done and including Modules.gmk is completely fine. Any
> <phase>-<module>.gmk file certainly qualifies here.

OK. Updated:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/05/

Thanks,
Severin

> /Erik
>
> > Thanks,
> > Severin
> >
> > > Otherwise build changes look ok.
> > >
> > > /Erik
> > >
> > > On 2019-02-07 09:09, Severin Gehwolf wrote:
> > > > Hi,
> > > >
> > > > Could I please get reviews for this enhancement? It adds a
> > > > debug
> > > > symbols stripping plug-in to jlink for Linux and Unix
> > > > platforms. It's
> > > > the first platform specific jlink plugin and the approach taken
> > > > for
> > > > keeping it contained is to use a plugin specific
> > > > ResourceBundle.
> > > > Discussion for this happened in [1].
> > > >
> > > > The test uses a native library which should never get debug
> > > > symbols
> > > > stripped during the test library build. As such, tiny
> > > > modifications
> > > > were needed to make/common/TestFilesCompilation.gmk. Hence,
> > > > build-dev
> > > > being on the list for this RFR. The test currently only runs on
> > > > Linux
> > > > and requires objcopy to be available. Otherwise the test is
> > > > being
> > > > skipped.
> > > >
> > > > Example usage of this plugin is described in the bug.
> > > >
> > > > webrev:
> > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/04/webrev/
> > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8214796
> > > >
> > > > Testing: test/jdk/tools/jlink test/jdk/jdk/modules tests on
> > > > Linux
> > > > x86_64 (with good and broken objcopy) and the newly added test.
> > > > It's
> > > > currently running through jdk/submit too.
> > > >
> > > > Thoughts?
> > > >
> > > > Thanks,
> > > > Severin
> > > >
> > > > [1]
> > > > http://mail.openjdk.java.net/pipermail/jigsaw-dev/2019-January/014109.html
> > > >

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries

Severin Gehwolf
In reply to this post by Alan Bateman
Hi Alan,

Thanks for the review!

On Fri, 2019-02-08 at 10:08 +0000, Alan Bateman wrote:

> On 07/02/2019 17:09, Severin Gehwolf wrote:
> > Hi,
> >
> > Could I please get reviews for this enhancement? It adds a debug
> > symbols stripping plug-in to jlink for Linux and Unix platforms. It's
> > the first platform specific jlink plugin and the approach taken for
> > keeping it contained is to use a plugin specific ResourceBundle.
> > Discussion for this happened in [1].
> >
> > The test uses a native library which should never get debug symbols
> > stripped during the test library build. As such, tiny modifications
> > were needed to make/common/TestFilesCompilation.gmk. Hence, build-dev
> > being on the list for this RFR. The test currently only runs on Linux
> > and requires objcopy to be available. Otherwise the test is being
> > skipped.
> >
> > Example usage of this plugin is described in the bug.
> >
> > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/04/webrev/
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8214796
> >
> I agree with Mandy that the CLI options need examination. The proposed
> `--strip-native-debug-symbols` seems reasonable but it will be
> inconsistent with the existing `--strip-debug` option. Mandy - what you
> would think about renaming the existing option to something that makes
> it clear that it strips the debug attribute from class files? (we would
> of course need to do something to keep the existing option working).
>
> The need to specify the argument as "defaults" or "options" is a bit
> annoying. It might be time to replace hasArguments in the plugin
> interface to allow for optional arguments. The plugin interface is not
> exported/documented/supported so we have flexibility to change it.

I've filed this bug for optional arguments:
https://bugs.openjdk.java.net/browse/JDK-8218761

It's a separate issue, after all.

> If we
> do this then it should mean the usages reduce down to:
>
> --strip-native-debug-symbols
> --strip-native-debug-symbols objcopy=<path-to-objcopy>:...
>
> I see the plugin has moved to src/jdk.jlink/unix in this iteration. It
> might be better to start out in src/jdk.jlink/linux - we can always move
> to the unix tree in the event that there is interest/support on other
> platforms.
>
> Mandy points out the issue with wrapping in the usage output. I agree
> that the`jlink --list-plugins` needs to be readable/tidy esp. in this
> case as there are many sub-options to read about.

I'll tackle those once there is some agreement as to what the option
should be called and which arguments it should have for the initial
implementation. I'll reply with a proposal in the other thread.

> The implementation will need a bit of a tidy up too. The readFileBytes
> method can be replaced with Files.readAllBytes. The BAIS usage can be
> replaced with Files.write. Ignoring exceptions thrown in stripBinary
> will also need consideration.

Should be fixed in the latest webrev:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/05/webrev/

FWIW, I've refrained from using Files.readAllBytes in the initial
webrev because of this note in the javadoc:

"""
Note that this method is intended for simple cases where it is
convenient to read all bytes into a byte array. It is not intended for
reading in large files.
"""

Cheers,
Severin

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries

Erik Joelsson
In reply to this post by Severin Gehwolf
Hello Severin,

I think you also need a $(wildcard ) around it, but I may be wrong. Did
you try this version?

Also, please do not indent so much. We have style guidelines [1], which
recommend 4 spaces for line break indentation.

/Erik

[1] http://openjdk.java.net/groups/build/doc/code-conventions.html

On 2019-02-11 10:03, Severin Gehwolf wrote:

> Hi Erik,
>
> On Thu, 2019-02-07 at 17:01 -0800, Erik Joelsson wrote:
>> On 2019-02-07 11:09, Severin Gehwolf wrote:
>>> Hi Erik,
>>>
>>> On Thu, 2019-02-07 at 09:39 -0800, Erik Joelsson wrote:
>>>> Hello Severin,
>>>>
>>>> There is a macro for automatically finding all source dirs for a module.
>>>> So in Gensrc-jdk.jlink.gmk, I think it would be better expressed using
>>>> that macro, like this:
>>>>
>>>> JLINK_RESOURCE_DIRS := $(wildcard $(addsuffix
>>>> /jdk/tools/jlink/resources, $(call FindModuleSrcSdirs, jdk.jlink)))
>>>>
>>>> The above could/should even be inlined.
>>> I've considered this. It seems, though, that FindModuleSrcDirs comes
>>> from make/common/Modules.gmk which isn't included in
>>> make/gensrc/Gensrc-jdk.jlink.gmk. Given that it has already caused
>>> problems with multiple includes of Modules.gmk (JDK-8213736) I was
>>> reluctant to include it here too. Without the new include the above
>>> won't work.
>>>
>>> The approach I've taken here seems to be the lesser evil. Thoughts?
>> I appreciate your concern, but JDK-8213736 was a problem in Main.gmk,
>> which is part of where Modules.gmk gets bootstrapped. In any normal
>> makefile (as in where stuff is just being built), the bootstrapping is
>> done and including Modules.gmk is completely fine. Any
>> <phase>-<module>.gmk file certainly qualifies here.
> OK. Updated:
> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/05/
>
> Thanks,
> Severin
>
>> /Erik
>>
>>> Thanks,
>>> Severin
>>>
>>>> Otherwise build changes look ok.
>>>>
>>>> /Erik
>>>>
>>>> On 2019-02-07 09:09, Severin Gehwolf wrote:
>>>>> Hi,
>>>>>
>>>>> Could I please get reviews for this enhancement? It adds a
>>>>> debug
>>>>> symbols stripping plug-in to jlink for Linux and Unix
>>>>> platforms. It's
>>>>> the first platform specific jlink plugin and the approach taken
>>>>> for
>>>>> keeping it contained is to use a plugin specific
>>>>> ResourceBundle.
>>>>> Discussion for this happened in [1].
>>>>>
>>>>> The test uses a native library which should never get debug
>>>>> symbols
>>>>> stripped during the test library build. As such, tiny
>>>>> modifications
>>>>> were needed to make/common/TestFilesCompilation.gmk. Hence,
>>>>> build-dev
>>>>> being on the list for this RFR. The test currently only runs on
>>>>> Linux
>>>>> and requires objcopy to be available. Otherwise the test is
>>>>> being
>>>>> skipped.
>>>>>
>>>>> Example usage of this plugin is described in the bug.
>>>>>
>>>>> webrev:
>>>>> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/04/webrev/
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8214796
>>>>>
>>>>> Testing: test/jdk/tools/jlink test/jdk/jdk/modules tests on
>>>>> Linux
>>>>> x86_64 (with good and broken objcopy) and the newly added test.
>>>>> It's
>>>>> currently running through jdk/submit too.
>>>>>
>>>>> Thoughts?
>>>>>
>>>>> Thanks,
>>>>> Severin
>>>>>
>>>>> [1]
>>>>> http://mail.openjdk.java.net/pipermail/jigsaw-dev/2019-January/014109.html
>>>>>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries

Severin Gehwolf
Hi Erik,

On Mon, 2019-02-11 at 11:12 -0800, Erik Joelsson wrote:
> Hello Severin,
>
> I think you also need a $(wildcard ) around it, but I may be wrong. Did
> you try this version?

Yes, this version works for me without $(wildcard). Why is it needed?

> Also, please do not indent so much. We have style guidelines [1], which
> recommend 4 spaces for line break indentation.

OK, sorry. Fixed locally.

Thanks,
Severin

> /Erik
>
> [1] http://openjdk.java.net/groups/build/doc/code-conventions.html
>
> On 2019-02-11 10:03, Severin Gehwolf wrote:
> > Hi Erik,
> >
> > On Thu, 2019-02-07 at 17:01 -0800, Erik Joelsson wrote:
> > > On 2019-02-07 11:09, Severin Gehwolf wrote:
> > > > Hi Erik,
> > > >
> > > > On Thu, 2019-02-07 at 09:39 -0800, Erik Joelsson wrote:
> > > > > Hello Severin,
> > > > >
> > > > > There is a macro for automatically finding all source dirs for a module.
> > > > > So in Gensrc-jdk.jlink.gmk, I think it would be better expressed using
> > > > > that macro, like this:
> > > > >
> > > > > JLINK_RESOURCE_DIRS := $(wildcard $(addsuffix
> > > > > /jdk/tools/jlink/resources, $(call FindModuleSrcSdirs, jdk.jlink)))
> > > > >
> > > > > The above could/should even be inlined.
> > > > I've considered this. It seems, though, that FindModuleSrcDirs comes
> > > > from make/common/Modules.gmk which isn't included in
> > > > make/gensrc/Gensrc-jdk.jlink.gmk. Given that it has already caused
> > > > problems with multiple includes of Modules.gmk (JDK-8213736) I was
> > > > reluctant to include it here too. Without the new include the above
> > > > won't work.
> > > >
> > > > The approach I've taken here seems to be the lesser evil. Thoughts?
> > > I appreciate your concern, but JDK-8213736 was a problem in Main.gmk,
> > > which is part of where Modules.gmk gets bootstrapped. In any normal
> > > makefile (as in where stuff is just being built), the bootstrapping is
> > > done and including Modules.gmk is completely fine. Any
> > > <phase>-<module>.gmk file certainly qualifies here.
> > OK. Updated:
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/05/
> >
> > Thanks,
> > Severin
> >
> > > /Erik
> > >
> > > > Thanks,
> > > > Severin
> > > >
> > > > > Otherwise build changes look ok.
> > > > >
> > > > > /Erik
> > > > >
> > > > > On 2019-02-07 09:09, Severin Gehwolf wrote:
> > > > > > Hi,
> > > > > >
> > > > > > Could I please get reviews for this enhancement? It adds a
> > > > > > debug
> > > > > > symbols stripping plug-in to jlink for Linux and Unix
> > > > > > platforms. It's
> > > > > > the first platform specific jlink plugin and the approach taken
> > > > > > for
> > > > > > keeping it contained is to use a plugin specific
> > > > > > ResourceBundle.
> > > > > > Discussion for this happened in [1].
> > > > > >
> > > > > > The test uses a native library which should never get debug
> > > > > > symbols
> > > > > > stripped during the test library build. As such, tiny
> > > > > > modifications
> > > > > > were needed to make/common/TestFilesCompilation.gmk. Hence,
> > > > > > build-dev
> > > > > > being on the list for this RFR. The test currently only runs on
> > > > > > Linux
> > > > > > and requires objcopy to be available. Otherwise the test is
> > > > > > being
> > > > > > skipped.
> > > > > >
> > > > > > Example usage of this plugin is described in the bug.
> > > > > >
> > > > > > webrev:
> > > > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/04/webrev/
> > > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8214796
> > > > > >
> > > > > > Testing: test/jdk/tools/jlink test/jdk/jdk/modules tests on
> > > > > > Linux
> > > > > > x86_64 (with good and broken objcopy) and the newly added test.
> > > > > > It's
> > > > > > currently running through jdk/submit too.
> > > > > >
> > > > > > Thoughts?
> > > > > >
> > > > > > Thanks,
> > > > > > Severin
> > > > > >
> > > > > > [1]
> > > > > > http://mail.openjdk.java.net/pipermail/jigsaw-dev/2019-January/014109.html
> > > > > >

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries

Erik Joelsson

On 2019-02-12 01:35, Severin Gehwolf wrote:
> Hi Erik,
>
> On Mon, 2019-02-11 at 11:12 -0800, Erik Joelsson wrote:
>> Hello Severin,
>>
>> I think you also need a $(wildcard ) around it, but I may be wrong. Did
>> you try this version?
> Yes, this version works for me without $(wildcard). Why is it needed?

I had to go back and check again to verify, but now I'm sure. The list
of directories returned by FindModuleSrcDirs is all src dirs for the
module. Not all of them are going to contain the specific directory
jdk/tools/jlink/resources. This means SetupCompileProperties will get
called with a few non existing directories. While this will work fine,
the find implementation on some platforms will complain (Macos in
particular), so to avoid introducing confusing warning messages in the
build, it's better to filter down the list of directories to those that
actually exist.

>> Also, please do not indent so much. We have style guidelines [1], which
>> recommend 4 spaces for line break indentation.
> OK, sorry. Fixed locally.

Thanks!

/Erik

> Thanks,
> Severin
>
>> /Erik
>>
>> [1] http://openjdk.java.net/groups/build/doc/code-conventions.html
>>
>> On 2019-02-11 10:03, Severin Gehwolf wrote:
>>> Hi Erik,
>>>
>>> On Thu, 2019-02-07 at 17:01 -0800, Erik Joelsson wrote:
>>>> On 2019-02-07 11:09, Severin Gehwolf wrote:
>>>>> Hi Erik,
>>>>>
>>>>> On Thu, 2019-02-07 at 09:39 -0800, Erik Joelsson wrote:
>>>>>> Hello Severin,
>>>>>>
>>>>>> There is a macro for automatically finding all source dirs for a module.
>>>>>> So in Gensrc-jdk.jlink.gmk, I think it would be better expressed using
>>>>>> that macro, like this:
>>>>>>
>>>>>> JLINK_RESOURCE_DIRS := $(wildcard $(addsuffix
>>>>>> /jdk/tools/jlink/resources, $(call FindModuleSrcSdirs, jdk.jlink)))
>>>>>>
>>>>>> The above could/should even be inlined.
>>>>> I've considered this. It seems, though, that FindModuleSrcDirs comes
>>>>> from make/common/Modules.gmk which isn't included in
>>>>> make/gensrc/Gensrc-jdk.jlink.gmk. Given that it has already caused
>>>>> problems with multiple includes of Modules.gmk (JDK-8213736) I was
>>>>> reluctant to include it here too. Without the new include the above
>>>>> won't work.
>>>>>
>>>>> The approach I've taken here seems to be the lesser evil. Thoughts?
>>>> I appreciate your concern, but JDK-8213736 was a problem in Main.gmk,
>>>> which is part of where Modules.gmk gets bootstrapped. In any normal
>>>> makefile (as in where stuff is just being built), the bootstrapping is
>>>> done and including Modules.gmk is completely fine. Any
>>>> <phase>-<module>.gmk file certainly qualifies here.
>>> OK. Updated:
>>> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/05/
>>>
>>> Thanks,
>>> Severin
>>>
>>>> /Erik
>>>>
>>>>> Thanks,
>>>>> Severin
>>>>>
>>>>>> Otherwise build changes look ok.
>>>>>>
>>>>>> /Erik
>>>>>>
>>>>>> On 2019-02-07 09:09, Severin Gehwolf wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Could I please get reviews for this enhancement? It adds a
>>>>>>> debug
>>>>>>> symbols stripping plug-in to jlink for Linux and Unix
>>>>>>> platforms. It's
>>>>>>> the first platform specific jlink plugin and the approach taken
>>>>>>> for
>>>>>>> keeping it contained is to use a plugin specific
>>>>>>> ResourceBundle.
>>>>>>> Discussion for this happened in [1].
>>>>>>>
>>>>>>> The test uses a native library which should never get debug
>>>>>>> symbols
>>>>>>> stripped during the test library build. As such, tiny
>>>>>>> modifications
>>>>>>> were needed to make/common/TestFilesCompilation.gmk. Hence,
>>>>>>> build-dev
>>>>>>> being on the list for this RFR. The test currently only runs on
>>>>>>> Linux
>>>>>>> and requires objcopy to be available. Otherwise the test is
>>>>>>> being
>>>>>>> skipped.
>>>>>>>
>>>>>>> Example usage of this plugin is described in the bug.
>>>>>>>
>>>>>>> webrev:
>>>>>>> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/04/webrev/
>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8214796
>>>>>>>
>>>>>>> Testing: test/jdk/tools/jlink test/jdk/jdk/modules tests on
>>>>>>> Linux
>>>>>>> x86_64 (with good and broken objcopy) and the newly added test.
>>>>>>> It's
>>>>>>> currently running through jdk/submit too.
>>>>>>>
>>>>>>> Thoughts?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Severin
>>>>>>>
>>>>>>> [1]
>>>>>>> http://mail.openjdk.java.net/pipermail/jigsaw-dev/2019-January/014109.html
>>>>>>>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries

Severin Gehwolf
Hi Erik,

On Tue, 2019-02-12 at 08:44 -0800, Erik Joelsson wrote:

> On 2019-02-12 01:35, Severin Gehwolf wrote:
> > Hi Erik,
> >
> > On Mon, 2019-02-11 at 11:12 -0800, Erik Joelsson wrote:
> > > Hello Severin,
> > >
> > > I think you also need a $(wildcard ) around it, but I may be wrong. Did
> > > you try this version?
> > Yes, this version works for me without $(wildcard). Why is it needed?
>
> I had to go back and check again to verify, but now I'm sure. The list
> of directories returned by FindModuleSrcDirs is all src dirs for the
> module. Not all of them are going to contain the specific directory
> jdk/tools/jlink/resources. This means SetupCompileProperties will get
> called with a few non existing directories. While this will work fine,
> the find implementation on some platforms will complain (Macos in
> particular), so to avoid introducing confusing warning messages in the
> build, it's better to filter down the list of directories to those that
> actually exist.

OK, thanks for the explanation. I suppose $(wildcard ...) does that,
then? I've added it back locally but I have no way of testing whether
this makes any difference, except jdk/submit perhaps?

diff --git a/make/gensrc/Gensrc-jdk.jlink.gmk b/make/gensrc/Gensrc-jdk.jlink.gmk
--- a/make/gensrc/Gensrc-jdk.jlink.gmk
+++ b/make/gensrc/Gensrc-jdk.jlink.gmk
@@ -29,8 +29,9 @@
 
 ################################################################################
 
-JLINK_RESOURCES_DIRS := $(addsuffix /jdk/tools/jlink/resources, \
-    $(call FindModuleSrcDirs, jdk.jlink))
+# Use wildcard so as to avoid getting non-existing directories back
+JLINK_RESOURCES_DIRS := $(wildcard $(addsuffix /jdk/tools/jlink/resources, \
+    $(call FindModuleSrcDirs, jdk.jlink)))
 
 $(eval $(call SetupCompileProperties, JLINK_PROPERTIES, \
     SRC_DIRS := $(JLINK_RESOURCES_DIRS), \

Thanks,
Severin

> > > Also, please do not indent so much. We have style guidelines [1], which
> > > recommend 4 spaces for line break indentation.
> > OK, sorry. Fixed locally.
>
> Thanks!
>
> /Erik
>
> > Thanks,
> > Severin
> >
> > > /Erik
> > >
> > > [1] http://openjdk.java.net/groups/build/doc/code-conventions.html
> > >
> > > On 2019-02-11 10:03, Severin Gehwolf wrote:
> > > > Hi Erik,
> > > >
> > > > On Thu, 2019-02-07 at 17:01 -0800, Erik Joelsson wrote:
> > > > > On 2019-02-07 11:09, Severin Gehwolf wrote:
> > > > > > Hi Erik,
> > > > > >
> > > > > > On Thu, 2019-02-07 at 09:39 -0800, Erik Joelsson wrote:
> > > > > > > Hello Severin,
> > > > > > >
> > > > > > > There is a macro for automatically finding all source dirs for a module.
> > > > > > > So in Gensrc-jdk.jlink.gmk, I think it would be better expressed using
> > > > > > > that macro, like this:
> > > > > > >
> > > > > > > JLINK_RESOURCE_DIRS := $(wildcard $(addsuffix
> > > > > > > /jdk/tools/jlink/resources, $(call FindModuleSrcSdirs, jdk.jlink)))
> > > > > > >
> > > > > > > The above could/should even be inlined.
> > > > > > I've considered this. It seems, though, that FindModuleSrcDirs comes
> > > > > > from make/common/Modules.gmk which isn't included in
> > > > > > make/gensrc/Gensrc-jdk.jlink.gmk. Given that it has already caused
> > > > > > problems with multiple includes of Modules.gmk (JDK-8213736) I was
> > > > > > reluctant to include it here too. Without the new include the above
> > > > > > won't work.
> > > > > >
> > > > > > The approach I've taken here seems to be the lesser evil. Thoughts?
> > > > > I appreciate your concern, but JDK-8213736 was a problem in Main.gmk,
> > > > > which is part of where Modules.gmk gets bootstrapped. In any normal
> > > > > makefile (as in where stuff is just being built), the bootstrapping is
> > > > > done and including Modules.gmk is completely fine. Any
> > > > > <phase>-<module>.gmk file certainly qualifies here.
> > > > OK. Updated:
> > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/05/
> > > >
> > > > Thanks,
> > > > Severin
> > > >
> > > > > /Erik
> > > > >
> > > > > > Thanks,
> > > > > > Severin
> > > > > >
> > > > > > > Otherwise build changes look ok.
> > > > > > >
> > > > > > > /Erik
> > > > > > >
> > > > > > > On 2019-02-07 09:09, Severin Gehwolf wrote:
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > Could I please get reviews for this enhancement? It adds a
> > > > > > > > debug
> > > > > > > > symbols stripping plug-in to jlink for Linux and Unix
> > > > > > > > platforms. It's
> > > > > > > > the first platform specific jlink plugin and the approach taken
> > > > > > > > for
> > > > > > > > keeping it contained is to use a plugin specific
> > > > > > > > ResourceBundle.
> > > > > > > > Discussion for this happened in [1].
> > > > > > > >
> > > > > > > > The test uses a native library which should never get debug
> > > > > > > > symbols
> > > > > > > > stripped during the test library build. As such, tiny
> > > > > > > > modifications
> > > > > > > > were needed to make/common/TestFilesCompilation.gmk. Hence,
> > > > > > > > build-dev
> > > > > > > > being on the list for this RFR. The test currently only runs on
> > > > > > > > Linux
> > > > > > > > and requires objcopy to be available. Otherwise the test is
> > > > > > > > being
> > > > > > > > skipped.
> > > > > > > >
> > > > > > > > Example usage of this plugin is described in the bug.
> > > > > > > >
> > > > > > > > webrev:
> > > > > > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/04/webrev/
> > > > > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8214796
> > > > > > > >
> > > > > > > > Testing: test/jdk/tools/jlink test/jdk/jdk/modules tests on
> > > > > > > > Linux
> > > > > > > > x86_64 (with good and broken objcopy) and the newly added test.
> > > > > > > > It's
> > > > > > > > currently running through jdk/submit too.
> > > > > > > >
> > > > > > > > Thoughts?
> > > > > > > >
> > > > > > > > Thanks,
> > > > > > > > Severin
> > > > > > > >
> > > > > > > > [1]
> > > > > > > > http://mail.openjdk.java.net/pipermail/jigsaw-dev/2019-January/014109.html
> > > > > > > >

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries

Erik Joelsson
Hello,

On 2019-02-12 09:05, Severin Gehwolf wrote:

> Hi Erik,
>
> On Tue, 2019-02-12 at 08:44 -0800, Erik Joelsson wrote:
>> On 2019-02-12 01:35, Severin Gehwolf wrote:
>>> Hi Erik,
>>>
>>> On Mon, 2019-02-11 at 11:12 -0800, Erik Joelsson wrote:
>>>> Hello Severin,
>>>>
>>>> I think you also need a $(wildcard ) around it, but I may be wrong. Did
>>>> you try this version?
>>> Yes, this version works for me without $(wildcard). Why is it needed?
>> I had to go back and check again to verify, but now I'm sure. The list
>> of directories returned by FindModuleSrcDirs is all src dirs for the
>> module. Not all of them are going to contain the specific directory
>> jdk/tools/jlink/resources. This means SetupCompileProperties will get
>> called with a few non existing directories. While this will work fine,
>> the find implementation on some platforms will complain (Macos in
>> particular), so to avoid introducing confusing warning messages in the
>> build, it's better to filter down the list of directories to those that
>> actually exist.
> OK, thanks for the explanation. I suppose $(wildcard ...) does that,
> then? I've added it back locally but I have no way of testing whether
> this makes any difference, except jdk/submit perhaps?

Yes, that is what wildcard does, it filters out any non existing dirs.

No need for you to verify anything but that it works as far as I am
concerned. I'm happy with the below.

/Erik

> diff --git a/make/gensrc/Gensrc-jdk.jlink.gmk b/make/gensrc/Gensrc-jdk.jlink.gmk
> --- a/make/gensrc/Gensrc-jdk.jlink.gmk
> +++ b/make/gensrc/Gensrc-jdk.jlink.gmk
> @@ -29,8 +29,9 @@
>  
>   ################################################################################
>  
> -JLINK_RESOURCES_DIRS := $(addsuffix /jdk/tools/jlink/resources, \
> -    $(call FindModuleSrcDirs, jdk.jlink))
> +# Use wildcard so as to avoid getting non-existing directories back
> +JLINK_RESOURCES_DIRS := $(wildcard $(addsuffix /jdk/tools/jlink/resources, \
> +    $(call FindModuleSrcDirs, jdk.jlink)))
>  
>   $(eval $(call SetupCompileProperties, JLINK_PROPERTIES, \
>       SRC_DIRS := $(JLINK_RESOURCES_DIRS), \
>
> Thanks,
> Severin
>
>>>> Also, please do not indent so much. We have style guidelines [1], which
>>>> recommend 4 spaces for line break indentation.
>>> OK, sorry. Fixed locally.
>> Thanks!
>>
>> /Erik
>>
>>> Thanks,
>>> Severin
>>>
>>>> /Erik
>>>>
>>>> [1] http://openjdk.java.net/groups/build/doc/code-conventions.html
>>>>
>>>> On 2019-02-11 10:03, Severin Gehwolf wrote:
>>>>> Hi Erik,
>>>>>
>>>>> On Thu, 2019-02-07 at 17:01 -0800, Erik Joelsson wrote:
>>>>>> On 2019-02-07 11:09, Severin Gehwolf wrote:
>>>>>>> Hi Erik,
>>>>>>>
>>>>>>> On Thu, 2019-02-07 at 09:39 -0800, Erik Joelsson wrote:
>>>>>>>> Hello Severin,
>>>>>>>>
>>>>>>>> There is a macro for automatically finding all source dirs for a module.
>>>>>>>> So in Gensrc-jdk.jlink.gmk, I think it would be better expressed using
>>>>>>>> that macro, like this:
>>>>>>>>
>>>>>>>> JLINK_RESOURCE_DIRS := $(wildcard $(addsuffix
>>>>>>>> /jdk/tools/jlink/resources, $(call FindModuleSrcSdirs, jdk.jlink)))
>>>>>>>>
>>>>>>>> The above could/should even be inlined.
>>>>>>> I've considered this. It seems, though, that FindModuleSrcDirs comes
>>>>>>> from make/common/Modules.gmk which isn't included in
>>>>>>> make/gensrc/Gensrc-jdk.jlink.gmk. Given that it has already caused
>>>>>>> problems with multiple includes of Modules.gmk (JDK-8213736) I was
>>>>>>> reluctant to include it here too. Without the new include the above
>>>>>>> won't work.
>>>>>>>
>>>>>>> The approach I've taken here seems to be the lesser evil. Thoughts?
>>>>>> I appreciate your concern, but JDK-8213736 was a problem in Main.gmk,
>>>>>> which is part of where Modules.gmk gets bootstrapped. In any normal
>>>>>> makefile (as in where stuff is just being built), the bootstrapping is
>>>>>> done and including Modules.gmk is completely fine. Any
>>>>>> <phase>-<module>.gmk file certainly qualifies here.
>>>>> OK. Updated:
>>>>> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/05/
>>>>>
>>>>> Thanks,
>>>>> Severin
>>>>>
>>>>>> /Erik
>>>>>>
>>>>>>> Thanks,
>>>>>>> Severin
>>>>>>>
>>>>>>>> Otherwise build changes look ok.
>>>>>>>>
>>>>>>>> /Erik
>>>>>>>>
>>>>>>>> On 2019-02-07 09:09, Severin Gehwolf wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> Could I please get reviews for this enhancement? It adds a
>>>>>>>>> debug
>>>>>>>>> symbols stripping plug-in to jlink for Linux and Unix
>>>>>>>>> platforms. It's
>>>>>>>>> the first platform specific jlink plugin and the approach taken
>>>>>>>>> for
>>>>>>>>> keeping it contained is to use a plugin specific
>>>>>>>>> ResourceBundle.
>>>>>>>>> Discussion for this happened in [1].
>>>>>>>>>
>>>>>>>>> The test uses a native library which should never get debug
>>>>>>>>> symbols
>>>>>>>>> stripped during the test library build. As such, tiny
>>>>>>>>> modifications
>>>>>>>>> were needed to make/common/TestFilesCompilation.gmk. Hence,
>>>>>>>>> build-dev
>>>>>>>>> being on the list for this RFR. The test currently only runs on
>>>>>>>>> Linux
>>>>>>>>> and requires objcopy to be available. Otherwise the test is
>>>>>>>>> being
>>>>>>>>> skipped.
>>>>>>>>>
>>>>>>>>> Example usage of this plugin is described in the bug.
>>>>>>>>>
>>>>>>>>> webrev:
>>>>>>>>> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/04/webrev/
>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8214796
>>>>>>>>>
>>>>>>>>> Testing: test/jdk/tools/jlink test/jdk/jdk/modules tests on
>>>>>>>>> Linux
>>>>>>>>> x86_64 (with good and broken objcopy) and the newly added test.
>>>>>>>>> It's
>>>>>>>>> currently running through jdk/submit too.
>>>>>>>>>
>>>>>>>>> Thoughts?
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Severin
>>>>>>>>>
>>>>>>>>> [1]
>>>>>>>>> http://mail.openjdk.java.net/pipermail/jigsaw-dev/2019-January/014109.html
>>>>>>>>>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries

Severin Gehwolf
In reply to this post by Mandy Chung
Hi Mandy, Alan,

Please find the proposal for CLI option of --strip-native-debug-symbols
below.

On Fri, 2019-02-08 at 10:53 -0800, Mandy Chung wrote:

>
> On 2/8/19 2:08 AM, Alan Bateman wrote:
> > I agree with Mandy that the CLI options need examination. The proposed
> > `--strip-native-debug-symbols` seems reasonable but it will be
> > inconsistent with the existing `--strip-debug` option. Mandy - what you
> > would think about renaming the existing option to something that makes
> > it clear that it strips the debug attribute from class files? (we would
> > of course need to do something to keep the existing option working).
>
> Renaming it to make it clear is good.  How about:
>
> --strip-debug-attribute
> --strip-native-debug-symbols
>
> --strip-debug will invoke both --strip-debug-attribute and
> --strip-native-debug-symbols, if supported.
>
> Typically we want to strip both.  If only stripping debug attribute,
> then it can use --strip-debug-attribute.
>
> What do you think?
>
> > The need to specify the argument as "defaults" or "options" is a bit
> > annoying. It might be time to replace hasArguments in the plugin  > interface to allow for optional arguments.
>
> Hmm... I thought it allows optional arguments.  LegalNoticeFilePlugin
> accepts an optional argument.
>
> On the other hand, pluginToMaps will put an empty map if hasArguments
> return false.  The plugin processing code (PluginsHelper) is quite
> complicated that I can't quite tell without spending time.
>
> In any case I think a plugin should support optional arguments.
>
> > The plugin interface is not
> > exported/documented/supported so we have flexibility to change it. If we
> > do this then it should mean the usages reduce down to:
> >
> > --strip-native-debug-symbols
> > --strip-native-debug-symbols objcopy=<path-to-objcopy>:...
>
> objcopy is fine.
>
> It would also be nice to allow `keep-debuginfo` taking an optional
> file extension.
>
> --strip-native-debug-symbols keep-debuginfo
> --strip-native-debug-symbols keep-debuginfo=dbg

The current implementation here has the following options:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/05/webrev/

[i]   --strip-native-debug-symbols defaults
[ii]  --strip-native-debug-symbols options:objcopy-cmd=<path/to/objcopycmd>
[iii] --strip-native-debug-symbols options:debuginfo-file-ext=<ext>
[iv]  --strip-native-debug-symbols options:include-debug-syms=true

The first option is a work-around for JDK-8218761. AFAIUI, fixing it
will need rework of the Plugin interface and probably of the options
parsing. Hence, I'd like to defer this post integration of the initial
version of --strip-native-debug-symbols plugin.

Cases [iii] and [iv] can be folded into one as suggested by Mandy with:

--strip-native-debug-symbols keep-debuginfo
--strip-native-debug-symbols keep-debuginfo=<ext>

Case [ii] would become:

--strip-native-debug-symbols objcopy=<path/to/objcopy>

So in summary I'd propose these, where a) and b) may be combined, c)
and a) or c) and b) combined would be an error:

[a] --strip-native-debug-symbols keep-debuginfo[=<ext>]
[b] --strip-native-debug-symbols objcopy=<path/to/objcopy>
[c] --strip-native-debug-symbols defaults

As a follow-up to an initial implementation of the above, I'd propose
to hook it up with the current --strip-debug by a follow-up patch. It
would first rename --strip-debug to --strip-debug-attribute or perhaps
--strip-java-debug-symbols, and then let --strip-debug perform java and
native debug symbols stripping as Alan suggested.

Does that sound reasonable to you?

Thanks,
Severin

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries

Severin Gehwolf
In reply to this post by Mandy Chung
Hi Mandy,

Thanks for the review!

On Thu, 2019-02-07 at 12:43 -0800, Mandy Chung wrote:

> Hi Severin,
>
> Using the plugin specific ResourceBundle is good.  Thanks for making
> the change.
>
> I see that Alan's comment [1] on the plugin options and I assume
> you will investigate the plugin options and bring back a proposal.
> Did I miss the discussion on these options?
>
> Option:
> --strip-native-debug-symbols=<defaults|options[:objcopy-cmd=<path/to/objcopycmd>][:debuginfo-file-ext=<ext>][:include-debug-syms=true]>
>
> Examples: --strip-native-debug-symbols=defaults
>
> I suggest the above be simplified and drop the "=defaults".  Simply:
>     --strip-native-debug-symbols
>
> Examples:
> --strip-native-debug-symbols=options:objcopy-cmd=/usr/bin/objcopy:debuginfo-file-ext=dbg:include-debug-syms=true
>
>
> If include-debug-syms=true then it will run
>    objcopy --only-keep-debug foo foo.<ext> to create debug symbols file
>    objcopy --add-gnu-debuglink=foo.dbg foo
>
> So what about simplifying the options to:
>  
> --strip-native-debug-symbols=keep-debug-symbols=dbg,objcopy-path=/usr/bin/objcopy
>
> We could also drop the word "symbols":
>
>  
> --strip-native-debug=[keep-debug=<debug-file-ext>,][objcopy-path=<path-of-objcopy>]
>
> By default, `--strip-native-debug` will strip native debug symbols
> and don't keep debug symbols.
>
> keep-debug=<debug-file-ext> creates the debug symbols file.
> <debug-file-ext> specifies the file extension.  It would be
> nice to use the default `debuginfo` extension and simply
> accept `keep-debug` option.
>
> `objcopy-cmd` may be okay too.  Others may have opinion.
>
> I think we should agree on the plugin options first before
> doing the code review.

OK. Here is the summary:
http://mail.openjdk.java.net/pipermail/jigsaw-dev/2019-February/014132.html

Personally, I find --strip-native-debug-info or --strip-native-debug-
symbols clearer than just --strip-native-debug.

>
> W.r.t. the test:
>
>  > The test currently only runs on Linux
>  > and requires objcopy to be available. Otherwise the test is being
>  > skipped.
>
> We can create a fake objcopy utility for testing purpose
> such that the test will validate if the options are passed
> properly to the test utility.

AFAIK, objcopy is a build requirement for OpenJDK already, so I'm
wondering whether it makes sense to stub objcopy for the tests. Perhaps
you meant that to be used in addition to existing tests?

Anyhow, I'll look into it. It'll likely have to use the '--strip-
native-debug-symbols objcopy=<path/to/fake-objcopy' option in that
case. It'll have to be some native code which will require some custom
make machinery to get compiled, no? If you have pointers to examples in
the JDK already, I'd appreciate it.

>  > webrev:
> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/04/webrev/
>
> I haven't reviewed the new files.  Just notice that very long
> lines in the new files that you may want to fix the formatting
> that will help further side-by-side review.

Some of the longer lines have been cleaned up in the lates webrev:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/05/webrev/

> The --list-plugin output is very very long.  The existing plugins
> didn't set a good example to keep the well formatted (I recorded
> that they were cleaned up at one point to keep 80 chars width).

Right. I'll make sure --list-plugins looks right for --strip-native-
debug-symbols once we've agreed on the options.

> One other question: should this plugin be moved to linux/classes
> rather than unix/classes given that that's the platform it
> intends to support?

Done in version 05.

Thanks,
Severin

> Mandy
> [1]
> http://mail.openjdk.java.net/pipermail/jigsaw-dev/2019-January/014099.html

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries

Mandy Chung


On 2/12/19 12:05 PM, Severin Gehwolf wrote:
> Hi Mandy,
>
> Thanks for the review!
>
> OK. Here is the summary:
> http://mail.openjdk.java.net/pipermail/jigsaw-dev/2019-February/014132.html
>
> Personally, I find --strip-native-debug-info or --strip-native-debug-
> symbols clearer than just --strip-native-debug.

Fine with me and we can stick with --strip-native-debug-symbols

>> W.r.t. the test:
>>
>>   > The test currently only runs on Linux
>>   > and requires objcopy to be available. Otherwise the test is being
>>   > skipped.
>>
>> We can create a fake objcopy utility for testing purpose
>> such that the test will validate if the options are passed
>> properly to the test utility.
>
> AFAIK, objcopy is a build requirement for OpenJDK already, so I'm
> wondering whether it makes sense to stub objcopy for the tests. Perhaps
> you meant that to be used in addition to existing tests?

Test machines may not have objcopy.  It'd increase the test
coverage to include a test that can run on all test machines.
Yes it's an additional test in addition to a test you have
that only runs if objcopy utility is present.

> Anyhow, I'll look into it. It'll likely have to use the '--strip-
> native-debug-symbols objcopy=<path/to/fake-objcopy' option in that
> case.

What I was thinking is:
--strip-native-debug-symbols objcopy="java fakeobjcopy"

where objcopy accepts a command that can contain arguments.

It'll have to be some native code which will require some custom
> make machinery to get compiled, no? If you have pointers to examples in
> the JDK already, I'd appreciate it.

See the idea above.

>>   > webrev:
>> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/04/webrev/
>>
>> I haven't reviewed the new files.  Just notice that very long
>> lines in the new files that you may want to fix the formatting
>> that will help further side-by-side review.
>
> Some of the longer lines have been cleaned up in the lates webrev:
> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/05/webrev/
>
>> The --list-plugin output is very very long.  The existing plugins
>> didn't set a good example to keep the well formatted (I recorded
>> that they were cleaned up at one point to keep 80 chars width).
>
> Right. I'll make sure --list-plugins looks right for --strip-native-
> debug-symbols once we've agreed on the options.
>
>> One other question: should this plugin be moved to linux/classes
>> rather than unix/classes given that that's the platform it
>> intends to support?
>
> Done in version 05.

Thanks for making the change.  I will reply the other thread.

Mandy
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries

Mandy Chung
In reply to this post by Severin Gehwolf


On 2/12/19 11:52 AM, Severin Gehwolf wrote:

> Hi Mandy, Alan,
>
> Please find the proposal for CLI option of --strip-native-debug-symbols
> below.
>
> The current implementation here has the following options:
> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/05/webrev/
>
> [i]   --strip-native-debug-symbols defaults
> [ii]  --strip-native-debug-symbols options:objcopy-cmd=<path/to/objcopycmd>
> [iii] --strip-native-debug-symbols options:debuginfo-file-ext=<ext>
> [iv]  --strip-native-debug-symbols options:include-debug-syms=true
>
> The first option is a work-around for JDK-8218761. AFAIUI, fixing it
> will need rework of the Plugin interface and probably of the options
> parsing. Hence, I'd like to defer this post integration of the initial
> version of --strip-native-debug-symbols plugin.
>
> Cases [iii] and [iv] can be folded into one as suggested by Mandy with:
>
> --strip-native-debug-symbols keep-debuginfo
> --strip-native-debug-symbols keep-debuginfo=<ext>
>
> Case [ii] would become:
>
> --strip-native-debug-symbols objcopy=<path/to/objcopy>

we could relax this to a command that can contain arguments.

> So in summary I'd propose these, where a) and b) may be combined, c)
> and a) or c) and b) combined would be an error:
>
> [a] --strip-native-debug-symbols keep-debuginfo[=<ext>]
> [b] --strip-native-debug-symbols objcopy=<path/to/objcopy>
> [c] --strip-native-debug-symbols defaults

This is a good compromise.  When JDK-8218761 is implemented,
[c] can become `--strip-native-debug-symbols`

"defaults" is unclear to what it does.  What about
    --strip-native-debug-symbols no-keep-debuginfo

> As a follow-up to an initial implementation of the above, I'd propose
> to hook it up with the current --strip-debug by a follow-up patch. It
> would first rename --strip-debug to --strip-debug-attribute or perhaps
> --strip-java-debug-symbols, and then let --strip-debug perform java and
> native debug symbols stripping as Alan suggested.

The renaming can be done separately.  I would prefer changing
--strip-debug to invoke --strip-native-debug-symbols, if present,
at the same time with this new strip native debug symbols plugin
to ensure that they all go in the same release.

In other words, the renaming should be done before this new plugin.
That's my opinion.

Mandy

> Does that sound reasonable to you?
>
> Thanks,
> Severin
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries

Severin Gehwolf
On Tue, 2019-02-12 at 15:29 -0800, Mandy Chung wrote:

>
> On 2/12/19 11:52 AM, Severin Gehwolf wrote:
> > Hi Mandy, Alan,
> >
> > Please find the proposal for CLI option of --strip-native-debug-symbols
> > below.
> >
> > The current implementation here has the following options:
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/05/webrev/
> >
> > [i]   --strip-native-debug-symbols defaults
> > [ii]  --strip-native-debug-symbols options:objcopy-cmd=<path/to/objcopycmd>
> > [iii] --strip-native-debug-symbols options:debuginfo-file-ext=<ext>
> > [iv]  --strip-native-debug-symbols options:include-debug-syms=true
> >
> > The first option is a work-around for JDK-8218761. AFAIUI, fixing it
> > will need rework of the Plugin interface and probably of the options
> > parsing. Hence, I'd like to defer this post integration of the initial
> > version of --strip-native-debug-symbols plugin.
> >
> > Cases [iii] and [iv] can be folded into one as suggested by Mandy with:
> >
> > --strip-native-debug-symbols keep-debuginfo
> > --strip-native-debug-symbols keep-debuginfo=<ext>
> >
> > Case [ii] would become:
> >
> > --strip-native-debug-symbols objcopy=<path/to/objcopy>
>
> we could relax this to a command that can contain arguments.

OK. I'll explore that.

> > So in summary I'd propose these, where a) and b) may be combined, c)
> > and a) or c) and b) combined would be an error:
> >
> > [a] --strip-native-debug-symbols keep-debuginfo[=<ext>]
> > [b] --strip-native-debug-symbols objcopy=<path/to/objcopy>
> > [c] --strip-native-debug-symbols defaults
>
> This is a good compromise.  When JDK-8218761 is implemented,
> [c] can become `--strip-native-debug-symbols`
>
> "defaults" is unclear to what it does.  What about
>     --strip-native-debug-symbols no-keep-debuginfo

Makes sense. How about this?

--strip-native-debug-symbols strip-debuginfo

or

--strip-native-debug-symbols remove-debuginfo

It would avoid using negation.

> > As a follow-up to an initial implementation of the above, I'd propose
> > to hook it up with the current --strip-debug by a follow-up patch. It
> > would first rename --strip-debug to --strip-debug-attribute or perhaps
> > --strip-java-debug-symbols, and then let --strip-debug perform java and
> > native debug symbols stripping as Alan suggested.
>
> The renaming can be done separately.  I would prefer changing
> --strip-debug to invoke --strip-native-debug-symbols, if present,
> at the same time with this new strip native debug symbols plugin
> to ensure that they all go in the same release.
>
> In other words, the renaming should be done before this new plugin.
> That's my opinion.

Sure. I've filed JDK-8218913 for doing this first. Would a name of --
strip-java-debug-symbols be acceptable? My thinking is that it would
have nice symmetry with --strip-native-debug-symbols. Thoughts?

Thanks,
Severin

> Mandy
>
> > Does that sound reasonable to you?
> >
> > Thanks,
> > Severin
> >

123