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

Thu Apr 18 08:53:45 UTC 2019

Alan, Mandy, any more comments?

Appreciate your help getting this reviewed.

Thanks,
Severin

On Tue, 2019-04-09 at 11:06 +0200, Severin Gehwolf wrote:
> 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