Re: RFR 8210670 (S): Accept double-dash VM-name options at launch time

[mandy chung]

On 9/12/18 2:16 PM, mark.reinh...@oracle.com wrote:

To make this trivial enhancement I first had to fix some broken tests,
so there are two issues and two webrevs here.

8210669: Some launcher tests assume a pre-JDK 9 run-time image layout

   Two launcher tests, ExecutionEnvironment.java and Test7029048.java (in
   test/jdk/tools/launcher), still assume the old image layout in which a
   HotSpot shared library (libjava.so) is found in $JDK/lib/$ARCH/$VMTYPE
   on Linux and Solaris.  The intermediate $ARCH directory was removed in
   JDK 9, as part of JEP 220 (Modular Run-Time Images) [1].  Since that
   time these tests have been succeeding anyway, because they don’t fail
   when the requested VM is not found.

   The tests should be fixed to fail when the requested VM is not found,
   and to look for VM shared libraries in the proper location.

   JBS: https://bugs.openjdk.java.net/browse/JDK-8210669
   Webrev: http://cr.openjdk.java.net/~mr/rev/8210669/


This looks okay.

8210670: Accept double-dash VM-name options at launch time

   Per JEP 293 (Guidelines for JDK Command-Line Tool Options) [2], enhance
   the Java launcher so that `--server` is accepted to select the server VM,
   in addition to `-server`, and likewise for any other VMs listed in the
   $JDK/lib/jvm.cfg file.

   JBS: https://bugs.openjdk.java.net/browse/JDK-8210670
   Webrev: http://cr.openjdk.java.net/~mr/rev/8210670/


jvm.cfg allows to specify an alias.  Have you considered leveraging 
jvm.cfg alias mechanism?


--server KNOWN ALIASED_TO -server
--client KNOWN ALIASED_TO -client

That'd invoke change in make/Copy-java.base.gmk as well as jvm.cfg to 
require the option

to start with `--` whereas the alias starts with `-`.

Mandy

Re: RFR(S) 8190737: use unicode version of the canonicalize() function to handle long path on windows

[Xueming Shen]

Hi Calvin,

I believe I was thinking of keeping the "getPrefixed" as an 
implementation details and
instead exporting a "A" version of winFileHandleOpen() back then. But I 
don't have a

strong opinion on this one, so your approach looks fine.

No, I don't have a better idea for now to avoid those mb->ws->mb as long 
as the canonicalize()
and zip_open() are two separate invocations. But since they are only for 
> max_path path. I'm

fine with it.

Yes, I think it's time to consider to migrate from interpreting the 
char* as "mb chars" to probably
"utf8 chars" (take a step back, I don't think it's easy to do to 
actually a wchar interface) for windows

platform, but that would be a separate and big rfes.

nit:

130 fname,  /* Ascii char path name */

the comment probably should be "path name in mb char", or ANSI charset.

Thanks,

-Sherman

On 9/12/18, 4:16 PM, Calvin Cheung wrote:

Hi Sherman,

Thanks for your review.
Please refer to my reply below...

On 9/10/18, 5:05 PM, Xueming Shen wrote:

On 9/10/18, 11:42 AM, Calvin Cheung wrote:

bug: https://bugs.openjdk.java.net/browse/JDK-8190737

webrev: http://cr.openjdk.java.net/~ccheung/8190737/webrev.00/

Please review this change for handling long path specified in the 
-Xbootclasspath/a on windows.


Highlight of changes:
- canonicalize_md.c
it makes use of the unicode version of canonicalize 
(wcanonicalize) if the original path is >= MAX_PATH.
So we are converting mb->wc->mb in wcanonicalize (when > max_path), 
and then feed the
resulting path into ZFILE_Open, in which we again do mb->wc-> + 
prefix -> CreateFileW(...)?
That's the minimal change I could come up with. Let me know if you 
have other suggestions.


Maybe it's time to consider a "wchar" interface between vm and 
libraries.

Good idea. I think it should be done in a separate RFE.

Maybe it's fine here given we
are only do this for > max_path case?

This was done so that this change has no impact on the <= MAX_PATH case.


Alan, now reading the win version canonicalize(...), remind me what's 
the real purpose of doing
FindFirstFile/FindClose() here? the API appears to suggest it is used 
to find the first match if there
is/are wildcards but we actually have checked/rejected the wildcards 
at the very beginning ? To

get the "real name" for case?



- zip_util.c
it uses the unicode version of CreateFile (CreateFileW) if the 
original path is >= MAX_PATH.


- io_util_md.c
Since zip_util.c (libzip) calls the getPrefixed() function in 
canonicalize_md.c (libjava), the getPrefixed() function needs to be 
exported.


I kinda remembered that we once wanted to avoid export an variant of 
winFileHandleOpen() from
libjava to libzip ... in this case the alternative is to simply 
copy/paste the getPrefix into libzip ...

but I don't remember the exact concern back then.

I also thought of copy/paste the getPrefix function into libzip.
After looking at the lib/CoreLibraries.gmk, I think libzip already has 
a dependency on libjava:

$(eval $(call SetupJdkLibrary, BUILD_LIBZIP, \
NAME := zip, \
OPTIMIZATION := LOW, \
EXCLUDES := $(LIBZIP_EXCLUDES), \
CFLAGS := $(CFLAGS_JDKLIB) \
$(LIBZ_CFLAGS), \
CFLAGS_unix := $(BUILD_LIBZIP_MMAP) -UDEBUG, \
LDFLAGS := $(LDFLAGS_JDKLIB) \
$(call SET_SHARED_LIBRARY_ORIGIN), \
LIBS_unix := -ljvm -ljava $(LIBZ_LIBS), \
LIBS_windows := jvm.lib $(WIN_JAVA_LIB), \
))

I've done tier1,2,3 testing and didn't see any new failures.
I can do higher tier testing if needed.

thanks,
Calvin

p.s. I didn't see this email until Ioi forwarded it to me since I 
wasn't in the core-libs-dev alias. (I've subscribed to the alias this 
morning.)




-Sherman






- java_md.h
The JVM_MAXPATHLEN has been increased from _MAX_PATH to 1024. It 
is because the the current usage of the canonicalize() function from 
vm code is to preallocate the output buffer as follows:
char* canonical_path = 
NEW_RESOURCE_ARRAY_IN_THREAD(thread, char, JVM_MAXPATHLEN);
if (!get_canonical_path(path, canonical_path, 
JVM_MAXPATHLEN)) {
Also the unix version of canonicalize() function has the 
following check:

int
   canonicalize(char *original, char *resolved, int len)
   {
   if (len < PATH_MAX) {
   errno = EINVAL;
   return -1;
   }
   So dynamically allocating the output buffer requires more work 
beyond the scope of this change.


- LongBCP.java
added a scenario to the test - a long path to a jar file in 
-Xbootclasspath/a.


Testing: tier-[1,2,3].

thanks,
Calvin

Re: RFR: 8210416: [linux] Poor StrictMath performance due to non-optimized compilation

[joe darcy]

Hello,

On 9/12/2018 1:16 AM, Severin Gehwolf wrote:

On Wed, 2018-09-12 at 17:58 +1000, David Holmes wrote:

But I don't understand why the optimization setting is being tied to the
availability of the -ffp-contract flag?

In configure we perform a check for gcc or clang whether that flag is
supported. If it is, it would be non-empty exactly having -ffp-contract
as value. It could be another set of flags for other arches if somebody
wanted to do the same, fwiw. In JDK 8, for example, it's "-mno-fused-
madd -fno-strict-aliasing" for ppc64:
http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/file/2660b127b407/make/lib/CoreLibraries.gmk#l63

We need support for that flag (or a set of flags) when we optimize
fdlibm since otherwise we would lose precision. If the flag is empty
we'd not optimize as we can't guarantee precision. That's why we tie
optimization to the availability of that flag. The expectation is for
this flag to be available on gcc/clang arches only at this point. Does
that make sense?




To condense a potentially long discussion, while the IEEE 754 standard 
has long specified particular results for arithmetic operations (+, -, 
*, /, etc.) on particular floating-point values, languages and their 
compilers often do not provide a reliable mapping of language constructs 
to IEEE 754 operations.


The Java language and JVM are distinctive in this sense because a 
reliable mapping of language-level operation to particular IEEE 754 
operation is mandated by the JLS. (I will leave aside a complicated but 
largely irrelevant discussion of non-strictfp floating-point.)


The C language standards I've looked at do not provide as reliably a 
mapping of floating-point operations as the JLS does. In particular, the 
C standards generally allow a fused multiply add to be used replace a 
pair of add and multiply instructions in an expression like (a * b + c). 
The -ffp-contract=off gcc compiler setting disables this and related 
transformations. (The Sun Studio compilers provide detailed 
configuration options for the sets of floating-point transformations 
that are allowed.)


The specification for StrictMath requires the fdlibm algorithms and the 
fdlibm algorithms rely on the semantics of the floating-point operations 
as written in the source and also rely on some way of doing a bit-wise 
conversion between an integral type and double. The latter is 
accomplished by interpreting the 64-bits of a double as comprising a 
two-element array of 32-bit ints. These idioms often don't work under 
the default C compiler options, leading to the long-standing need to 
have a separate set of compiler options for FDLIBM. A safe, if slow, set 
of options is to fully disable optimization for FDLIBM. That is not 
necessary if sufficient control over the floating-point and aliasing 
semantics is possible via the C compiler options.


In the fullness of time, when (if?) I finish porting the FDLIBM code to 
Java, these sorts of concerns will no longer apply due to the more 
reliably mapping of source expressions in Java to IEEE 754 
floating-point operations.


HTH,

-Joe

Re: RFR: 8210416: [linux] Poor StrictMath performance due to non-optimized compilation

[David Holmes]

Correction ...

On 13/09/2018 8:31 AM, David Holmes wrote:

On 12/09/2018 6:16 PM, Severin Gehwolf wrote:

On Wed, 2018-09-12 at 17:58 +1000, David Holmes wrote:

But I don't understand why the optimization setting is being tied to the
availability of the -ffp-contract flag?


In configure we perform a check for gcc or clang whether that flag is
supported. If it is, it would be non-empty exactly having -ffp-contract
as value. It could be another set of flags for other arches if somebody
wanted to do the same, fwiw. In JDK 8, for example, it's "-mno-fused-
madd -fno-strict-aliasing" for ppc64:
http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/file/2660b127b407/make/lib/CoreLibraries.gmk#l63 



We need support for that flag (or a set of flags) when we optimize
fdlibm since otherwise we would lose precision. If the flag is empty
we'd not optimize as we can't guarantee precision. That's why we tie
optimization to the availability of that flag. The expectation is for
this flag to be available on gcc/clang arches only at this point. Does
that make sense?


Yes that makes sense - thanks. I didn't quite glean that from the comment:

   42 # If FDLIBM_CFLAGS is non-empty we know that we can optimize
   43 # fdlibm by adding those extra C flags. Currently GCC,


I think this should say "when adding" not "by adding".

Thanks,
David


   44 # and clang only.
   45 ifneq ($(FDLIBM_CFLAGS), )
   46   BUILD_LIBFDLIBM_OPTIMIZATION := LOW

But now I can read it and understand.

Thanks,
David


Thanks,
Severin

Re: RFR(JDK12/JAXP/java.xml) 8207760: SAXException: Invalid UTF-16 surrogate detected: d83c ?

[Lance Andersen]

Hi Joe,

The change  seems reasonable

> On Sep 12, 2018, at 2:11 PM, Joe Wang  wrote:
> 
> Hi,
> 
> Please review a patch for a situation where a surrogate pair is at the edge 
> of a buffer. What the existing impl did was to report it as an error. This 
> patch fixes it by caching the high surrogate and prints it out along with the 
> low surrogate. Similar issue exists also in the CDATA section and is fixed in 
> this patch. The CDATA impl had a couple of bugs where an indent could be 
> written inside the CDATA and an unicode character written in between two 
> CDATA sections. Both are fixed in this patch.
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8207760
> webrevs: http://cr.openjdk.java.net/~joehw/jdk12/8207760/webrev/
> 
> Thanks,
> Joe
> 
> 

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

Re: Re: RFR(S) 8190737: use unicode version of the canonicalize() function to handle long path on windows

[Calvin Cheung]

Hi Sherman,

Thanks for your review.
Please refer to my reply below...

On 9/10/18, 5:05 PM, Xueming Shen wrote:

On 9/10/18, 11:42 AM, Calvin Cheung wrote:

bug: https://bugs.openjdk.java.net/browse/JDK-8190737

webrev: http://cr.openjdk.java.net/~ccheung/8190737/webrev.00/

Please review this change for handling long path specified in the 
-Xbootclasspath/a on windows.


Highlight of changes:
- canonicalize_md.c
it makes use of the unicode version of canonicalize 
(wcanonicalize) if the original path is >= MAX_PATH.
So we are converting mb->wc->mb in wcanonicalize (when > max_path), 
and then feed the
resulting path into ZFILE_Open, in which we again do mb->wc-> + prefix 
-> CreateFileW(...)?
That's the minimal change I could come up with. Let me know if you have 
other suggestions.


Maybe it's time to consider a "wchar" interface between vm and libraries.

Good idea. I think it should be done in a separate RFE.

Maybe it's fine here given we
are only do this for > max_path case?

This was done so that this change has no impact on the <= MAX_PATH case.


Alan, now reading the win version canonicalize(...), remind me what's 
the real purpose of doing
FindFirstFile/FindClose() here? the API appears to suggest it is used 
to find the first match if there
is/are wildcards but we actually have checked/rejected the wildcards 
at the very beginning ? To

get the "real name" for case?



- zip_util.c
it uses the unicode version of CreateFile (CreateFileW) if the 
original path is >= MAX_PATH.


- io_util_md.c
Since zip_util.c (libzip) calls the getPrefixed() function in 
canonicalize_md.c (libjava), the getPrefixed() function needs to be 
exported.


I kinda remembered that we once wanted to avoid export an variant of 
winFileHandleOpen() from
libjava to libzip ... in this case the alternative is to simply 
copy/paste the getPrefix into libzip ...

but I don't remember the exact concern back then.

I also thought of copy/paste the getPrefix function into libzip.
After looking at the lib/CoreLibraries.gmk, I think libzip already has a 
dependency on libjava:

$(eval $(call SetupJdkLibrary, BUILD_LIBZIP, \
NAME := zip, \
OPTIMIZATION := LOW, \
EXCLUDES := $(LIBZIP_EXCLUDES), \
CFLAGS := $(CFLAGS_JDKLIB) \
$(LIBZ_CFLAGS), \
CFLAGS_unix := $(BUILD_LIBZIP_MMAP) -UDEBUG, \
LDFLAGS := $(LDFLAGS_JDKLIB) \
$(call SET_SHARED_LIBRARY_ORIGIN), \
LIBS_unix := -ljvm -ljava $(LIBZ_LIBS), \
LIBS_windows := jvm.lib $(WIN_JAVA_LIB), \
))

I've done tier1,2,3 testing and didn't see any new failures.
I can do higher tier testing if needed.

thanks,
Calvin

p.s. I didn't see this email until Ioi forwarded it to me since I wasn't 
in the core-libs-dev alias. (I've subscribed to the alias this morning.)




-Sherman






- java_md.h
The JVM_MAXPATHLEN has been increased from _MAX_PATH to 1024. It 
is because the the current usage of the canonicalize() function from 
vm code is to preallocate the output buffer as follows:
char* canonical_path = 
NEW_RESOURCE_ARRAY_IN_THREAD(thread, char, JVM_MAXPATHLEN);
if (!get_canonical_path(path, canonical_path, 
JVM_MAXPATHLEN)) {
Also the unix version of canonicalize() function has the 
following check:

int
   canonicalize(char *original, char *resolved, int len)
   {
   if (len < PATH_MAX) {
   errno = EINVAL;
   return -1;
   }
   So dynamically allocating the output buffer requires more work 
beyond the scope of this change.


- LongBCP.java
added a scenario to the test - a long path to a jar file in 
-Xbootclasspath/a.


Testing: tier-[1,2,3].

thanks,
Calvin

Re: RFR: 8210416: [linux] Poor StrictMath performance due to non-optimized compilation

[David Holmes]

On 12/09/2018 6:16 PM, Severin Gehwolf wrote:

On Wed, 2018-09-12 at 17:58 +1000, David Holmes wrote:

But I don't understand why the optimization setting is being tied to the
availability of the -ffp-contract flag?


In configure we perform a check for gcc or clang whether that flag is
supported. If it is, it would be non-empty exactly having -ffp-contract
as value. It could be another set of flags for other arches if somebody
wanted to do the same, fwiw. In JDK 8, for example, it's "-mno-fused-
madd -fno-strict-aliasing" for ppc64:
http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/file/2660b127b407/make/lib/CoreLibraries.gmk#l63

We need support for that flag (or a set of flags) when we optimize
fdlibm since otherwise we would lose precision. If the flag is empty
we'd not optimize as we can't guarantee precision. That's why we tie
optimization to the availability of that flag. The expectation is for
this flag to be available on gcc/clang arches only at this point. Does
that make sense?


Yes that makes sense - thanks. I didn't quite glean that from the comment:

  42 # If FDLIBM_CFLAGS is non-empty we know that we can optimize
  43 # fdlibm by adding those extra C flags. Currently GCC,
  44 # and clang only.
  45 ifneq ($(FDLIBM_CFLAGS), )
  46   BUILD_LIBFDLIBM_OPTIMIZATION := LOW

But now I can read it and understand.

Thanks,
David


Thanks,
Severin

Re: RFR 8210670 (S): Accept double-dash VM-name options at launch time

[mark . reinhold]

2018/9/12 14:16:00 -0700, mark.reinh...@oracle.com:
> ...
> 
> 8210670: Accept double-dash VM-name options at launch time
> 
>   Per JEP 293 (Guidelines for JDK Command-Line Tool Options) [2], enhance
>   the Java launcher so that `--server` is accepted to select the server VM,
>   in addition to `-server`, and likewise for any other VMs listed in the
>   $JDK/lib/jvm.cfg file.
> 
>   JBS: https://bugs.openjdk.java.net/browse/JDK-8210670
>   Webrev: http://cr.openjdk.java.net/~mr/rev/8210670/
> 
> Passes tier1 tests; tier2-3 in progress.

Addendum: Joe reminded me that this change should have a CSR, so here it
is: https://bugs.openjdk.java.net/browse/JDK-8210682 .  A review of that
too would be appreciated.

I’ve also revised the code for this change, and hence the webrev, to
show the double-dash VM-name options in the `java --help` output.

- Mark

Re: ByteArrayOutputStream should not have a new writeBytes method in Java 11

[Stuart Marks]

On 9/11/18 10:36 PM, Michael Rasmussen wrote:

Can a language syntax/feature be deprecated to produce a warning in javac?
Perhaps this one should.


We haven't reached the point of formally deprecating language features, but 
certain features like this one (and others) are certainly frowned upon by 
various style guides and reviewers. So, informally deprecated I guess.


I'd think the next step would be to have javac issue a lint warning.

Filed:

https://bugs.openjdk.java.net/browse/JDK-8210681

s'marks

RFR 8210670 (S): Accept double-dash VM-name options at launch time

[mark . reinhold]

To make this trivial enhancement I first had to fix some broken tests,
so there are two issues and two webrevs here.

8210669: Some launcher tests assume a pre-JDK 9 run-time image layout

  Two launcher tests, ExecutionEnvironment.java and Test7029048.java (in
  test/jdk/tools/launcher), still assume the old image layout in which a
  HotSpot shared library (libjava.so) is found in $JDK/lib/$ARCH/$VMTYPE
  on Linux and Solaris.  The intermediate $ARCH directory was removed in
  JDK 9, as part of JEP 220 (Modular Run-Time Images) [1].  Since that
  time these tests have been succeeding anyway, because they don’t fail
  when the requested VM is not found.

  The tests should be fixed to fail when the requested VM is not found,
  and to look for VM shared libraries in the proper location.

  JBS: https://bugs.openjdk.java.net/browse/JDK-8210669
  Webrev: http://cr.openjdk.java.net/~mr/rev/8210669/

8210670: Accept double-dash VM-name options at launch time

  Per JEP 293 (Guidelines for JDK Command-Line Tool Options) [2], enhance
  the Java launcher so that `--server` is accepted to select the server VM,
  in addition to `-server`, and likewise for any other VMs listed in the
  $JDK/lib/jvm.cfg file.

  JBS: https://bugs.openjdk.java.net/browse/JDK-8210670
  Webrev: http://cr.openjdk.java.net/~mr/rev/8210670/

Passes tier1 tests; tier2-3 in progress.

Thanks,
- Mark


[1] http://openjdk.java.net/jeps/220
[2] http://openjdk.java.net/jeps/293

Re: ByteArrayOutputStream should not have a new writeBytes method in Java

[Stuart Marks]

Cool, I didn't know that IntelliJ IDEA has such an inspection, but I'm not 
surprised.


Perhaps a volunteer can run this inspection over parts of the JDK code base and 
see what comes up, and possibly propose some patches. :-)


s'marks


On 9/11/18 10:34 PM, Tomasz Linkowski wrote:

Hello Stuart,

I'm not sure whether you're aware that IntelliJ IDEA has an automatic 
inspection named "C-style array declaration":
- description: 
https://github.com/JetBrains/intellij-community/blob/master/plugins/InspectionGadgets/src/inspectionDescriptions/CStyleArrayDeclaration.html
- logic: 
https://github.com/JetBrains/intellij-community/blob/master/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/style/CStyleArrayDeclarationInspection.java


You can run such single inspection over the entire project as described here:
https://www.jetbrains.com/help/idea/running-inspection-by-name.html

It will fix all such C-style array declarations for you automatically.

--
Regards,
Tomasz Linkowski


From: Stuart Marks mailto:stuart.ma...@oracle.com>>
To: "ullenb...@gmail.com "
mailto:ullenb...@gmail.com>>
Cc: "core-libs-dev@openjdk.java.net
" mailto:core-libs-dev@openjdk.java.net>>
Bcc:
Date: Tue, 11 Sep 2018 13:23:41 -0700
Subject: Re: ByteArrayOutputStream should not have a new writeBytes method
in Java 11 

2. even if, it should not be byte b[] but byte[] b 



Yeah we need to clean occurrences of this anachronistic array declaration
from the JDK. I noticed a few others nearby. It's startling that a new
occurrence has crept it. (Or maybe not, perhaps it was done to conform to
the nearby code.) 



Any volunteers to clean this up? 



An interesting exercise would be to write a detector for this declaration
style. 



s'marks

Re: SSL session cache default maximum number of entries

[Hohensee, Paul]

Thanks very much for investigating. We're aware that the cache size can be set 
by the user, but many of our users haven't done so because it hasn't been 
necessary, and boom.

Paul

On 9/11/18, 12:49 PM, "core-libs-dev on behalf of Sean Mullan" 
 
wrote:

Hi Paul,

Thank you for bringing this issue to our attention. While we agree that 
this does indeed seem like an issue that should be addressed, it is 
quite late in the JDK 11 schedule, and it does not appear to be a new 
issue introduced in JDK 11. We will be investigating this offline and 
will get back to you as soon as we can with more details. Offhand, I 
think that we would be able to change the default in an update release.

Also, you are probably already be aware of this, but you can use the 
SSLSessionContext.setSessionCacheSize() API as well as the 
"javax.net.ssl.sessionCacheSize" system property to customize the cache 
size.

--Sean

On 9/11/18 12:02 PM, Sean Mullan wrote:
> cross-posting to security-dev since this is related to SSL/TLS.
> 
> On 9/11/18 11:41 AM, Hohensee, Paul wrote:
>> The default value for the maximum number of entries in the SSL session 
>> cache (which is a SoftReference cache) is infinite, and the entry 
>> timeout is 24 hours. With larger heaps, we’re running into situations 
>> where the cache ends up with several million entries when the 24 hours 
>> are up. They’re then all invalidated at the same time, resulting in 
>> multi-minute pauses (effectively service failures). We’ve experimented 
>> with using 10k as the default maximum number of entries with good 
>> results (i.e., no latency increases due to sessions falling out of the 
>> cache). It’s late and a long shot for JDK11: we’d love to see it 
>> changed there because 11 is an LTS release and this is, at least 
>> nominally, a behavior change which might not be acceptable in 11u. 
>> What do people think?
>>
>> Thanks,
>>
>> Paul
>>

RFR(JDK12/JAXP/java.xml) 8207760: SAXException: Invalid UTF-16 surrogate detected: d83c ?

[Joe Wang]

Hi,

Please review a patch for a situation where a surrogate pair is at the 
edge of a buffer. What the existing impl did was to report it as an 
error. This patch fixes it by caching the high surrogate and prints it 
out along with the low surrogate. Similar issue exists also in the CDATA 
section and is fixed in this patch. The CDATA impl had a couple of bugs 
where an indent could be written inside the CDATA and an unicode 
character written in between two CDATA sections. Both are fixed in this 
patch.


JBS: https://bugs.openjdk.java.net/browse/JDK-8207760
webrevs: http://cr.openjdk.java.net/~joehw/jdk12/8207760/webrev/

Thanks,
Joe

Re: 8207690: Parsing API for classpath and similar path strings

[Jonathan Gibbons]

On 9/12/18 9:31 AM, Peter Levart wrote:



On 09/12/2018 04:30 PM, Roger Riggs wrote:

Hi Stuart,

The implementation retains the previous handling of empty path 
elements for URLClassPath
in the implementation.  The spec for the new methods is explicit 
about dropping empty elements.


For a library API, it is inadvisable to support implicit context such 
as the current working directory.

I received some offline comments about proposing too many methods and
dropped the variants that supported replacing empty path elements 
with an explicit path.


Keeping the empty path elements would simplify the spec though.


...and it's easy to .filter() them out if the parsing method returns 
Stream...

or .map() them to some default value, such as the current working directory.

-- Jon


Regards, Peter



Thanks, Roger


On 9/11/18 4:54 PM, Stuart Marks wrote:

Hi Roger,

 110 * Returns a list of path strings parsed from a string with 
empty paths removed.


The Unix shell and the Java launcher's -classpath processing treat 
an empty path entry as the current working directory. (I don't know 
what happens on Windows.) Removing empty paths thus would seem to 
change the semantics of search paths, at least on some systems.


s'marks

Re: [12]: RFR: 8209167: Use CLDR's time zone mappings for Windows

[Erik Joelsson]

Looks good.

/Erik


On 2018-09-12 10:33, naoto.s...@oracle.com wrote:

Hi Magnus, Erik,

Thank you for your comments. I updated the webrev according to your 
suggestions:


http://cr.openjdk.java.net/~naoto/8209167/webrev.02/

As to the duplicated "common" in the dependency, yes that's a separate 
issue. Since it's obvious, I included the fix with this changeset (it 
was actually described as a comment in the jira issue).


Naoto

On 9/12/18 9:08 AM, Erik Joelsson wrote:

On 2018-09-12 03:19, Magnus Ihse Bursie wrote:



On 2018-09-10 23:34, Naoto Sato wrote:

Hello,

Please review the fix to the following issue:

https://bugs.openjdk.java.net/browse/JDK-8209167

The proposed changeset is located at:

http://cr.openjdk.java.net/~naoto/8209167/webrev.01/


Some comments:
In make/copy/Copy-java.base.gmk:
+ifneq ($(findstring $(OPENJDK_TARGET_OS), aix),)

The findstring construct is hard to read and only needed when we 
test more than one OS. Please rewrite as a single ifeq test for aix.


In GensrcCLDR.gmk:
I don't understand the CLDR_WINTZMAPPINGS file? There's no rule to 
generate it, there's just a dependency..?


It's generated by the same rule as the other file. This is the 
easiest workaround for two files generated from the same rule. It 
sort of works (for clean builds and if the input is chagned), but 
won't handle all cases where one file is removed externally and the 
other is not. I suggested this construct to Naoto. Perhaps a comment 
explaining this would be good.
The removal of the duplicate "common", that seems like a separate 
bug fix?



It does indeed. Before this fix, the wildcards must have returned empty.

/Erik

/Magnus



This fix is to remove the hand crafted map file (lib/tzmappings) on 
Windows, which maps Windows time zones to Java's tzid. It is now 
dynamically generated at the build time, using CLDR's data file 
(windowsZones.xml)


Naoto

Re: Re-iterate JDK-6194856: Zip Files lose ALL ownership and permissions of the files

[Xueming Shen]

It's hard to get the ZipEntry API right to perfectly handle these 
platform specific
"file system attributes" especially given the potential performance and 
security

concern.

I would assume zipfs might be a better place to handle this if "really" 
desired,
in which already has the base to handle "file attributes". We can have a 
zipfs

based jar/zip somewhere to take advantage of its file system nature. For
example, start from "open/test/jdk/jdk/nio/zipfs/Demo.java" :-)

-Sherman


On 9/12/18, 9:18 AM, Martin Buchholz wrote:

In principle I support making system specific extensions to the ZIP
spec more supported, as other zip implementations do.  There is long
standing tension between Java trying to be highly portable and
providing access to the data you need.  It's already the case that
some implementation bits are not exposed but the zip implementation
handles it "magically" under the covers, e.g. IIRC the utf-8 bit.
There is also tension between ZipEntry as just a mutable struct and a
read-only interface to an existing zip file.  Performance of zip
implementations is critical so one would like to do lazy lookup.
Coming up with a good design for j.u.zip evolution is hard.

On Wed, Sep 12, 2018 at 7:44 AM, Langer, Christoph
  wrote:

Hi all,

I'm currently revisiting a long standing shortcoming of the java.util.zip (and 
java.util.jar) implementation.

The lack is that in the current implementation, Unix mode bits (e.g. rwx) are not 
stored/read from zip or jar files and the jar tool has no capabilities to store/preserve 
the mode information. There have been several bugs opened but they were closed with 
"Won't Fix". One of them is JDK-6194856, mentioned in this mail's subject [1]. 
Unfortunately there is no detailed reasoning given why that can't be done. Maybe these 
are compatibility or security issues? Maybe someone can enlighten me about previous 
discussions concerning this matter...

I personally can imagine contributing the following:

First step: Enrich java.util.zip.ZipEntry with some method(s) like 
getUnixPermissions() and setUnixPermissions().
For writing zip files it would mean depending on the availability of Unix permission information in a 
ZipEntry object, the entry version would be set to "3 - Unix" according to section "D.  
Explanation of fields", "version made by (2 bytes)" of the zip specification that is 
referenced in the current Javadoc for the java.util.zip package [2].
Reading Zip files would take into account the file attribute mapping of the version field 
and in case of "3 - Unix", it would read the permissions settings from the 
external file attributes section of the entry header.
This seems to be the de-facto standard of the GNU zip/unzip implementation.
With that addition the the java.util.zip API, developers using it would get the 
ability to cope with Unix permissions.

As a second step, I can imagine spending a flag for the jar utility to have it 
handle Unix permissions when packing/extracting.

Isn't that something that should be done? Or, if not, why?

Thanks and best regards in advance
Christoph

[1] https://bugs.openjdk.java.net/browse/JDK-6194856
[2] http://www.info-zip.org/doc/appnote-19970311-iz.zip

Re: [12]: RFR: 8209167: Use CLDR's time zone mappings for Windows

[naoto . sato]

Hi Magnus, Erik,

Thank you for your comments. I updated the webrev according to your 
suggestions:


http://cr.openjdk.java.net/~naoto/8209167/webrev.02/

As to the duplicated "common" in the dependency, yes that's a separate 
issue. Since it's obvious, I included the fix with this changeset (it 
was actually described as a comment in the jira issue).


Naoto

On 9/12/18 9:08 AM, Erik Joelsson wrote:

On 2018-09-12 03:19, Magnus Ihse Bursie wrote:



On 2018-09-10 23:34, Naoto Sato wrote:

Hello,

Please review the fix to the following issue:

https://bugs.openjdk.java.net/browse/JDK-8209167

The proposed changeset is located at:

http://cr.openjdk.java.net/~naoto/8209167/webrev.01/


Some comments:
In make/copy/Copy-java.base.gmk:
+ifneq ($(findstring $(OPENJDK_TARGET_OS), aix),)

The findstring construct is hard to read and only needed when we test 
more than one OS. Please rewrite as a single ifeq test for aix.


In GensrcCLDR.gmk:
I don't understand the CLDR_WINTZMAPPINGS file? There's no rule to 
generate it, there's just a dependency..?


It's generated by the same rule as the other file. This is the easiest 
workaround for two files generated from the same rule. It sort of works 
(for clean builds and if the input is chagned), but won't handle all 
cases where one file is removed externally and the other is not. I 
suggested this construct to Naoto. Perhaps a comment explaining this 
would be good.
The removal of the duplicate "common", that seems like a separate bug 
fix?



It does indeed. Before this fix, the wildcards must have returned empty.

/Erik

/Magnus



This fix is to remove the hand crafted map file (lib/tzmappings) on 
Windows, which maps Windows time zones to Java's tzid. It is now 
dynamically generated at the build time, using CLDR's data file 
(windowsZones.xml)


Naoto

JDK-8124977: deadlock between engineers? (cmdline encoding challenges on Windows)

[Anthony Vanelverdinghe]

Hi

What is the status of this issue [1]? It addresses some long-standing issues 
with Java's Unicode support on Windows and was contributed by a team of 
Microsoft engineers. However, it seems to have gone dormant right before the 
finish line, and I can't really figure out who's waiting for whom at this point.

A little reconstruction from what I could find:
In January 2015, Martin Sawicki made the initial proposal to address the 
cmdline encoding challenges on Windows [2]
>From June to September, there were ongoing discussions [3][4][5][6]
In November, this was shortly picked up again by the Oracle engineers [7]
In January 2016, after a ping by a Microsoft engineer, the discussions 
restarted [8]
In February 2016, the patch seemed nearly ready for integration, with an Oracle 
engineer asking whom to mention as contributors etc. [9]
Since then, I was unable to find any messages related to this issue

(Personally, I would love to see this issue progress, in order to be able to 
associate Java programs with file extensions on Windows. This is currently 
problematic, since a file containing Unicode characters will not get passed 
correctly as an argument to the Java program)

Kind regards,
Anthony

[1] https://bugs.openjdk.java.net/browse/JDK-8124977
[2] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-January/031068.html
[3] http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-June/034226.html
[4] http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-July/034488.html
[5] http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-August/034838.html
[6] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035466.html
[7] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-November/036769.html
[8] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-January/037927.html
[9] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-February/039013.html

Re: 8207690: Parsing API for classpath and similar path strings

[Peter Levart]

On 09/12/2018 04:30 PM, Roger Riggs wrote:

Hi Stuart,

The implementation retains the previous handling of empty path 
elements for URLClassPath
in the implementation.  The spec for the new methods is explicit about 
dropping empty elements.


For a library API, it is inadvisable to support implicit context such 
as the current working directory.

I received some offline comments about proposing too many methods and
dropped the variants that supported replacing empty path elements with 
an explicit path.


Keeping the empty path elements would simplify the spec though.


...and it's easy to .filter() them out if the parsing method returns 
Stream...


Regards, Peter



Thanks, Roger


On 9/11/18 4:54 PM, Stuart Marks wrote:

Hi Roger,

 110 * Returns a list of path strings parsed from a string with 
empty paths removed.


The Unix shell and the Java launcher's -classpath processing treat an 
empty path entry as the current working directory. (I don't know what 
happens on Windows.) Removing empty paths thus would seem to change 
the semantics of search paths, at least on some systems.


s'marks

Re: Re-iterate JDK-6194856: Zip Files lose ALL ownership and permissions of the files

[Martin Buchholz]

In principle I support making system specific extensions to the ZIP
spec more supported, as other zip implementations do.  There is long
standing tension between Java trying to be highly portable and
providing access to the data you need.  It's already the case that
some implementation bits are not exposed but the zip implementation
handles it "magically" under the covers, e.g. IIRC the utf-8 bit.
There is also tension between ZipEntry as just a mutable struct and a
read-only interface to an existing zip file.  Performance of zip
implementations is critical so one would like to do lazy lookup.
Coming up with a good design for j.u.zip evolution is hard.

On Wed, Sep 12, 2018 at 7:44 AM, Langer, Christoph
 wrote:
> Hi all,
>
> I'm currently revisiting a long standing shortcoming of the java.util.zip 
> (and java.util.jar) implementation.
>
> The lack is that in the current implementation, Unix mode bits (e.g. rwx) are 
> not stored/read from zip or jar files and the jar tool has no capabilities to 
> store/preserve the mode information. There have been several bugs opened but 
> they were closed with "Won't Fix". One of them is JDK-6194856, mentioned in 
> this mail's subject [1]. Unfortunately there is no detailed reasoning given 
> why that can't be done. Maybe these are compatibility or security issues? 
> Maybe someone can enlighten me about previous discussions concerning this 
> matter...
>
> I personally can imagine contributing the following:
>
> First step: Enrich java.util.zip.ZipEntry with some method(s) like 
> getUnixPermissions() and setUnixPermissions().
> For writing zip files it would mean depending on the availability of Unix 
> permission information in a ZipEntry object, the entry version would be set 
> to "3 - Unix" according to section "D.  Explanation of fields", "version made 
> by (2 bytes)" of the zip specification that is referenced in the current 
> Javadoc for the java.util.zip package [2].
> Reading Zip files would take into account the file attribute mapping of the 
> version field and in case of "3 - Unix", it would read the permissions 
> settings from the external file attributes section of the entry header.
> This seems to be the de-facto standard of the GNU zip/unzip implementation.
> With that addition the the java.util.zip API, developers using it would get 
> the ability to cope with Unix permissions.
>
> As a second step, I can imagine spending a flag for the jar utility to have 
> it handle Unix permissions when packing/extracting.
>
> Isn't that something that should be done? Or, if not, why?
>
> Thanks and best regards in advance
> Christoph
>
> [1] https://bugs.openjdk.java.net/browse/JDK-6194856
> [2] http://www.info-zip.org/doc/appnote-19970311-iz.zip
>

Re: [12]: RFR: Use CLDR's time zone mappings for Windows

[Erik Joelsson]

On 2018-09-12 03:19, Magnus Ihse Bursie wrote:



On 2018-09-10 23:34, Naoto Sato wrote:

Hello,

Please review the fix to the following issue:

https://bugs.openjdk.java.net/browse/JDK-8209167

The proposed changeset is located at:

http://cr.openjdk.java.net/~naoto/8209167/webrev.01/


Some comments:
In make/copy/Copy-java.base.gmk:
+ifneq ($(findstring $(OPENJDK_TARGET_OS), aix),)

The findstring construct is hard to read and only needed when we test 
more than one OS. Please rewrite as a single ifeq test for aix.


In GensrcCLDR.gmk:
I don't understand the CLDR_WINTZMAPPINGS file? There's no rule to 
generate it, there's just a dependency..?


It's generated by the same rule as the other file. This is the easiest 
workaround for two files generated from the same rule. It sort of works 
(for clean builds and if the input is chagned), but won't handle all 
cases where one file is removed externally and the other is not. I 
suggested this construct to Naoto. Perhaps a comment explaining this 
would be good.
The removal of the duplicate "common", that seems like a separate bug 
fix?



It does indeed. Before this fix, the wildcards must have returned empty.

/Erik

/Magnus



This fix is to remove the hand crafted map file (lib/tzmappings) on 
Windows, which maps Windows time zones to Java's tzid. It is now 
dynamically generated at the build time, using CLDR's data file 
(windowsZones.xml)


Naoto

Re: Runtime.exec : vfork() concerns and a fix proposal

[Martin Buchholz]

On Wed, Sep 12, 2018 at 8:16 AM, David Lloyd  wrote:

> Seems worthwhile though, given vfork's now-10-year-old obsolescence.
> It looks like Linux is the only platform still using vfork for
> ProcessImpl in OpenJDK.

I'm fine with switching to posix_spawn on Linux as long as we don't
reintroduce the memory overcommit problem, which would be a
deal-breaker.  But glibc posix_spawn will use either vfork or clone
with CLONE_VFORK, so we should be OK.

Re: Runtime.exec : vfork() concerns and a fix proposal

[Martin Buchholz]

On Wed, Sep 12, 2018 at 7:33 AM, David Lloyd  wrote:
>> The child process is created using vfork(2) instead of fork(2) when [...] 
>> file_actions is NULL and the spawn-flags element of the attributes object 
>> pointed to by attrp does not contain POSIX_SPAWN_SETSIGMASK, 
>> POSIX_SPAWN_SETSIGDEF, POSIX_SPAWN_SETSCHEDPARAM, POSIX_SPAWN_SETSCHEDULER, 
>> POSIX_SPAWN_SETPGROUP, or POSIX_SPAWN_RESETIDS.

Yes, the posix_spawn man page says that, but
https://github.com/bminor/glibc/blob/master/sysdeps/unix/sysv/linux/spawni.c#L36
says:

/* The Linux implementation of posix_spawn{p} uses the clone syscall directly
with CLONE_VM and CLONE_VFORK flags and an allocated stack. The new stack
and start function solves most the vfork limitation (possible parent
clobber due stack spilling).

Re: Runtime.exec : vfork() concerns and a fix proposal

[David Lloyd]

On Wed, Sep 12, 2018 at 9:58 AM Roger Riggs  wrote:
>
> Hi David,
>
> How does your proposal differ from the current posix_spawn and
> jspawnhelper MODE_POSIX_SPAWN
> implementation available on Solaris and AIX?

Reading through the code, it ends up being pretty much identical
AFAICT; if I understand correctly, Linux would be using vfork (or its
equivalent) internally in this case.

> This area is sensitive enough that it would be prudent to implement it
> as a new mode in ProcessImpl/ProcessImpl; retaining the existing options.
> Changing the default could be a build option and would require extensive
> testing and a long stability period.

Seems worthwhile though, given vfork's now-10-year-old obsolescence.
It looks like Linux is the only platform still using vfork for
ProcessImpl in OpenJDK.

-- 
- DML

Re: Runtime.exec : vfork() concerns and a fix proposal

[Roger Riggs]

Hi David,

How does your proposal differ from the current posix_spawn and 
jspawnhelper MODE_POSIX_SPAWN

implementation available on Solaris and AIX?
This area is sensitive enough that it would be prudent to implement it 
as a new mode in ProcessImpl/ProcessImpl; retaining the existing options.
Changing the default could be a build option and would require extensive 
testing and a long stability period.


Regards, Roger

On 9/12/18 10:33 AM, David Lloyd wrote:

On Wed, Sep 12, 2018 at 9:21 AM Martin Buchholz  wrote:

I agree that your proposal makes the implementation more robust, so if
you are willing to implement it and it doesn't cost too much, then I
would support it.  The current implementation doesn't seem to cause
trouble in practice, or at least we don't see any bug reports.

When you benchmark with open file descriptors, try with and without
FD_CLOEXEC flag set.  Almost all fds should be created with the
FD_CLOEXEC flag set - openjdk is moving in that direction.  When there
are many open files, your implementation may be a weird sort of
optimization.

On Tue, Sep 11, 2018 at 10:51 AM, Thomas Stüfe  wrote:


The point of this technique is that we minimize the window in the
child between vfork and the first exec. In fact, we are now fully
POSIX compliant.

2004-POSIX compliant ... but in the mean time they removed vfork().
But vfork is critical for Google (and probably others)  due to the
momentary overcommit problem on Linux.  Or has that changed somehow?

If the lack of POSIX support is a problem, posix_spawn could possibly
be used instead in this case:


The child process is created using vfork(2) instead of fork(2) when [...] 
file_actions is NULL and the spawn-flags element of the attributes object 
pointed to by attrp does not contain POSIX_SPAWN_SETSIGMASK, 
POSIX_SPAWN_SETSIGDEF, POSIX_SPAWN_SETSCHEDPARAM, POSIX_SPAWN_SETSCHEDULER, 
POSIX_SPAWN_SETPGROUP, or POSIX_SPAWN_RESETIDS.

This would only work if the helper program is run directly though.

Re-iterate JDK-6194856: Zip Files lose ALL ownership and permissions of the files

[Langer, Christoph]

Hi all,

I'm currently revisiting a long standing shortcoming of the java.util.zip (and 
java.util.jar) implementation.

The lack is that in the current implementation, Unix mode bits (e.g. rwx) are 
not stored/read from zip or jar files and the jar tool has no capabilities to 
store/preserve the mode information. There have been several bugs opened but 
they were closed with "Won't Fix". One of them is JDK-6194856, mentioned in 
this mail's subject [1]. Unfortunately there is no detailed reasoning given why 
that can't be done. Maybe these are compatibility or security issues? Maybe 
someone can enlighten me about previous discussions concerning this matter...

I personally can imagine contributing the following:

First step: Enrich java.util.zip.ZipEntry with some method(s) like 
getUnixPermissions() and setUnixPermissions().
For writing zip files it would mean depending on the availability of Unix 
permission information in a ZipEntry object, the entry version would be set to 
"3 - Unix" according to section "D.  Explanation of fields", "version made by 
(2 bytes)" of the zip specification that is referenced in the current Javadoc 
for the java.util.zip package [2].
Reading Zip files would take into account the file attribute mapping of the 
version field and in case of "3 - Unix", it would read the permissions settings 
from the external file attributes section of the entry header.
This seems to be the de-facto standard of the GNU zip/unzip implementation.
With that addition the the java.util.zip API, developers using it would get the 
ability to cope with Unix permissions.

As a second step, I can imagine spending a flag for the jar utility to have it 
handle Unix permissions when packing/extracting.

Isn't that something that should be done? Or, if not, why?

Thanks and best regards in advance
Christoph

[1] https://bugs.openjdk.java.net/browse/JDK-6194856
[2] http://www.info-zip.org/doc/appnote-19970311-iz.zip

Re: Runtime.exec : vfork() concerns and a fix proposal

[David Lloyd]

On Wed, Sep 12, 2018 at 9:21 AM Martin Buchholz  wrote:
>
> I agree that your proposal makes the implementation more robust, so if
> you are willing to implement it and it doesn't cost too much, then I
> would support it.  The current implementation doesn't seem to cause
> trouble in practice, or at least we don't see any bug reports.
>
> When you benchmark with open file descriptors, try with and without
> FD_CLOEXEC flag set.  Almost all fds should be created with the
> FD_CLOEXEC flag set - openjdk is moving in that direction.  When there
> are many open files, your implementation may be a weird sort of
> optimization.
>
> On Tue, Sep 11, 2018 at 10:51 AM, Thomas Stüfe  
> wrote:
>
> > The point of this technique is that we minimize the window in the
> > child between vfork and the first exec. In fact, we are now fully
> > POSIX compliant.
>
> 2004-POSIX compliant ... but in the mean time they removed vfork().
> But vfork is critical for Google (and probably others)  due to the
> momentary overcommit problem on Linux.  Or has that changed somehow?

If the lack of POSIX support is a problem, posix_spawn could possibly
be used instead in this case:

> The child process is created using vfork(2) instead of fork(2) when [...] 
> file_actions is NULL and the spawn-flags element of the attributes object 
> pointed to by attrp does not contain POSIX_SPAWN_SETSIGMASK, 
> POSIX_SPAWN_SETSIGDEF, POSIX_SPAWN_SETSCHEDPARAM, POSIX_SPAWN_SETSCHEDULER, 
> POSIX_SPAWN_SETPGROUP, or POSIX_SPAWN_RESETIDS.

This would only work if the helper program is run directly though.

-- 
- DML

Re: 8207690: Parsing API for classpath and similar path strings

[Roger Riggs]

Hi Stuart,

The implementation retains the previous handling of empty path elements 
for URLClassPath
in the implementation.  The spec for the new methods is explicit about 
dropping empty elements.


For a library API, it is inadvisable to support implicit context such as 
the current working directory.

I received some offline comments about proposing too many methods and
dropped the variants that supported replacing empty path elements with 
an explicit path.


Keeping the empty path elements would simplify the spec though.

Thanks, Roger


On 9/11/18 4:54 PM, Stuart Marks wrote:

Hi Roger,

 110  * Returns a list of path strings parsed from a string with 
empty paths removed.


The Unix shell and the Java launcher's -classpath processing treat an 
empty path entry as the current working directory. (I don't know what 
happens on Windows.) Removing empty paths thus would seem to change 
the semantics of search paths, at least on some systems.


s'marks

On 9/10/18 11:16 AM, Roger Riggs wrote:
Please review the API and implementation of an API to parse Path 
strings.
Two methods are added to java.nio.file.Paths to parse a string using 
the path separator delimiter
and return either List or List.  Empty path elements 
are ignored.


For compatibility with current URLClassPath behavior the internal 
implementation handles

replacement of empty paths.

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-8207690_parsing_api_for_classpath_and_similar_path_strings/ 



CSR:
   https://bugs.openjdk.java.net/browse/JDK-8208208

Thanks, Roger

Re: 8207690: Parsing API for classpath and similar path strings

[Roger Riggs]

Hi,

My original intention was to provide minimal support for parsing a 
property value

and handling OS specific behaviors (quoting).

java.nio.file.Paths is a bit of backwater (now that Path.of was added) 
and its factories are dedicated to handling Paths.


I considered a new type representing a search path but that seemed to 
expand the scope beyond the minimum.


Search path logic is not one of the high profile functions used by lots 
of developers and there seem to be many variations on how they are used.


It is also a slippery slope since it could easily expand to add many 
useful functions

that applications already deal with themselves.

 - Searching the path for a particular file or directory
 - Supporting patterns for matching files on the path, using filters or 
glob patterns

 - Caching of previous lookups or directory contents
 - Optimizing the search path, ignoring duplicate entries, etc.

If that's the direction this should go it expands the scope quite a bit.

Thanks, Roger

On 9/11/18 12:49 PM, mark.reinh...@oracle.com wrote:

2018/9/11 4:50:49 -0700, chris.hega...@oracle.com:

On 11 Sep 2018, at 10:24, Alan Bateman  wrote:
On 10/09/2018 21:55, Roger Riggs wrote:

Nope! there's quoting on Windows that gets short changed with that work around.

Other opinions?, Suggestions?

One suggestion is reduce this down to one method that returns a
stream rather than a collection. It could work lazily if you
want. Something like:

  Stream splitSearchPath(String input)

I agree with Alan, this seems like the lowest-order primitive.

Agreed, but it begs a question that applies to all these alternatives:
Why does this API belong in java.nio.file.Paths?

In other words, search paths are a different abstraction from filesystem
paths.  Should they have their own class, even if it only winds up with
a couple of static utility methods?  java.nio.file.SearchPath?

 public class SearchPath {
 public static Stream splitToStream(String searchPath);
 public static List splitToPaths(String searchPath);
 public static String join(Collection);
 }

- Mark

Re: Runtime.exec : vfork() concerns and a fix proposal

[Martin Buchholz]

I agree that your proposal makes the implementation more robust, so if
you are willing to implement it and it doesn't cost too much, then I
would support it.  The current implementation doesn't seem to cause
trouble in practice, or at least we don't see any bug reports.

When you benchmark with open file descriptors, try with and without
FD_CLOEXEC flag set.  Almost all fds should be created with the
FD_CLOEXEC flag set - openjdk is moving in that direction.  When there
are many open files, your implementation may be a weird sort of
optimization.

On Tue, Sep 11, 2018 at 10:51 AM, Thomas Stüfe  wrote:

> The point of this technique is that we minimize the window in the
> child between vfork and the first exec. In fact, we are now fully
> POSIX compliant.

2004-POSIX compliant ... but in the mean time they removed vfork().
But vfork is critical for Google (and probably others)  due to the
momentary overcommit problem on Linux.  Or has that changed somehow?

Re: RFR: 8210416: [linux] Poor StrictMath performance due to non-optimized compilation

[Gustavo Romero]

Hi Andrew,

On 09/12/2018 12:51 AM, Gustavo Romero wrote:


Maybe Andrew can enlight us.


I was not sure if 'enlight us' was the correct form here, so I did some search
and I found with horror today that 'enlighten us' can also be considered an
insult (!?).

That's really not the case here. So if it's really possible to interpret it in
a non-positive / non-cordial way I sincerely apologize for the wrong use.

I was really only asking for an advice. :)


Regards,
Gustavo

Re: RFR(s): 8210549: Runtime.exec: in closeDescriptors(), use FD_CLOEXEC instead of close()

[Martin Buchholz]

In 2018, we can assume everyone has implemented FD_CLOEXEC.

Back in 1970, 20 file descriptors was enough for any program, but
today they're effectively unlimited, so futilely closing _SC_OPEN_MAX
descriptors has become unreasonable (as you say).  OTOH, there is
still no standard way of ensuring all our file descriptors are closed.
Do we need the fallback on your beloved HP-UX?  Should we simply close
the first 64k file descriptors and pray, as is traditional?

freebsd offers closefrom(2)
https://www.freebsd.org/cgi/man.cgi?query=closefrom&sektion=2&manpath=freebsd-release-ports
and today we can autoconfiscate the existence of closefrom and use
that where available (BSD and Solaris?)

closeDescriptors needs a better name now - maybe ensureDescriptorsAreClosed ?

I'd give this function a javadoc style docstring.

(Finding the FD_DIR directory has become rather ugly, especially the
AIX-specific code)

Re: Runtime.exec : vfork() concerns and a fix proposal

[David Lloyd]

On Wed, Sep 12, 2018 at 2:04 AM Thomas Stüfe  wrote:
> Hi David,
>
> On Tue, Sep 11, 2018 at 8:29 PM, David Lloyd  wrote:
> > I think this is a cool idea.
>
> Thanks. I think I did not come up with it though, I think the
> technique was known already.
>
> > Do you have any performance numbers?
>
> Sure:
>
> small program, just spawning off /bin/true a 1000 times, measured on
> my t450s running Ubuntu 16.4:
>
> Number open files:  1000   10
>
> openjdk8:  305ms 1.5s115s
> sapjvm8:   721ms  2.3s   142s
>
> factor 2.4   1.531.23

This doesn't seem too bad, even for a first implementation.  I wonder
if it could be improved further using e.g. shared memory to reduce
syscalls?

-- 
- DML

Re: RFR(s): 8210549: Runtime.exec: in closeDescriptors(), use FD_CLOEXEC instead of close()

[Martin Buchholz]

On Wed, Sep 12, 2018 at 5:46 AM, Thomas Stüfe  wrote:
> On Wed, Sep 12, 2018 at 2:30 PM, Martin Buchholz  wrote:

> Btw, should I retry the readdir() on EINTR?

If I read
http://pubs.opengroup.org/onlinepubs/9699919799/functions/readdir.html
correctly, then readdir should never fail with EINTR

Re: RFR(s): 8210549: Runtime.exec: in closeDescriptors(), use FD_CLOEXEC instead of close()

[Thomas Stüfe]

On Wed, Sep 12, 2018 at 2:30 PM, Martin Buchholz  wrote:
> The reference
> from_fd + 2
> needs updating since it assumes two file descriptors have already been closed?

Good catch, I will fix that.

Btw, should I retry the readdir() on EINTR?

..Thomas

>
> On Tue, Sep 11, 2018 at 10:27 AM, Thomas Stüfe  
> wrote:
>> Hi all,
>>
>> May I please have reviews for this small fix:
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8210549
>>
>> patch (full): 
>> http://cr.openjdk.java.net/~stuefe/webrevs/8210549-Use-FD_CLOEXEC-instead-of-close/webrev.00/webrev/
>> patch (minimal alternative):
>> http://cr.openjdk.java.net/~stuefe/webrevs/8210549-Use-FD_CLOEXEC-instead-of-close/webrev.minimal/webrev/
>>
>> See also prior discussion:
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-September/055173.html
>>
>> Submit tests ran with full patch successfully through.
>>
>> ---
>>
>> Background:
>>
>> After fork()/vfork() and before exec(), the child process needs to
>> close all inherited file descriptors apart from the stdin/out/err pipe
>> ends. We do this by iterating thru all file descriptors in
>> /proc//fd or whatever the equivalent is on that platform. This is
>> done using opendir(), readdir().
>>
>> We then close all these file descriptors using close().
>>
>> The problem with that technique is that we may accidentally close the
>> file descriptor opendir() is using internally to read the directory
>> content of /proc//fd, thereby sawing off the branch we are
>> sitting on. The coding does some magic to attempt to prevent this:
>>
>> 
>> 86/* We're trying to close all file descriptors, but opendir() might
>> 87 * itself be implemented using a file descriptor, and we certainly
>> 88 * don't want to close that while it's in use.  We assume that if
>> 89 * opendir() is implemented using a file descriptor, then it uses
>> 90 * the lowest numbered file descriptor, just like open().  So we
>> 91 * close a couple explicitly.  */
>> 92
>> 93close(from_fd);  /* for possible use by opendir() */
>> 94close(from_fd + 1);  /* another one for good luck */
>>
>> ...
>> 108while ((dirp = readdir64(dp)) != NULL) {
>> 109int fd;
>> 110if (isAsciiDigit(dirp->d_name[0]) &&
>> 111(fd = strtol(dirp->d_name, NULL, 10)) >= from_fd +
>> 2)Improve "close all filedescriptors" coding.
>> 112close(fd);
>> 113}
>>
>> 
>>
>> This workaround can be removed if, instead of outright closing the
>> file descriptor in the loop, we were to set the file descriptor to
>> FD_CLOEXEC. Closing all descriptors would be defered to the actual
>> exec() call some milliseconds later.
>>
>> 
>>
>> The patch changes the close() call to fcntl(FD_CLOEXEC). This removes
>> the need for the workaround as described.
>>
>> In addition to that, the full version of the patch also adds error
>> handling to the readdir() loop.
>>
>> But note that this exposes us to a behavioral change should we run
>> into a readdir() error:
>>
>> - currently, we would leave the remaining file descriptors quietly
>> unclosed. This usually would have no effect, but in rare cases may
>> lead to difficult-to-analyze errors when child processes accidentally
>> disturb parent process IO.
>>
>> - With this patch, we will fail if readdir fails. However, we do not
>> just yet fail the Runtime.exec() call, but enter a fallback code
>> section here:
>>
>> 364 /* close everything */
>> 365 if (closeDescriptors() == 0) { /* failed,  close the old way */
>> 366 int max_fd = (int)sysconf(_SC_OPEN_MAX);
>> 367 int fd;
>> 368 for (fd = FAIL_FILENO + 1; fd < max_fd; fd++)
>> 369 if (close(fd) == -1 && errno != EBADF)
>> 370 goto WhyCantJohnnyExec;
>> 371 }
>>
>> I am not convinced this is a good fallback. _SC_OPEN_MAX depends on
>> ulimit -Hn and may be high. On my Ubuntu 16.4 box, default value after
>> installation is 1048576. When we hit this fallback code, it takes
>> almost a full second to spawn a single child process. In fork
>> intensive scenarios this can get pretty bad.
>>
>> What do you think?
>>
>> Thanks & Best Regards,
>>
>> Thomas

Re: RFR(s): 8210549: Runtime.exec: in closeDescriptors(), use FD_CLOEXEC instead of close()

[Martin Buchholz]

The reference
from_fd + 2
needs updating since it assumes two file descriptors have already been closed?

On Tue, Sep 11, 2018 at 10:27 AM, Thomas Stüfe  wrote:
> Hi all,
>
> May I please have reviews for this small fix:
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8210549
>
> patch (full): 
> http://cr.openjdk.java.net/~stuefe/webrevs/8210549-Use-FD_CLOEXEC-instead-of-close/webrev.00/webrev/
> patch (minimal alternative):
> http://cr.openjdk.java.net/~stuefe/webrevs/8210549-Use-FD_CLOEXEC-instead-of-close/webrev.minimal/webrev/
>
> See also prior discussion:
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-September/055173.html
>
> Submit tests ran with full patch successfully through.
>
> ---
>
> Background:
>
> After fork()/vfork() and before exec(), the child process needs to
> close all inherited file descriptors apart from the stdin/out/err pipe
> ends. We do this by iterating thru all file descriptors in
> /proc//fd or whatever the equivalent is on that platform. This is
> done using opendir(), readdir().
>
> We then close all these file descriptors using close().
>
> The problem with that technique is that we may accidentally close the
> file descriptor opendir() is using internally to read the directory
> content of /proc//fd, thereby sawing off the branch we are
> sitting on. The coding does some magic to attempt to prevent this:
>
> 
> 86/* We're trying to close all file descriptors, but opendir() might
> 87 * itself be implemented using a file descriptor, and we certainly
> 88 * don't want to close that while it's in use.  We assume that if
> 89 * opendir() is implemented using a file descriptor, then it uses
> 90 * the lowest numbered file descriptor, just like open().  So we
> 91 * close a couple explicitly.  */
> 92
> 93close(from_fd);  /* for possible use by opendir() */
> 94close(from_fd + 1);  /* another one for good luck */
>
> ...
> 108while ((dirp = readdir64(dp)) != NULL) {
> 109int fd;
> 110if (isAsciiDigit(dirp->d_name[0]) &&
> 111(fd = strtol(dirp->d_name, NULL, 10)) >= from_fd +
> 2)Improve "close all filedescriptors" coding.
> 112close(fd);
> 113}
>
> 
>
> This workaround can be removed if, instead of outright closing the
> file descriptor in the loop, we were to set the file descriptor to
> FD_CLOEXEC. Closing all descriptors would be defered to the actual
> exec() call some milliseconds later.
>
> 
>
> The patch changes the close() call to fcntl(FD_CLOEXEC). This removes
> the need for the workaround as described.
>
> In addition to that, the full version of the patch also adds error
> handling to the readdir() loop.
>
> But note that this exposes us to a behavioral change should we run
> into a readdir() error:
>
> - currently, we would leave the remaining file descriptors quietly
> unclosed. This usually would have no effect, but in rare cases may
> lead to difficult-to-analyze errors when child processes accidentally
> disturb parent process IO.
>
> - With this patch, we will fail if readdir fails. However, we do not
> just yet fail the Runtime.exec() call, but enter a fallback code
> section here:
>
> 364 /* close everything */
> 365 if (closeDescriptors() == 0) { /* failed,  close the old way */
> 366 int max_fd = (int)sysconf(_SC_OPEN_MAX);
> 367 int fd;
> 368 for (fd = FAIL_FILENO + 1; fd < max_fd; fd++)
> 369 if (close(fd) == -1 && errno != EBADF)
> 370 goto WhyCantJohnnyExec;
> 371 }
>
> I am not convinced this is a good fallback. _SC_OPEN_MAX depends on
> ulimit -Hn and may be high. On my Ubuntu 16.4 box, default value after
> installation is 1048576. When we hit this fallback code, it takes
> almost a full second to spawn a single child process. In fork
> intensive scenarios this can get pretty bad.
>
> What do you think?
>
> Thanks & Best Regards,
>
> Thomas

Re: RFR(s): 8210549: Runtime.exec: in closeDescriptors(), use FD_CLOEXEC instead of close()

[Thomas Stüfe]

Thank you Christoph :)

On Wed, Sep 12, 2018 at 1:47 PM, Langer, Christoph
 wrote:
> Hi Thomas,
>
> I like the full patch 😊 +1
>
> Best regards
> Christoph
>
>> -Original Message-
>> From: core-libs-dev  On Behalf
>> Of Thomas Stüfe
>> Sent: Dienstag, 11. September 2018 19:27
>> To: Java Core Libs 
>> Subject: RFR(s): 8210549: Runtime.exec: in closeDescriptors(), use
>> FD_CLOEXEC instead of close()
>>
>> Hi all,
>>
>> May I please have reviews for this small fix:
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8210549
>>
>> patch (full): http://cr.openjdk.java.net/~stuefe/webrevs/8210549-Use-
>> FD_CLOEXEC-instead-of-close/webrev.00/webrev/
>> patch (minimal alternative):
>> http://cr.openjdk.java.net/~stuefe/webrevs/8210549-Use-FD_CLOEXEC-
>> instead-of-close/webrev.minimal/webrev/
>>
>> See also prior discussion:
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-
>> September/055173.html
>>
>> Submit tests ran with full patch successfully through.
>>
>> ---
>>
>> Background:
>>
>> After fork()/vfork() and before exec(), the child process needs to
>> close all inherited file descriptors apart from the stdin/out/err pipe
>> ends. We do this by iterating thru all file descriptors in
>> /proc//fd or whatever the equivalent is on that platform. This is
>> done using opendir(), readdir().
>>
>> We then close all these file descriptors using close().
>>
>> The problem with that technique is that we may accidentally close the
>> file descriptor opendir() is using internally to read the directory
>> content of /proc//fd, thereby sawing off the branch we are
>> sitting on. The coding does some magic to attempt to prevent this:
>>
>> 
>> 86/* We're trying to close all file descriptors, but opendir() might
>> 87 * itself be implemented using a file descriptor, and we certainly
>> 88 * don't want to close that while it's in use.  We assume that if
>> 89 * opendir() is implemented using a file descriptor, then it uses
>> 90 * the lowest numbered file descriptor, just like open().  So we
>> 91 * close a couple explicitly.  */
>> 92
>> 93close(from_fd);  /* for possible use by opendir() */
>> 94close(from_fd + 1);  /* another one for good luck */
>>
>> ...
>> 108while ((dirp = readdir64(dp)) != NULL) {
>> 109int fd;
>> 110if (isAsciiDigit(dirp->d_name[0]) &&
>> 111(fd = strtol(dirp->d_name, NULL, 10)) >= from_fd +
>> 2)Improve "close all filedescriptors" coding.
>> 112close(fd);
>> 113}
>>
>> 
>>
>> This workaround can be removed if, instead of outright closing the
>> file descriptor in the loop, we were to set the file descriptor to
>> FD_CLOEXEC. Closing all descriptors would be defered to the actual
>> exec() call some milliseconds later.
>>
>> 
>>
>> The patch changes the close() call to fcntl(FD_CLOEXEC). This removes
>> the need for the workaround as described.
>>
>> In addition to that, the full version of the patch also adds error
>> handling to the readdir() loop.
>>
>> But note that this exposes us to a behavioral change should we run
>> into a readdir() error:
>>
>> - currently, we would leave the remaining file descriptors quietly
>> unclosed. This usually would have no effect, but in rare cases may
>> lead to difficult-to-analyze errors when child processes accidentally
>> disturb parent process IO.
>>
>> - With this patch, we will fail if readdir fails. However, we do not
>> just yet fail the Runtime.exec() call, but enter a fallback code
>> section here:
>>
>> 364 /* close everything */
>> 365 if (closeDescriptors() == 0) { /* failed,  close the old way */
>> 366 int max_fd = (int)sysconf(_SC_OPEN_MAX);
>> 367 int fd;
>> 368 for (fd = FAIL_FILENO + 1; fd < max_fd; fd++)
>> 369 if (close(fd) == -1 && errno != EBADF)
>> 370 goto WhyCantJohnnyExec;
>> 371 }
>>
>> I am not convinced this is a good fallback. _SC_OPEN_MAX depends on
>> ulimit -Hn and may be high. On my Ubuntu 16.4 box, default value after
>> installation is 1048576. When we hit this fallback code, it takes
>> almost a full second to spawn a single child process. In fork
>> intensive scenarios this can get pretty bad.
>>
>> What do you think?
>>
>> Thanks & Best Regards,
>>
>> Thomas

RE: RFR(s): 8210549: Runtime.exec: in closeDescriptors(), use FD_CLOEXEC instead of close()

[Langer, Christoph]

Hi Thomas,

I like the full patch 😊 +1

Best regards
Christoph

> -Original Message-
> From: core-libs-dev  On Behalf
> Of Thomas Stüfe
> Sent: Dienstag, 11. September 2018 19:27
> To: Java Core Libs 
> Subject: RFR(s): 8210549: Runtime.exec: in closeDescriptors(), use
> FD_CLOEXEC instead of close()
> 
> Hi all,
> 
> May I please have reviews for this small fix:
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8210549
> 
> patch (full): http://cr.openjdk.java.net/~stuefe/webrevs/8210549-Use-
> FD_CLOEXEC-instead-of-close/webrev.00/webrev/
> patch (minimal alternative):
> http://cr.openjdk.java.net/~stuefe/webrevs/8210549-Use-FD_CLOEXEC-
> instead-of-close/webrev.minimal/webrev/
> 
> See also prior discussion:
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-
> September/055173.html
> 
> Submit tests ran with full patch successfully through.
> 
> ---
> 
> Background:
> 
> After fork()/vfork() and before exec(), the child process needs to
> close all inherited file descriptors apart from the stdin/out/err pipe
> ends. We do this by iterating thru all file descriptors in
> /proc//fd or whatever the equivalent is on that platform. This is
> done using opendir(), readdir().
> 
> We then close all these file descriptors using close().
> 
> The problem with that technique is that we may accidentally close the
> file descriptor opendir() is using internally to read the directory
> content of /proc//fd, thereby sawing off the branch we are
> sitting on. The coding does some magic to attempt to prevent this:
> 
> 
> 86/* We're trying to close all file descriptors, but opendir() might
> 87 * itself be implemented using a file descriptor, and we certainly
> 88 * don't want to close that while it's in use.  We assume that if
> 89 * opendir() is implemented using a file descriptor, then it uses
> 90 * the lowest numbered file descriptor, just like open().  So we
> 91 * close a couple explicitly.  */
> 92
> 93close(from_fd);  /* for possible use by opendir() */
> 94close(from_fd + 1);  /* another one for good luck */
> 
> ...
> 108while ((dirp = readdir64(dp)) != NULL) {
> 109int fd;
> 110if (isAsciiDigit(dirp->d_name[0]) &&
> 111(fd = strtol(dirp->d_name, NULL, 10)) >= from_fd +
> 2)Improve "close all filedescriptors" coding.
> 112close(fd);
> 113}
> 
> 
> 
> This workaround can be removed if, instead of outright closing the
> file descriptor in the loop, we were to set the file descriptor to
> FD_CLOEXEC. Closing all descriptors would be defered to the actual
> exec() call some milliseconds later.
> 
> 
> 
> The patch changes the close() call to fcntl(FD_CLOEXEC). This removes
> the need for the workaround as described.
> 
> In addition to that, the full version of the patch also adds error
> handling to the readdir() loop.
> 
> But note that this exposes us to a behavioral change should we run
> into a readdir() error:
> 
> - currently, we would leave the remaining file descriptors quietly
> unclosed. This usually would have no effect, but in rare cases may
> lead to difficult-to-analyze errors when child processes accidentally
> disturb parent process IO.
> 
> - With this patch, we will fail if readdir fails. However, we do not
> just yet fail the Runtime.exec() call, but enter a fallback code
> section here:
> 
> 364 /* close everything */
> 365 if (closeDescriptors() == 0) { /* failed,  close the old way */
> 366 int max_fd = (int)sysconf(_SC_OPEN_MAX);
> 367 int fd;
> 368 for (fd = FAIL_FILENO + 1; fd < max_fd; fd++)
> 369 if (close(fd) == -1 && errno != EBADF)
> 370 goto WhyCantJohnnyExec;
> 371 }
> 
> I am not convinced this is a good fallback. _SC_OPEN_MAX depends on
> ulimit -Hn and may be high. On my Ubuntu 16.4 box, default value after
> installation is 1048576. When we hit this fallback code, it takes
> almost a full second to spawn a single child process. In fork
> intensive scenarios this can get pretty bad.
> 
> What do you think?
> 
> Thanks & Best Regards,
> 
> Thomas

RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives

[Langer, Christoph]

Looks good to me.

> -Original Message-
> From: Baesken, Matthias
> Sent: Mittwoch, 12. September 2018 11:27
> To: Sean Mullan ; Langer, Christoph
> ; Weijun Wang 
> Cc: security-...@openjdk.java.net; core-libs-dev@openjdk.java.net
> Subject: RE: [RFR] 8205525 : Improve exception messages during manifest
> parsing of jar archives
> 
> Hello I changed it to jar , also added  some minor adjustments suggested by
> Christoph .
> 
> http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.12/
> 
> 
> Regards, Matthias
> 
> 
> > -Original Message-
> > From: Sean Mullan 
> > Sent: Dienstag, 11. September 2018 20:44
> > To: Langer, Christoph ; Baesken, Matthias
> > ; Weijun Wang 
> > Cc: security-...@openjdk.java.net; core-libs-dev@openjdk.java.net
> > Subject: Re: [RFR] 8205525 : Improve exception messages during manifest
> > parsing of jar archives
> >
> > On 9/11/18 8:14 AM, Langer, Christoph wrote:
> > > Hi,
> > >
> > > first of all, I suggest to use "jarDetails" instead of "jarPath" as 
> > > category
> > name. Because with this contribution we add the notion of jar file plus line
> of
> > manifest to Exceptions occurring when parsing jar manifests. And if there
> > were further Exception details to be added in the area of jar files, they
> could
> > go under a category "jarDetails" as well. Would you agree? Then, in
> > Manifest.java, the field "jarPathInExceptionText" could be renamed to
> > "detailedExceptions".
> >
> > Or just "jar" would be my preference. I don't like "jarPath" as that
> > sounds too much like a file pathname to me, which we have now removed.
> >
> > --Sean

Re: RFR: 8210416: [linux] Poor StrictMath performance due to non-optimized compilation

[Magnus Ihse Bursie]

On 2018-09-11 18:14, Severin Gehwolf wrote:

Hi Erik,

Thanks for the review!

On Tue, 2018-09-11 at 08:57 -0700, Erik Joelsson wrote:

Hello Severin,

Even if using the macro, I still think you need to add a condition on
the compiler types where the switch can be reasonably expected to exist.

How about this?
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/webrev.05/

Looks good to me, too.

Thanks for keeping the spirit high despite the high number of respins of 
the patch!


/Magnus



Thanks,
Severin


On 2018-09-11 05:02, Severin Gehwolf wrote:

On Mon, 2018-09-10 at 09:29 -0700, Erik Joelsson wrote:

I see. I was not aware of that issue, but we clearly need to file a bug
for it and fix it. In this case I think it's fine to us the macro however.

OK. Update webrev, which now uses FLAGS_COMPILER_CHECK_ARGUMENTS.

http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/webrev.04/

Micro-benchmark results from running [1] for x86_64 and ppc64le are
here (-O2 is sufficient it seems):

http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/microbenchmarks_results/

More thoughts?

Thanks,
Severin

[1] https://github.com/gromero/strictmath/

Re: [12]: RFR: Use CLDR's time zone mappings for Windows

[Magnus Ihse Bursie]

On 2018-09-12 12:19, Magnus Ihse Bursie wrote:



On 2018-09-10 23:34, Naoto Sato wrote:

Hello,

Please review the fix to the following issue:

https://bugs.openjdk.java.net/browse/JDK-8209167

The proposed changeset is located at:

http://cr.openjdk.java.net/~naoto/8209167/webrev.01/


Oh, and there's a typo in CLDRConverter.java: " Note: the entries are 
alphabetically soreted, *except* the "world" region".


/Magnus

Re: [12]: RFR: Use CLDR's time zone mappings for Windows

[Magnus Ihse Bursie]

On 2018-09-10 23:34, Naoto Sato wrote:

Hello,

Please review the fix to the following issue:

https://bugs.openjdk.java.net/browse/JDK-8209167

The proposed changeset is located at:

http://cr.openjdk.java.net/~naoto/8209167/webrev.01/


Some comments:
In make/copy/Copy-java.base.gmk:
+ifneq ($(findstring $(OPENJDK_TARGET_OS), aix),)

The findstring construct is hard to read and only needed when we test 
more than one OS. Please rewrite as a single ifeq test for aix.


In GensrcCLDR.gmk:
I don't understand the CLDR_WINTZMAPPINGS file? There's no rule to 
generate it, there's just a dependency..?


The removal of the duplicate "common", that seems like a separate bug fix?

/Magnus



This fix is to remove the hand crafted map file (lib/tzmappings) on 
Windows, which maps Windows time zones to Java's tzid. It is now 
dynamically generated at the build time, using CLDR's data file 
(windowsZones.xml)


Naoto

RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives

[Baesken, Matthias]

Hello I changed it to jar , also added  some minor adjustments suggested by 
Christoph .

http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.12/


Regards, Matthias


> -Original Message-
> From: Sean Mullan 
> Sent: Dienstag, 11. September 2018 20:44
> To: Langer, Christoph ; Baesken, Matthias
> ; Weijun Wang 
> Cc: security-...@openjdk.java.net; core-libs-dev@openjdk.java.net
> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest
> parsing of jar archives
> 
> On 9/11/18 8:14 AM, Langer, Christoph wrote:
> > Hi,
> >
> > first of all, I suggest to use "jarDetails" instead of "jarPath" as category
> name. Because with this contribution we add the notion of jar file plus line 
> of
> manifest to Exceptions occurring when parsing jar manifests. And if there
> were further Exception details to be added in the area of jar files, they 
> could
> go under a category "jarDetails" as well. Would you agree? Then, in
> Manifest.java, the field "jarPathInExceptionText" could be renamed to
> "detailedExceptions".
> 
> Or just "jar" would be my preference. I don't like "jarPath" as that
> sounds too much like a file pathname to me, which we have now removed.
> 
> --Sean

Re: RFR: 8210416: [linux] Poor StrictMath performance due to non-optimized compilation

[Severin Gehwolf]

On Wed, 2018-09-12 at 17:58 +1000, David Holmes wrote:
> But I don't understand why the optimization setting is being tied to the 
> availability of the -ffp-contract flag?

In configure we perform a check for gcc or clang whether that flag is
supported. If it is, it would be non-empty exactly having -ffp-contract 
as value. It could be another set of flags for other arches if somebody
wanted to do the same, fwiw. In JDK 8, for example, it's "-mno-fused-
madd -fno-strict-aliasing" for ppc64:
http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/file/2660b127b407/make/lib/CoreLibraries.gmk#l63

We need support for that flag (or a set of flags) when we optimize
fdlibm since otherwise we would lose precision. If the flag is empty
we'd not optimize as we can't guarantee precision. That's why we tie
optimization to the availability of that flag. The expectation is for
this flag to be available on gcc/clang arches only at this point. Does
that make sense?

Thanks,
Severin

Re: RFR: 8210416: [linux] Poor StrictMath performance due to non-optimized compilation

[David Holmes]

Hi Severin,

On 12/09/2018 5:48 PM, Severin Gehwolf wrote:

Hi David,

On Wed, 2018-09-12 at 13:57 +1000, David Holmes wrote:

Hi Severin,

Sorry I'm a bit confused now.

Originally for ppc/s390x/aarch64 the optimization level for fdlibm was
HIGH. But now it will be LOW due to:

45 ifneq ($(FDLIBM_CFLAGS), )
46   BUILD_LIBFDLIBM_OPTIMIZATION := LOW

as those platforms will use gcc/clang with support for -ffp-contract and
so FDLIBM_CFLAGS will not be empty.

??


Correct. That was done based on Andrew Haley's comment here:
http://mail.openjdk.java.net/pipermail/build-dev/2018-September/023160.html

I've done some measurements with -O2 and -O3. -O2 is sufficient for a
2.75 speedup for sin/cos on ppc64le as compared to the -O0 baseline. On
the other hand the speedup of -O3 as compared to -O2 is only 1.05 for
sin/cos on ppc64le.

Since the difference between them were not huge, I've used -O2, a.k.a
LOW. See also:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/microbenchmarks_results/

To me it seems -O2 is sufficiently good. Note that HIGH == -O3 and LOW
== -O2 on gcc arches. Does that clear things up?


Yes that explains the missing -O2 - thanks.

But I don't understand why the optimization setting is being tied to the 
availability of the -ffp-contract flag?


Thanks,
David


FWIW, I'm happy to change it back to HIGH if there are concerns. See
also Gustavo's reply here for some more data points:
http://mail.openjdk.java.net/pipermail/build-dev/2018-September/023193.html

Thanks,
Severin


David

On 12/09/2018 2:14 AM, Severin Gehwolf wrote:

Hi Erik,

Thanks for the review!

On Tue, 2018-09-11 at 08:57 -0700, Erik Joelsson wrote:

Hello Severin,

Even if using the macro, I still think you need to add a condition on
the compiler types where the switch can be reasonably expected to exist.


How about this?
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/webrev.05/

Thanks,
Severin


On 2018-09-11 05:02, Severin Gehwolf wrote:

On Mon, 2018-09-10 at 09:29 -0700, Erik Joelsson wrote:

I see. I was not aware of that issue, but we clearly need to file a bug
for it and fix it. In this case I think it's fine to us the macro however.


OK. Update webrev, which now uses FLAGS_COMPILER_CHECK_ARGUMENTS.

http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/webrev.04/

Micro-benchmark results from running [1] for x86_64 and ppc64le are
here (-O2 is sufficient it seems):

http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/microbenchmarks_results/

More thoughts?

Thanks,
Severin

[1] https://github.com/gromero/strictmath/

Re: RFR: 8210416: [linux] Poor StrictMath performance due to non-optimized compilation

[Severin Gehwolf]

On Tue, 2018-09-11 at 10:30 -0700, Erik Joelsson wrote:
> Looks good, thanks!

Thanks for the review, Erik.

We've ran JCK 11 on this patch which passed on our end. I'll wait a few
more days whether there are objections and then push it.

Thanks,
Severin

> /Erik
> 
> 
> On 2018-09-11 09:14, Severin Gehwolf wrote:
> > Hi Erik,
> > 
> > Thanks for the review!
> > 
> > On Tue, 2018-09-11 at 08:57 -0700, Erik Joelsson wrote:
> > > Hello Severin,
> > > 
> > > Even if using the macro, I still think you need to add a
> > > condition on
> > > the compiler types where the switch can be reasonably expected to
> > > exist.
> > 
> > How about this?
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/webrev.05/
> > 
> > Thanks,
> > Severin
> > 
> > > On 2018-09-11 05:02, Severin Gehwolf wrote:
> > > > On Mon, 2018-09-10 at 09:29 -0700, Erik Joelsson wrote:
> > > > > I see. I was not aware of that issue, but we clearly need to
> > > > > file a bug
> > > > > for it and fix it. In this case I think it's fine to us the
> > > > > macro however.
> > > > 
> > > > OK. Update webrev, which now uses
> > > > FLAGS_COMPILER_CHECK_ARGUMENTS.
> > > > 
> > > > 
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/webrev.04/
> > > > 
> > > > Micro-benchmark results from running [1] for x86_64 and ppc64le
> > > > are
> > > > here (-O2 is sufficient it seems):
> > > > 
> > > > 
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/microbenchmarks_results/
> > > > 
> > > > More thoughts?
> > > > 
> > > > Thanks,
> > > > Severin
> > > > 
> > > > [1] https://github.com/gromero/strictmath/
> > > > 
> 
> 

Re: RFR: 8210416: [linux] Poor StrictMath performance due to non-optimized compilation

[Severin Gehwolf]

Hi David,

On Wed, 2018-09-12 at 13:57 +1000, David Holmes wrote:
> Hi Severin,
> 
> Sorry I'm a bit confused now.
> 
> Originally for ppc/s390x/aarch64 the optimization level for fdlibm was 
> HIGH. But now it will be LOW due to:
> 
>45 ifneq ($(FDLIBM_CFLAGS), )
>46   BUILD_LIBFDLIBM_OPTIMIZATION := LOW
> 
> as those platforms will use gcc/clang with support for -ffp-contract and 
> so FDLIBM_CFLAGS will not be empty.
> 
> ??

Correct. That was done based on Andrew Haley's comment here:
http://mail.openjdk.java.net/pipermail/build-dev/2018-September/023160.html

I've done some measurements with -O2 and -O3. -O2 is sufficient for a
2.75 speedup for sin/cos on ppc64le as compared to the -O0 baseline. On
the other hand the speedup of -O3 as compared to -O2 is only 1.05 for
sin/cos on ppc64le.

Since the difference between them were not huge, I've used -O2, a.k.a
LOW. See also:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/microbenchmarks_results/

To me it seems -O2 is sufficiently good. Note that HIGH == -O3 and LOW
== -O2 on gcc arches. Does that clear things up?

FWIW, I'm happy to change it back to HIGH if there are concerns. See
also Gustavo's reply here for some more data points:
http://mail.openjdk.java.net/pipermail/build-dev/2018-September/023193.html

Thanks,
Severin

> David
> 
> On 12/09/2018 2:14 AM, Severin Gehwolf wrote:
> > Hi Erik,
> > 
> > Thanks for the review!
> > 
> > On Tue, 2018-09-11 at 08:57 -0700, Erik Joelsson wrote:
> > > Hello Severin,
> > > 
> > > Even if using the macro, I still think you need to add a condition on
> > > the compiler types where the switch can be reasonably expected to exist.
> > 
> > How about this?
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/webrev.05/
> > 
> > Thanks,
> > Severin
> > 
> > > On 2018-09-11 05:02, Severin Gehwolf wrote:
> > > > On Mon, 2018-09-10 at 09:29 -0700, Erik Joelsson wrote:
> > > > > I see. I was not aware of that issue, but we clearly need to file a 
> > > > > bug
> > > > > for it and fix it. In this case I think it's fine to us the macro 
> > > > > however.
> > > > 
> > > > OK. Update webrev, which now uses FLAGS_COMPILER_CHECK_ARGUMENTS.
> > > > 
> > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/webrev.04/
> > > > 
> > > > Micro-benchmark results from running [1] for x86_64 and ppc64le are
> > > > here (-O2 is sufficient it seems):
> > > > 
> > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/microbenchmarks_results/
> > > > 
> > > > More thoughts?
> > > > 
> > > > Thanks,
> > > > Severin
> > > > 
> > > > [1] https://github.com/gromero/strictmath/
> > > > 
> > > 
> > > 

[12] RFR of JDK-8209772: Refactor shell test java/util/ServiceLoader/basic/basic.sh to java

[Amy Lu]

test/jdk/java/util/ServiceLoader/basic/basic.sh

Please review this patch to refactor above shell script test to java.

bug: https://bugs.openjdk.java.net/browse/JDK-8209772
webrev: http://cr.openjdk.java.net/~amlu/8209772/webrev.00/

Thanks,
Amy

Re: Runtime.exec : vfork() concerns and a fix proposal

[Thomas Stüfe]

Hi David,

On Tue, Sep 11, 2018 at 8:29 PM, David Lloyd  wrote:
> I think this is a cool idea.

Thanks. I think I did not come up with it though, I think the
technique was known already.

> Do you have any performance numbers?

Sure:

small program, just spawning off /bin/true a 1000 times, measured on
my t450s running Ubuntu 16.4:

Number open files:  1000   10

openjdk8:  305ms 1.5s115s
sapjvm8:   721ms  2.3s   142s

factor 2.4   1.531.23

So, it starts off with factor 2.3, but penalty diminishes with the
number of open files. This comparison is a imprecise however since we
compare different JVMs with completely different Runtime.exec()
implementations. We do more checks in our JVM, which may mean more
syscalls per fork().

..Thomas

> On Tue, Sep 11, 2018 at 12:52 PM Thomas Stüfe  wrote:
>>
>> Hi all,
>>
>> I wanted to gauge opinions on the following issue:
>>
>> Runtime.exec, on Linux, uses vfork(2) by default. It gives us better
>> performance compared with fork() and robustness in constrained memory
>> situations.
>>
>> But as we know vfork() can be dangerous if used incorrectly. In the
>> child process before exec'ing, we live in the memory of the parent
>> process. If we are not very careful we can influence or crash the
>> parent process.
>>
>> According to POSIX pretty much the only thing the child process is
>> allowed to do after vfork(2) is to exec(3) immediately; if that fails,
>> you must call _exit(2).
>>
>> http://pubs.opengroup.org/onlinepubs/009604599/functions/vfork.html
>>
>> However, in the openjdk we do a number of things beyond that:
>>
>> - stdin,out,err pipe handling business
>> - closing all file descriptors
>> - we change the working directory
>> - we may actually modify errno manually
>> - in case exec fails, we communicate the error back to the parent using pipe.
>>
>> This involves calling a number of libc functions beyond exec(), namely
>> read, close, dup2, opendir/readdir, write, chdir... It also needs a
>> bit of stack, since we assemble path names.
>>
>> --
>>
>> I was curious whether there were any real issues, so I tested (on
>> Ubuntu 16.4) and found:
>>
>> 1) A crash - any crash - in the child process before exec() will kill
>> the parent jvm dead. Weirdly enough, we do not even enter our error
>> handling, but seem to die instantly with the default "Segmentation
>> Fault".
>>
>> 2) Signals received by the child process before exec() influence the
>> parent process. For example:
>>  - SIGINT set to the child ends both parent and child, immediately
>>  - SIGABRT aborts both child and parent
>>  - any error signal sent to the child lead to the behavior described at (1)
>>
>> 3) A stack overflow in the child before exec() also kills the parent.
>> Unsurprising, since guard page hit -> segfault -> see (1).
>>
>> 4) more amusing, setting errno in the child before exec() changes the
>> errno in the parent process. propagates to the parent process.
>> But since errno is thread local and the thread in the parent process
>> is waiting in vfork() and will, upon return, not look at errno (after
>> all, vfork succeeded) this causes no trouble.
>>
>> There may be more issues, but these were the ones I tested.
>>
>> In all cases I counter-tested with fork() instead of vfork() and as
>> expected  with fork() the parent process stays unaffected as it should
>> be.
>>
>> -
>>
>> Whether you think these issues are worth solving is an open question.
>>
>> All these cases may happen in the wild (well, apart from
>> crash-by-programming-error if one assumes the program to be really bug
>> free) albeit with a very small probability. But once these bugs occur,
>> they can be very difficult to analyse. So fixing this may be
>> worthwhile.
>>
>> At SAP, we opted for robustness, so we changed the Runtime.exec()
>> implementation to deal with vfork() issues. Basically, we employ the
>> exec-twice technique:
>>
>> - in the child, after the vfork(), we immediately exec() into a little
>> bootstrap binary ("forkhelper").
>> - Now we are safe in the sense that we do not share memory with the
>> parent process anymore
>> - Then, parent process communicates with the child via pipes and gives
>> it all information needed to do the "real" exec: environ, current dir,
>> arguments... .
>> - Now the child exec's a second time, this time into the real target binary.
>>
>> The point of this technique is that we minimize the window in the
>> child between vfork and the first exec. In fact, we are now fully
>> POSIX compliant. This solves the described pathological cases.
>>
>> It has some other advantages too, e.g. allowing for better error
>> handling and tracing in the Runtime.exec() area. Performance-wise it
>> depends: we exec twice, so we pay twice. However, since the parent
>> continues execution after the first exec, it spends less time waiting
>> on the