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
