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

Tue Apr 9 09:06:53 UTC 2019

Hi Mandy,

Sorry for the long delay. Other priorities came up, but I am ready to
pick this up again. CSR updated and comments in-line.

On Tue, 2019-02-26 at 13:35 -0800, Mandy Chung wrote:
> Hi Severin,
> 
> This is my initial set of comments. 

Thanks for the review! Latest webrev with the updates:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/09/webrev/

> On 2/26/19 3:01 AM, Severin Gehwolf wrote:
> > --list-plugins output for --strip-native-debug-symbols:
> > 
> > Plugin Name: strip-native-debug-symbols
> > Option: --strip-native-debug-symbols=<exclude-debuginfo-files|keep-
> > debuginfo-files>
> > Description: Strip debug symbols from native libraries (if any).
> >  This plugin requires at least one option:
> >    objcopy: The path to the objcopy binary. Defaults to objcopy in
> > PATH.
> >    exclude-debuginfo-files: Omit debug info files. Defaults to
> > true.
>  
> s/Omit/Exclude/

Fixed in the latest webrev.

> >    keep-debuginfo-files[=<ext>]: Keep debug info files in
> > <file>.<ext>.
> >                                  Defaults to <file>.debuginfo
> >  Examples: --strip-native-debug-symbols objcopy=/usr/bin/objcopy
>  
> This is a little awkward that this defaults to exclude-debuginfo-
> files.  As described above, I would expect "exclude-debuginfo-files"
> or "keep-debuginfo-files" must be specified.  It means that the
> command would be:
>     --strip-native-debug-symbols exclude-debuginfo-
> files:objcopy=/usr/bin/objcopy

Sure. I've updated the examples.

> >            --strip-native-debug-symbols=exclude-debuginfo-files
> >            --strip-native-debug-symbols keep-debuginfo-
> > files:objcopy=objcopy
> > 
>  
> What is the expected behavior when jlink --strip-debug --strip-
> native-debug-symbols keep-debuginfo-files is specified?   Should it
> be accepted?  This should be specified in the CSR. 

I believe the best way forward would be for them to conflict. Knowing
that --strip-debug invokes "--strip-native-debug-symbols exclude-
debuginfo-files", it would be weird to accept it. The latest patch
behaves like this:

./bin/jlink --add-modules java.base --strip-native-debug-symbols keep-debuginfo-files --strip-debug --verbose --output custom-image-foo
java.base file:///disk/openjdk/upstream-sources/openjdk-head/build/linux-x86_64-server-fastdebug/images/jdk/jmods/java.base.jmod

Providers:
  java.base provides java.nio.file.spi.FileSystemProvider used by java.base
Error: --strip-debug (-G) conflicts with --strip-native-debug-symbols. Please use one or the other, but not both.

I've updated the CSR.

> > Latest webrev which implements this:
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/08/webrev/
> > 
>  
> DefaultStripDebugPlugin.java
> 
> I still think that conceptually it's more straight-forward to treat
> --strip-debug option to imply as if --strip-java-debug-attributes and
> --strip-native-debug-symbols option, if available and not specified,
> were specified on the command line.  These plugins will be invoked
> accordingly rather than an explicit invocation of these 2 plugins.  
> But we don't have such precedence to follow.   W.r.t. your current
> implementation:
> 
> I think this can be simplfied to keep Optional<Plugin> field and
> replace
> DefaultNativePluginFactory and NativePluginFactory with a simple
> static
> method to return Optional.ofNullable(...).

Right. We could do that. I've chosen this approach so as to facilitate
better testing of this code. See DefaultStripDebugPluginTest.java.
Unless I'm missing something, that won't work as well when refactored
the way you suggest.

>   47     private static final String OMIT_DEBUGINFO = "exclude-
> debuginfo-files";
> 
> Nit: rename OMIT_ to EXCLUDE_ to match the name

Fixed.

>   77             Map<String, String> stripNativeConfig = Map.<String,
> String>of(
> 
> drop the diamond operation.

Fixed.

> StripNativeDebugSymbolsPlugin.java
> 
>   56     public static final String NAME = "strip-native-debug-
> symbols";
> 
> Does this need to be public?  Should it be private static field?

It doesn't need to be, but it helps testing. Other plugins seems to
have the name public too. If possible, I'd like to keep it. It's in an
internal package anyway.

>  101             if (resource.type() ==
> ResourcePoolEntry.Type.NATIVE_LIB ||
>  102                 resource.type() ==
> ResourcePoolEntry.Type.NATIVE_CMD) {
>  103                 String moduleName = resource.moduleName();
>  104                 if (resource.path().endsWith(SHARED_LIBS_EXT) ||
>  105                     resource.path().startsWith("/" + moduleName
> + "/bin")) {
> 
> 
> Alternatively:
> 
>  if ((resource.type() == ResourcePoolEntry.Type.NATIVE_LIB 
>         && resource.path().endsWith(SHARED_LIBS_EXT)) ||
>     resource.type() == ResourcePoolEntry.Type.NATIVE_CMD) 

Thanks. Updated as suggested.

> If --strip-native-commands is specified, this may want to skip
> processing NATIVE_CMD?  Alternatively it should ensure -strip-native-
> commands is invoked before --strip-native-debug-symbols; otherwise,
> debuginfo files might have left behind if keep-debuginfo-files is
> specified while the native commands are stripped later.

I don't think any special casing for this is needed. debuginfo entries
will have the same type as the resource they've been created from.
Thus, strip-native-commands will remove binaries as well as associated
debuginfo files. They both would have type
ResourcePoolEntry.Type.NATIVE_CMD.

> doConfigure compares if arg matches one of the expected argument
> names.  I think it does not report if any invalid argument?   

It now does and I've added tests for it.

> Looks like it also accepts --strip-native-debug-symbols=exclude-
> debuginfo-files=abc:keep-debuginfo-files=cde.  This might be a jlink
> plugin parsing bug?

Yes, the whole parsing of arguments is a bit of a mess. For now, I've
treated 'exclude-debuginfo-files=abc' as if 'exclude-debuginfo-files'
has been specified. Then, if both exclude-debuginfo-files and keep-
debuginfo-files are specified it'll refuse to accept those conflicting
options.

> createDebugSymbolsFile, addGnuDebugLink and stripBinary
> 
> - if the command fails, should the plugin fail instead? you currently
> silently ignore any error.

I've added some diagnostics when stripping fails. If stripping wasn't
successfull to begin with for a resource an error message is being
printed. If -Djlink.debug=true is being passed, more output will be
there to help diagnose what actually went wrong.

Since the plugin potentially strips many resources, I'd find it awkward
if the plugin fails in its entirety. After all, it might succeed on
some native libs and fail on others. That's why I've chosen this
approach.

> ObjCopyCmdBuilder.build(String objCopy) can be changed to 
> ObjCopyCmdBuilder.build(String objCopy, String... opts) to build the
> entire command line in one go.

OK, done.

> Nit: StrippedDebugInfoBinary can be made immutable.  It might be
> cleaner to refactor the createDebugSymbolsFile, addGnuDebugLink and
> stripBinary methods in a builder class to build a
> StrippedDebugInfoBinary for a given ResourcePoolEntry.

I've refactored this a bit. Hopefully that seems cleaner now.

> I suggest to place the tests under
> test/jdk/tools/jlink/plugins/StripNativeDebugSymbolsPlugin/ which I
> think jlink tests are organized with plugin class name rather than
> plugin option name.

Done.

Thanks,
Severin