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

[Mandy Chung]

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