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
