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

[Severin Gehwolf]

Hi Mandy,

Thanks for the review! I'll update the webrev ASAP and will get back to
you once ready.

Thanks,
Severin

On Tue, 2019-02-26 at 13:35 -0800, Mandy Chung wrote:
> Hi Severin,
> 
> This is my initial set of comments. 
> 
> 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/
> >    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
> >            --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. 
> > 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(...).
>  
>   47     private static final String OMIT_DEBUGINFO = "exclude-
> debuginfo-files";
> 
> Nit: rename OMIT_ to EXCLUDE_ to match the name
> 
>   77             Map<String, String> stripNativeConfig = Map.<String,
> String>of(
> 
> drop the diamond operation.
> 
> StripNativeDebugSymbolsPlugin.java
> 
>   56     public static final String NAME = "strip-native-debug-
> symbols";
> 
> Does this need to be public?  Should it be private static field?
> 
>  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) 
> 
> 
> 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.
> 
> doConfigure compares if arg matches one of the expected argument
> names.  I think it does not report if any invalid argument?   
> 
> 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?
> 
> createDebugSymbolsFile, addGnuDebugLink and stripBinary
> 
> - if the command fails, should the plugin fail instead? you currently
> silently ignore any error.
> 
> ObjCopyCmdBuilder.build(String objCopy) can be changed to 
> ObjCopyCmdBuilder.build(String objCopy, String... opts) to build the
> entire command line in one go.
> 
> 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 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.
> 
> Mandy