Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux

[Florian Weimer]

On Wed, 1 Sep 2021 06:45:26 GMT, Wu Yan  wrote:

> Hi,
> Please help me review the change to enhance getting  time zone ID from 
> /etc/localtime on linux.
> 
> We use `realpath` instead of `readlink` to obtain the link name of 
> /etc/localtime, because `readlink` can only read the value of a symbolic of 
> link, not the canonicalized absolute pathname.
> 
> For example, the value of /etc/localtime is 
> "../usr/share/zoneinfo//Asia/Shanghai", then the linkbuf obtained by 
> `readlink` is "../usr/share/zoneinfo//Asia/Shanghai", and then the call of 
> `getZoneName(linkbuf)` will get "/Asia/Shanghai", not "Asia/Shanghai", which 
> consider as invalid in `ZoneInfoFile.getZoneInfo()`. Using `realpath`, you 
> can get “/usr/share/zoneinfo/Asia/Shanghai“ directly from “/etc/localtime“.
> 
> Thanks,
> wuyan

Using `realpath` instead of `readlink` will change results on systems which use 
symbolic links instead of hard links to de-duplicate the timezone files. For 
example, on Debian, if `UTC` is configured (`/etc/localtime` points to 
`/usr/share/zoneinfo/UTC`), I think the result will be `Etc/UTC` instead of 
`UTC` after this change. But I have not actually tested this.

src/java.base/unix/native/libjava/TimeZone_md.c line 292:

> 290: /* canonicalize the path */
> 291: char resolvedpath[PATH_MAX + 1];
> 292: char *path = realpath(DEFAULT_ZONEINFO_FILE, resolvedpath);

You really should use `realpath` with `NULL` as the second argument, to avoid 
any risk of buffer overflow. Future C library headers may warn about non-null 
arguments here.

-

PR: https://git.openjdk.java.net/jdk/pull/5327

Re: RFR: 6506405: Math.abs(float) is slow

[Florian Weimer]

On Thu, 8 Jul 2021 00:45:48 GMT, Joe Darcy  wrote:

> However, the bitwise conversion should now be fast everywhere.

Doesn't it require moves between general-purpose and floating-point registers? 
Those have to go through memory for some targets (including old x86, where the 
ISA supports it, but implementations were slow).

-

PR: https://git.openjdk.java.net/jdk/pull/4711

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]

[Florian Weimer]

On Fri, 12 Feb 2021 12:22:44 GMT, Vladimir Kempik  wrote:

>> src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 435:
>> 
>>> 433: //||\  Java thread created by VM does not 
>>> have glibc
>>> 434: //|glibc guard page| - guard, attached Java thread usually 
>>> has
>>> 435: //||/  1 glibc guard page.
>> 
>> Is this code going to be built by GCC (with glibc) or will only
>> macOS compilers and libraries be used?
>
> only macos comiplers

The comment is also wrong for glibc: The AArch64 ABI requires a 64 KiB guard 
region independently of page size, otherwise `-fstack-clash-protection` is not 
reliable.

-

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: Is SharedSecrets thread-safe?

[Florian Weimer]

* some-java-user:

> However, neither the static fields are `volatile` nor are the getter
> methods synchronized. So if my understanding of the Java Memory
> Model is correct, the compiler is free to reorder the two static
> field reads. So it is in theory possible that the first read yields
> a non-`null` value, but the second read yields a `null` value which
> leads to incorrect behavior (for further reading [1]).

> [1] 
> https://shipilev.net/blog/2016/close-encounters-of-jmm-kind/#wishful-benign-is-resilient

Can the JVM be multi-threaded at this point?  If not, program order
results in the desired behavior.

Re: SoftReferences and java.lang.OutOfMemoryError: Direct buffer memory

[Florian Weimer]

* David Holmes:

> More accurately soft-references will be cleared before throwing an OOME 
> due to Java heap exhaustion. There are other things that can throw OOME 
> (like your array example, or "throw new OutOfMemoryError();") that don't 
> correspond to heap exhaustion and and so soft-reference clearing doesn't 
> enter the picture.

I think it's still a bug in the spec due to the way it is worded.

Re: RFR: 8180352: Add Stream.toList() method [v2]

[Florian Weimer]

* Peter Levart:

> But I see that the new  IMM_LIST_NULLS type is needed for one other 
> thing - the immutable list implementation of that type has different 
> behavior of indexOf and lastIndexOf methods (it doesn't throw NPE when 
> null is passed to those methods) so this behavior has to be preserved in 
> the deserialized instance and there is not way to achieve that on old 
> JDK with existing serialization format, so there has to be an 
> incompatible change in the serialization format for that matter.

I think it's also needed for an efficient null element check in
List::copyOf.  I have a hunch that with the posted implementation, it
would incorrectly produce an immutable list that contains null
elements.

Re: RFR: 8180352: Add Stream.toList() method

[Florian Weimer]

On Tue, 3 Nov 2020 01:33:32 GMT, Stuart Marks  wrote:

> This change introduces a new terminal operation on Stream. This looks like a 
> convenience method for Stream.collect(Collectors.toList()) or 
> Stream.collect(Collectors.toUnmodifiableList()), but it's not. Having this 
> method directly on Stream enables it to do what can't easily by done by a 
> Collector. In particular, it allows the stream to deposit results directly 
> into a destination array (even in parallel) and have this array be wrapped in 
> an unmodifiable List without copying.
> 
> In the past we've kept most things from the Collections Framework as 
> implementations of Collector, not directly on Stream, whereas only 
> fundamental things (like toArray) appear directly on Stream. This is true of 
> most Collections, but it does seem that List is special. It can be a thin 
> wrapper around an array; it can handle generics better than arrays; and 
> unlike an array, it can be made unmodifiable (shallowly immutable); and it 
> can be value-based. See John Rose's comments in the bug report:
> 
> https://bugs.openjdk.java.net/browse/JDK-8180352?focusedCommentId=14133065=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14133065
> 
> This operation is null-tolerant, which matches the rest of Streams. This 
> isn't specified, though; a general statement about null handling in Streams 
> is probably warranted at some point.
> 
> Finally, this method is indeed quite convenient (if the caller can deal with 
> what this operation returns), as collecting into a List is the most common 
> stream terminal operation.

test/jdk/java/util/stream/test/org/openjdk/tests/java/util/stream/ToListOpTest.java
 line 47:

> 45: }
> 46: 
> 47: private void checkUnmodifiable(List list) {

I'd expect a test here that if the list contains a null element, `List::copyOf` 
throws `NullPointerException`.

-

PR: https://git.openjdk.java.net/jdk/pull/1026

Re: RFR(S): 8252407: Build failure with gcc-8+ and asan

[Florian Weimer]

* Kim Barrett:

> And strlen is not even necessarily the best solution, as it likely
> introduces an additional otherwise unnecessary string traversal. For
> example, getFlags could be changed to reject an overly long ifname,
> without using strlen, thusly:
>
> strncpy(if2.ifr_name, ifname, sizeof(if2.ifr_name));
> if (if2.ifr_name[sizeof(if2.ifr_name) - 1] != '\0') {
> return -1;
> }
>
> Unfortunately, gcc10 -Wstringop-truncation whines about this entirely
> reasonable code.

Thanks, I filed this as: 

Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: RFR(S): 8252407: Build failure with gcc-8+ and asan

[Florian Weimer]

* Kim Barrett:

>> On Sep 4, 2020, at 7:50 AM, Florian Weimer  wrote:
>> 
>> * Daniel Fuchs:
>> 
>>> Hi,
>>> 
>>> On 02/09/2020 08:19, Florian Weimer wrote:
>>>> At least one of the bugs was in theory user-visible: the network
>>>> interface code would return data for an interface that does not actually
>>>> exist on the system.
>>> 
>>> WRT NetworkInterface.c, I might support using `strnlen` to check
>>> the length before hand, if that solves both cases (gcc8 and gcc10).
>>> I'm always a bit nervous of changing the behavior in this library
>>> as it's hard to verify that no regression is introduced.
>> 
>> I think you should use strlen.  If the string is longer than the buffer
>> sent to the kernel, it cannot match an existing interface because all
>> the names are shorter.  So some sort of “not found” error needs to
>> reported.
>
> That may be, but I think doing so probably won't do anything to
> address the -Wstringop-truncation warnings.

There is no reason to use strncpy.  At least on Linux, the struct field
needs to be null-terminated, and you need to compute the length for the
length check.  So you might as well use memcpy with the length plus one
(to copy the null terminator).  You can keep using strncpy, and the
warning should be gone (because GCC will recognize a dominating strlen
check), but it's not necessary.

On current GNU/Linux, the most common structs now have the appropriate
annotations, so you get the strncpy warnings only in cases where there
is an actual truncation bug.

Thanks,
Florian

Re: RFR(S): 8252407: Build failure with gcc-8+ and asan

[Florian Weimer]

* Daniel Fuchs:

> Hi,
>
> On 02/09/2020 08:19, Florian Weimer wrote:
>> At least one of the bugs was in theory user-visible: the network
>> interface code would return data for an interface that does not actually
>> exist on the system.
>
> WRT NetworkInterface.c, I might support using `strnlen` to check
> the length before hand, if that solves both cases (gcc8 and gcc10).
> I'm always a bit nervous of changing the behavior in this library
> as it's hard to verify that no regression is introduced.

I think you should use strlen.  If the string is longer than the buffer
sent to the kernel, it cannot match an existing interface because all
the names are shorter.  So some sort of “not found” error needs to
reported.

(I assume that it's actually a bug that you can look up a network
interface by a name that merely shares the same prefix with an actual
interface on the system.)

Thanks,
Florian

Re: RFR(S): 8252407: Build failure with gcc-8+ and asan

[Florian Weimer]

* Magnus Ihse Bursie:

> Maybe we should have a common library for all native code where we
> supply our own string operation functions? It will then be much easier 
> to make sure the implementation passes different compiler versions,
> and that we provide sane semantics (which isn't really the  case with
> the original C library functions; hence all this warning churning).

When I looked at this (sorry that I never sent a patch), pretty much all
uses of strncpy were actually bugs: The code should check the actual
string length using strlen, report an error if it would be truncated,
and then use memcpy with the length already computed, plus one.

In other words, the strncpy warnings are correct, and there is only
churn in the sense that GCC gets smarter at discovering bugs.

At least one of the bugs was in theory user-visible: the network
interface code would return data for an interface that does not actually
exist on the system.

Thanks,
Florian

Re: [PATCH] optimization opportunity regarding misuse of BufferedInputStream

[Florian Weimer]

* Сергей Цыпанов:

> Hi,
>
>> Isn't this InputStream::readAllBytes?
>
> thanks for pointing this out! Indeed, InputStream::readAllBytes() allows to 
> save even more memory:
>
>   Mode  Cnt   Score Error   Units
> read  avgt   50 227.054 ±   1.354   us/op
> read:·gc.alloc.rate.norm  avgt   50  138605.638 ±  20.778B/op
> readNoVerify  avgt   50 226.606 ±   1.748   us/op
> readNoVerify:·gc.alloc.rate.norm  avgt   50  137810.392 ±   7.020B/op
>
> Would you sponsor the changes?

Sorry, I don't have that kind of role on the OpenJDK project.

Florian

Re: [PATCH] optimization opportunity regarding misuse of BufferedInputStream

[Florian Weimer]

* Сергей Цыпанов:

> @@ -105,12 +105,8 @@
>  private byte[] getBytes(InputStream is)
>  throws IOException
>  {
> -byte[] buffer = new byte[8192];
>  ByteArrayOutputStream baos = new ByteArrayOutputStream(2048);
> -int n;
> -while ((n = is.read(buffer, 0, buffer.length)) != -1) {
> -baos.write(buffer, 0, n);
> -}
> +is.transferTo(baos);
>  return baos.toByteArray();
>  }

Isn't this InputStream::readAllBytes?

Thanks,
Florian

Re: SoftCleanable and WeakCleanable

[Florian Weimer]

* Florian Weimer:

> * Alan Bateman:
>
>> On 01/08/2020 10:23, Florian Weimer wrote:
>>> Are jdk.internal.ref.SoftCleanable and jdk.internal.ref.WeakCleanable
>>> actually used?
>>>
>>> CleanerTest rests them, but I don't see any other mentions of these
>>> classes.
>
>> Do you mean used outside of the cleaner implementation? Maybe you are 
>> looking to change them to non-public?
>
> It's not clear to me how the cleaner implementation itself uses them.
> java.lang.ref.Cleaner only needs PhantomCleanableRef.

Any further comments on this topic?  Thanks.

Re: RFR[8238286]: 'Add new flatMap stream operation that is more amenable to pushing’

[Florian Weimer]

* Remi Forax:

> I still don't like the fact that IntMapMultiConsumer,
> DoubleMapMultiConsumer and LongMapMultiConsumer are not in
> java.util.function unlike all other functional interfaces used by
> the Stream API.

They seem rather too specific for java.util.function to me.  Maybe we
should add BiIntObjectConsumer etc., a BiConsumer whose first argument
is fixed as an int?  And the second consumer would be injected as a
type parameter, specific to the use in IntStream?

Re: RFR[8238286]: 'Add new flatMap stream operation that is more amenable to pushing’

[Florian Weimer]

* Patrick Concannon:

> webrev: http://cr.openjdk.java.net/~pconcannon/8238286/webrevs/webrev.05/

+ * The results of this method are undefined if the second {@link 
IntConsumer}
+ * argument is operated on outside the scope of the mapper function.

Should this say “after the mapper function has returned”?  Otherwise,
one could argue that is possible to use a consumer from a previous
invocation of the mapper function.

(And maybe add a comma after “second”?  Not a native speaker.)

Re: SoftCleanable and WeakCleanable

[Florian Weimer]

* Alan Bateman:

> On 01/08/2020 10:23, Florian Weimer wrote:
>> Are jdk.internal.ref.SoftCleanable and jdk.internal.ref.WeakCleanable
>> actually used?
>>
>> CleanerTest rests them, but I don't see any other mentions of these
>> classes.

> Do you mean used outside of the cleaner implementation? Maybe you are 
> looking to change them to non-public?

It's not clear to me how the cleaner implementation itself uses them.
java.lang.ref.Cleaner only needs PhantomCleanableRef.

SoftCleanable and WeakCleanable

[Florian Weimer]

Are jdk.internal.ref.SoftCleanable and jdk.internal.ref.WeakCleanable
actually used?

CleanerTest rests them, but I don't see any other mentions of these
classes.

Re: Type inference: bug or feature?

[Florian Weimer]

* Justin Dekeyser:

> Nevertheless, the point was not really about list stuffs, and is not
> related to the lack of co/contra variance.

> You may replace List with any other generic class Foo and Integer
> and Number with any other class satisfying the same inheritance
> relations.

Ahh, well, but why are such casts useful?  The results will be wrong in
pretty much every case.  Their correctness cannot be checked at run
time, either.

Thanks,
Florian

Re: Type inference: bug or feature?

[Florian Weimer]

* Justin Dekeyser:

> Then the following codes does not compile (for the same reason):
>
> var x = (List) emptyList(Number.class);
> List x = (List) emptyList(Number.class);
>
> incompatible types: List cannot be converted to List
>
> however, the following do compile:
>
> var x = emptyList(Number.class); // inferred to List
> List x = emptyList(Number.class); // no mistake here, it's Integer
> on the left
>
> Is this the expected behavior? Why does casting operation here interfere
> with type inference like this?

emptyList() is special, it only works this way because the returned list
is always empty, so that the element type does not matter.  For any
other list-returning function, the types List and List
are not interchangeable because for each type, there are operations
which are not valid for the other.  That's why we often need wildcards
once type hierarchies are involved (List or
List, depending on context).

A somewhat similar case is functions which do not return (normally).
They too can have a very general generic type which does not depend on
function arguments.

Thanks,
Florian

Re: Sometimes constraints are questionable

[Florian Weimer]

* Martin Buchholz:

> I wrote an earlier version of this grow logic, and then it was
> transplanted into other classes.
>
> We strongly suspect that the VM will throw OOME when we try to
> allocate an array beyond MAX_ARRAY_SIZE, so are reluctant to do so,
> but we also consider the VM behavior a bug that may eventually get
> fixed (or is already a non-issue with a different VM).  We are trying
> for good behavior with both sorts of VM.

I still don't think this explains the present code.  However, I
wouldn't be surprised if there have been early bugs where the VM would
produce a corrupt array instead of failing the allocation.  But these
bugs will have been fixed by now.

C++ has a subclass of std::bad_alloc for certain allocations that
exceed implementation limits (std::bad_array_new_length).  Since
memory is rarely tight when this happens, it allows providing better
diagnostics.

Re: os::javaTimeSystemUTC to call nanosecond precision OS API, so Clock.systemUTC() can give nanosecond precision UTC

[Florian Weimer]

* Florian Weimer:

> * Mark Kralj-Taylor:
>
>> The wording of Linux docs suggests that clock_gettime(CLOCK_REALTIME)
>> should be supported if the clock_gettime() API exists. But other clock
>> sources are not mandatory.
>
> Really old glibc emulates clock_gettime (CLOCK_REALTIME) using
> gettimeofday, yes.
>
> clock_gettime was already in Linux 2.12 (and possibly quite a bit

That should have been Linux 2.6.12, sorry.

> earlier, I did not check), so that is not likely to be a limitation.
>
> A tricky question is whether it's possible to avoid loading librt.
> The present code already uses dlopen, but I think it could try a
> little bit harder (try resolving clock_gettime first, and then load
> librt, and try again).  For distribution builds that do not need to be
> compatible with glibc versions before 2.17, directly calling
> clock_gettime would also be an option. (clock_gettime moved to
> libc.so.6 in glibc 2.17, but a lot of software keeps linking against
> librt for the definition of clock_gettime, something that we are
> finally tackling on the glibc side.)
>
> Making a direct system call instead is a bit tricky because it's
> absolutely required to use the vDSO if possible, for performance
> reasons.  But it's possible to obtain the address of the vDSO function
> using dlvsym, so that might be an option as well.

Re: os::javaTimeSystemUTC to call nanosecond precision OS API, so Clock.systemUTC() can give nanosecond precision UTC

[Florian Weimer]

* Mark Kralj-Taylor:

> The wording of Linux docs suggests that clock_gettime(CLOCK_REALTIME)
> should be supported if the clock_gettime() API exists. But other clock
> sources are not mandatory.

Really old glibc emulates clock_gettime (CLOCK_REALTIME) using
gettimeofday, yes.

clock_gettime was already in Linux 2.12 (and possibly quite a bit
earlier, I did not check), so that is not likely to be a limitation.

A tricky question is whether it's possible to avoid loading librt.
The present code already uses dlopen, but I think it could try a
little bit harder (try resolving clock_gettime first, and then load
librt, and try again).  For distribution builds that do not need to be
compatible with glibc versions before 2.17, directly calling
clock_gettime would also be an option. (clock_gettime moved to
libc.so.6 in glibc 2.17, but a lot of software keeps linking against
librt for the definition of clock_gettime, something that we are
finally tackling on the glibc side.)

Making a direct system call instead is a bit tricky because it's
absolutely required to use the vDSO if possible, for performance
reasons.  But it's possible to obtain the address of the vDSO function
using dlvsym, so that might be an option as well.

Re: [PING] RFR(s): 8176894 Provide specialized implementation for default methods putIfAbsent, computeIfAbsent, computeIfPresent, compute, merge in TreeMap

[Florian Weimer]

* Thomas Stüfe:

> Hi Tagir,
>
> nice work. Only a partwise review for TreeMap.java, did not yet look at the
> tests.
>
> remapValue:
>
>  711 } else {
>  712 // replace old mapping
>  713 t.value = newValue;
>  714 return newValue;
>  715 }
>
> Should we increase the modification count here?

It's not a structural modification, is it?  Only structural
modifications increase the modification count, as far as I can see.

Re: RFR: (T) 8241144 Javadoc is not generated for new module jdk.nio.mapmode

[Florian Weimer]

* Magnus Ihse Bursie:

> On 2020-03-24 09:59, Andrew Dinn wrote:
>> On 23/03/2020 18:38, Erik Joelsson wrote:
>>> Looks good.
>> Thanks for the review, Erik.
>>
>> I'm assuming that also implies it is trivial (because, copyright update
>> a side, it really is a 1-liner :-).
>
> For code in the build system, we do not have the Hotspot rules of 
> multiple reviewers, waiting period or trtiviality. A single OK review is 
> enough to be allowed to push it.

Where are these rules documented?  I looked for them on
openjdk.java.net, but could not find them unfortunately.

Re: 回复：VM crashed at StringTable expansion

[Florian Weimer]

* 向伟(识月):

> Hi Florian,
>
> This isn't a common usage.
> For the below code:
>
> String s1 = "s1".intern();
> f.set("s1".intern(), f.get("s2"));
>
> After calling reflection, the value of s1 is changed to "s2".
> In some special scenarios, the original jar file can't be modified. But the 
> user 
> expects to change the value of some string, and uses the above code to
> implement it.
>
> Although this usage isn't recommended, it isn't forbidden. We don't expect
> the crash because of the usage.

That's not what I see.  A modified reproducer:

import java.lang.reflect.Field;
public class StringTableTest {
public static void main(String[] args) throws Exception {
Field f = String.class.getDeclaredField("value");
f.setAccessible(true);
f.set("s1".intern(), f.get("s2"));
System.out.println("s1");
System.out.println("s2");
}
}

Prints this:

s2
s2

It also has a clear warning:

WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by StringTableTest (file:/tmp/) to field 
java.lang.String.value
WARNING: Please consider reporting this to the maintainers of StringTableTest
WARNING: Use --illegal-access=warn to enable warnings of further illegal 
reflective access operations
WARNING: All illegal access operations will be denied in a future release

I'm not sure what else we can do.

Thanks,
Florian

Re: VM crashed at StringTable expansion

[Florian Weimer]

* 向伟(识月):

> We create a simple case to reproduce the issue:
>
> import java.lang.reflect.Field;
> import java.lang.reflect.Modifier;
> public class StringTableTest {
> public static void main(String[] args) throws Exception {
> Field f = String.class.getDeclaredField("value");
> f.setAccessible(true);
> f.set("s1".intern(), f.get("s2"));
> for (int i = 0; i < 4_000_000; i++) {
> ("s_" + i).intern();
> }
> }
> }

Doesn't this change the value of the "s1" string literal to the string
"s2"?

I don't think this is supportable at all.  For example, if the
implementation happens to use "s1" anywhere, it's no longer getting the
string "s1", as expected.  There's also a non-trivial interaction with
string deduplication.

Thanks,
Florian

Re: RFR: 8239563 - Reduce public exports in dynamic libraries built from static JDK libraries

[Florian Weimer]

* Bob Vandette:

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

For ELF (at least in the GNU incarnation), a change to the installed
jni.h is not required to make the symbols hidden; a hidden definition
would be sufficient for that.  The declaration may however allow the
compiler to generate better code because it can assume a local
definition.

> All JNI entrypoints that exist in JDK static libraries are declared as
> exported or visible.  If a dynamic library is built from these static
> libraries, we end up with many exported functions that we don't want
> to provide access to,

Do you expect any impact on processes which have loaded multiple copies
of the same static libraries (same library and version) linked into
different shared objects?  Non-hidden symbols tend to make this work
transparently, even for code that does something like

   static const char *const label = ...;

   if (ptr != label)
 free (ptr);

With hidden symbols, this tends to break if ptr is passed from one
shared object to the next because all code copies are active, and their
label variables have distinct addresses.

> This is a security issue and a potential problem if this library is
> mixed with other libraries containing these symbols.

This problem still persists within shared objects themselves, of course.

Thanks,
Florian

Re: RFR: 8236641: Improve Set.of(...).iterator() warmup characteristics

[Florian Weimer]

* Claes Redestad:

> we're doing plenty of iterations over Set.of() instances during
> bootstrap, which makes these operations show up prominently in
> startup profiles. This patch refactors a few hot methods to get a
> measureable startup improvement without regressing on targeted
> microbenchmarks.
>
> Bug:https://bugs.openjdk.java.net/browse/JDK-8236641
> Webrev: http://cr.openjdk.java.net/~redestad/8236641/open.00/
>
> (The patch is baselined against 8236850)

Would it be possible to replace

  idx = SALT % elements.length;

  idx = (SALT % (table.length >> 1)) << 1;

with this?

  idx = (int) ((elements.length * (long) SALT) >>> 32);

  idx = (int) (((table.length >> 1) * (long) SALT) >>> 31);

A long multiplication followed by a shift should be faster than an int
modulo.

See this thread for some background:



Thanks,
Florian

Re: RFR 8234362: k_standard.c is not needed and should be removed

[Florian Weimer]

* Andrew Haley:

> On 11/18/19 9:49 PM, Florian Weimer wrote:
>> I think the file should just be removed.
>> 
>> Bug: <https://bugs.openjdk.java.net/browse/JDK-8234362>
>> Webrev: <http://cr.openjdk.java.net/~fweimer/8234362/webrev.01/>
>> 
>> As usual, I'll need a sponsor for this.
>
> This is OK.

Great.  Would you be able to sponsor it?

> How many patches have you done now? maybe it's time to give you commit
> access.

I count 14 commits and two Co-authored-by: lines.  However, I consider
all but two or three of them trivial, so I'm not sure if they qualify.

Thanks,
Florian

Re: Note about Manifest Header Names starting with "From"

[Florian Weimer]

* Philipp Kunz:

> Hi,
>
> The Jar File Specification [1] states that,
>
>> Note: To prevent mangling of files sent via straight e-mail,
>> no header will start with the four letters "From".
>
> But I can't see that this is actually the case.

I think the problem are the five bytes "From " at the start of a line,
not just "From".

From-Field or something like that should indeed be fine.

Re: RFR 8234362: k_standard.c is not needed and should be removed

[Florian Weimer]

* Florian Weimer:

> * Florian Weimer:
>
>> __kernel_standard in src/java.base/share/native/libfdlibm/k_standard.c
>> is built for _IEEE_LIBM targets as well, but it appears superfluous
>> there.
>>
>> In noticed this because GCC 10 flags an uninitialized variable in this
>> code:
>>
>> …/src/java.base/share/native/libfdlibm/k_standard.c: In function 
>> '__j__kernel_standard':
>> …/src/java.base/share/native/libfdlibm/k_standard.c:743:19: error: 
>> 'exc.retval' may be used uninitialized in this function 
>> [-Werror=maybe-uninitialized]
>>   743 | return exc.retval;
>>   |~~~^~~
>>
>> Rather than debating whether this warning is reasonableor not, I would
>> suggest to #ifdef out this code for _IEEE_LIBM targets (are there any
>> else left?).
>
> I think the file should just be removed.
>
> Bug: <https://bugs.openjdk.java.net/browse/JDK-8234362>
> Webrev: <http://cr.openjdk.java.net/~fweimer/8234362/webrev.01/>
>
> As usual, I'll need a sponsor for this.

Is there anything I can do to make this patch more palatable?

Thanks,
Florian

Re: New candidate JEP: 370: Foreign-Memory Access API

[Florian Weimer]

* Maurizio Cimadamore:

> While this could be done (and it was considered early during the design 
> phase), we decided against it for two reasons: first, the VarHandle API 
> is very expressive and already supports. atomic compareAndSwap  
> operations out of the box, which are _very_ handy when dealing with 
> shared memory segments. If we had to add all the methods from the 
> VarHandle API, multiplied by _each_ possible carrier type, the API 
> footprint would quickly balloon up.

I think some of the atomic operations for VarHandles are currently
specified in such a way that they are only implementable within one
JVM on certain architectures because some separate lock is needed, or
there is no suitable non-writing atomic load.

I expect that the foreign-memory access API is also intended for
process-shared mappings or read-only mappings, and supporting those
scenarios on such architectures will need kernel assists.

On the other hand, it doesn't look like any of the current OpenJDK
ports suffer from this problem.

RFR 8234362: k_standard.c is not needed and should be removed (was: Re: _IEEE_LIBM targets and __kernel_standard)

[Florian Weimer]

* Florian Weimer:

> __kernel_standard in src/java.base/share/native/libfdlibm/k_standard.c
> is built for _IEEE_LIBM targets as well, but it appears superfluous
> there.
>
> In noticed this because GCC 10 flags an uninitialized variable in this
> code:
>
> …/src/java.base/share/native/libfdlibm/k_standard.c: In function 
> '__j__kernel_standard':
> …/src/java.base/share/native/libfdlibm/k_standard.c:743:19: error: 
> 'exc.retval' may be used uninitialized in this function 
> [-Werror=maybe-uninitialized]
>   743 | return exc.retval;
>   |~~~^~~
>
> Rather than debating whether this warning is reasonableor not, I would
> suggest to #ifdef out this code for _IEEE_LIBM targets (are there any
> else left?).

I think the file should just be removed.

Bug: <https://bugs.openjdk.java.net/browse/JDK-8234362>
Webrev: <http://cr.openjdk.java.net/~fweimer/8234362/webrev.01/>

As usual, I'll need a sponsor for this.

Thanks,
Florian

_IEEE_LIBM targets and __kernel_standard

[Florian Weimer]

__kernel_standard in src/java.base/share/native/libfdlibm/k_standard.c
is built for _IEEE_LIBM targets as well, but it appears superfluous
there.

In noticed this because GCC 10 flags an uninitialized variable in this
code:

…/src/java.base/share/native/libfdlibm/k_standard.c: In function 
'__j__kernel_standard':
…/src/java.base/share/native/libfdlibm/k_standard.c:743:19: error: 'exc.retval' 
may be used uninitialized in this function [-Werror=maybe-uninitialized]
  743 | return exc.retval;
  |~~~^~~

Rather than debating whether this warning is reasonableor not, I would
suggest to #ifdef out this code for _IEEE_LIBM targets (are there any
else left?).

Thanks,
Florian

Re: RFR: CSR Core-libs support for records

[Florian Weimer]

* Vicente Romero:

> Please review the draft of the CSR for Core-libs support for records at [1]
>
> Thanks,
> Vicente
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8233436

I would have expected something regarding serialization in the
description of java.lang.Record.

Right now, it appears to be possible to have a record implement
java.io.Serializable, and serialization will appear to work, but
because the names of the implicitly generated private fields are not
specified, it is not portable across compilers and even compiler
versions.

Re: Question about String.toUpperCase behaviour

[Florian Weimer]

* Сергей Цыпанов:

> Hello,
>
> current implementation of e.g. String.toUpperCase() returns the object
> it was called on in case all code points are in upper case.
>
> E.g. this test 
>
> @Test
> public void upperCase() {
>   String str = "AZ";// English
>   assert str == str.toUpperCase();
>   
>   str = "АЯ";   // Russian
>   assert str == str.toUpperCase();
> }
>
> passes for both Latin-1 and UTF-16. This assumption allows to simplify this:
>
> boolean isUpperCase = str.toUpperCase().equals(str); 
>
> to
>
> boolean isUpperCase = str.toUpperCase() == str;
>
> Could this transformation be legal assuming that existing behaviour of
> String.toUpperCase is not documented?

Valid for whom?  For the JDK itself, sure.  But programmers really
should not assume such undocumented behavior when writing Java
programs, and neither shoud external tools targeting the JVM.

Re: RFR: JDK-8227715: GPLv2 files missing Classpath Exception

[Florian Weimer]

* Adam Farley8:

> Four GPLv2 files in 8u seem to be missing the classpath exception from the 
> copyright section.
>
> Requesting reviews and a sponsor.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8227715
>
> Webrev: http://cr.openjdk.java.net/~afarley/8227715/webrev/

All these files are not used at run time, so this could be deliberate.

Re: Formatting rules for exception messages

[Florian Weimer]

* mark reinhold:

> 2019/3/25 5:24:37 -0700, Florian Weimer :
>> Are there any guidelines for formatting exception messages?
>> 
>> In particular, I'm interested in the case when the exception message
>> is a (clipped) sentence.  Is it supposed to start with a capital
>> letter?
>> 
>> If the message refers to a parameter name, the spelling should reflect
>> the name exactly, of course.  There seems to be a slight bias towards
>> capitalization, based on a few greps.  ...
>
> The first word of any exception message in code that I’ve written, or
> reviewed, is always capitalized unless that word conveys case-sensitive
> technical information (e.g., a parameter name, as you mentioned).  This
> improves readability, especially in longer exception messages that
> contain additional punctuation characters.

Thank you for confirming my observation.  Would it make sense to have
these rules documented somewhere?

Re: RFR(s): 8228580: DnsClient TCP socket timeout

[Florian Weimer]

* Milan Mimica:

> On Thu, 5 Sep 2019 at 18:59, Florian Weimer  wrote:
>>
>> But I think in the UDP case, the client will retry.  I think the total
>> timeout in the TCP case should equal the total timeout in the UDP case.
>> That's what I'm going to implement for glibc.  The difference is that in
>> the TCP case, the TCP stack will take care of the retries, not the
>> application code.
>
> I understand that, and it does make sense, but we have to put it in
> context of how current DnsClient.java works:
> //
> // The UDP retry strategy is to try the 1st server, and then
> // each server in order. If no answer, double the timeout
> // and try each server again.
> //

Ahh.  The other option is to stick with one server and keep resending
with larger and larger timeouts.  Switching has the advantage that in
case of a server problem, you get to a working server more quickly.
Staying means that if the answer is delayed and you resend exactly the
same query, you might still pick up the answer to the original query and
process it, after the first timeout.

> Fallback to TCP happens within this process. Going immediately with
> timeout*2^maxRetry could yield significantly larger delays, if there
> happens to be some other server on the list that works better.
> I would rather look into reusing TCP connections, not to close them 
> immediately.

But we know that the server is up because it responded our UDP, so
waiting more than one second for the TCP handshake to succeed might
worthwhile, yes.

> What about read() and non-handshake TCP retransmissions? Do those
> usually happen faster?

I think so, yes.

Thanks,
Florian

Re: RFR(s): 8228580: DnsClient TCP socket timeout

[Florian Weimer]

* Milan Mimica:

> On Wed, 4 Sep 2019 at 20:32, Florian Weimer  wrote:
>>
>> If you use the initial UDP timeout (one second, I think), the kernel
>> will not complete the TCP handshake if the initial SYN segment is lost
>> because the retransmit delay during the handshake is longer than that.
>
> We could set a higher timeout value, but I would not prefer that.
> After all, 1 second is just default value, and can be changed. If the
> user wants us to give up on DNS query after the specified timeout then
> lets do just that.

But I think in the UDP case, the client will retry.  I think the total
timeout in the TCP case should equal the total timeout in the UDP case.
That's what I'm going to implement for glibc.  The difference is that in
the TCP case, the TCP stack will take care of the retries, not the
application code.

Thanks,
Florian

Re: RFR(s): 8228580: DnsClient TCP socket timeout

[Florian Weimer]

* Pavel Rappo:

>> On 4 Sep 2019, at 18:54, Florian Weimer  wrote:
>> 
>> You should use a larger timeout than the initial UDP timeout,
>> though.
>
> Could you elaborate on that?

If you use the initial UDP timeout (one second, I think), the kernel
will not complete the TCP handshake if the initial SYN segment is lost
because the retransmit delay during the handshake is longer than that.

Thanks,
Florian

Re: RFR(s): 8228580: DnsClient TCP socket timeout

[Florian Weimer]

* Pavel Rappo:

>> On 4 Sep 2019, at 18:38, Florian Weimer  wrote:
>> 
>> 
>> 
>> Maybe I'm mistaken, but I think this:
>> 
>> 692 Tcp(InetAddress server, int port, int timeout) throws IOException {
>> 693 sock = new Socket(server, port);
>> 694 sock.setTcpNoDelay(true);
>> 695 out = new java.io.BufferedOutputStream(sock.getOutputStream());
>> 696 in = new java.io.BufferedInputStream(sock.getInputStream());
>> 697 timeoutLeft = timeout;
>> 698 }
>> 
>> creates the TCP socket and connects it.  This is a potentially blocking
>> operation as well.  
>
> You are right, it definitely is a blocking operation. I missed it. I was 
> focused on
>
> 712 sock.setSoTimeout(timeoutLeft);
>
> So I'd suggest we use explicit connect with timeout
>
> java.net.Socket#connect(java.net.SocketAddress, int)
>
> Are you okay with that?

Sure.  You should use a larger timeout than the initial UDP timeout,
though.  Can you compute the maximum amount of time the UDP code would
wait for reply and use that?  Or is that the timeoutLeft value?

Thanks,
Florian

Re: RFR(s): 8228580: DnsClient TCP socket timeout

[Florian Weimer]

* Pavel Rappo:

>> On 4 Sep 2019, at 17:35, Florian Weimer  wrote:
>> 
>> * Milan Mimica:
>> 
>>> Continuing in a new thread with a RFR of my own:
>>> http://cr.openjdk.java.net/~mmimica/8228580/webrev.00/
>> 
>> I would add a comment why there's no explicit TCP connection timeout in
>> the code.  I assume you rely on the TCP stack having its own timeout,
>> right?  But I think it can be quite long under some circumstances.
>
> If I get this right, there's a default timeout of 1,000 ms (1 second)
> on L70
> src/jdk.naming.dns/share/classes/com/sun/jndi/dns/DnsContext.java
> which applies to the case where there's no explicit timeout. I agree
> though that this deserves a comment.

Maybe I'm mistaken, but I think this:

 692 Tcp(InetAddress server, int port, int timeout) throws IOException {
 693 sock = new Socket(server, port);
 694 sock.setTcpNoDelay(true);
 695 out = new java.io.BufferedOutputStream(sock.getOutputStream());
 696 in = new java.io.BufferedInputStream(sock.getInputStream());
 697 timeoutLeft = timeout;
 698 }

creates the TCP socket and connects it.  This is a potentially blocking
operation as well.  It would make sense to align the timeout for that
with what typical TCP stacks do for SYN segment retransmission.
Different TCP stacks have different connection timeouts, ranging from 70
to 130 seconds or even longer.  So the defaults are much larger than
typical DNS timeouts.  (Coincidentally, I'm working on fixing the glibc
stub resolver, and I've decided to use a non-blocking connect there
because of the long default timeouts.)

I agree on the matter of clock warp issues.  UDP needs fixing too, but
not in this change.

Thanks,
Florian

Re: RFR(s): 8228580: DnsClient TCP socket timeout

[Florian Weimer]

* Milan Mimica:

> Continuing in a new thread with a RFR of my own:
> http://cr.openjdk.java.net/~mmimica/8228580/webrev.00/

I would add a comment why there's no explicit TCP connection timeout in
the code.  I assume you rely on the TCP stack having its own timeout,
right?  But I think it can be quite long under some circumstances.

The timeout will not be enforced properly if the clock jumps backwards.

Thanks,
Florian

Re: [8u] 8226392: Launcher should not enable legacy stdio streams on GNU/Linux (glibc)

[Florian Weimer]

* Andrew John Hughes:

> On 24/06/2019 14:54, Florian Weimer wrote:
>> * Severin Gehwolf:
>> 
>>> Could I please get reviews for this 8u only change? The JDK 8u build
>>> logic for launcher files includes linker version script files (a.k.a
>>> mapfiles). The script file for x86 (32bit) marks symbol _IO_stdin_used
>>> as local. When the symbol is not visible to the dynamic loader, glibc
>>> will use a legacy stdio implementation instead. This is a historic, x86
>>> (32bit) only glibc issue, I've been told.
>> 
>> It may impact other historic GNU/Linux targets, but I don't think they
>> have OpenJDK ports.  POWER and Z are 64-bit only, I assume.
>> 
>> Thanks,
>> Florian
>> 
>
> OpenJDK is built on s390 (31-bit) and ppc (32-bit) for RHEL 7.

Oh.  I did not know this.

I looked at a recent internal s390 build, and found this:

Symbol table [ 6] '.dynsym' contains 8 entries:
 1 local symbol  String table: [ 7] '.dynstr'
  Num:Value   Size TypeBind   Vis  Ndx Name
0:   0 NOTYPE  LOCAL  DEFAULTUNDEF 
1:   0 NOTYPE  WEAK   DEFAULTUNDEF 
_ITM_deregisterTMCloneTable
2:   0 NOTYPE  WEAK   DEFAULTUNDEF __gmon_start__
3:   0 NOTYPE  WEAK   DEFAULTUNDEF _Jv_RegisterClasses
4:   0 NOTYPE  WEAK   DEFAULTUNDEF _ITM_registerTMCloneTable
5: 00400474  0 FUNCGLOBAL DEFAULTUNDEF 
__libc_start_main@GLIBC_2.0 (2)
6: 00400494  0 FUNCGLOBAL DEFAULTUNDEF 
JLI_Launch@SUNWprivate_1.1 (3)
7: 004007b8  4 OBJECT  GLOBAL DEFAULT   16 _IO_stdin_used

Same for a ppc build:

Symbol table [ 6] '.dynsym' contains 8 entries:
 1 local symbol  String table: [ 7] '.dynstr'
  Num:Value   Size TypeBind   Vis  Ndx Name
0:   0 NOTYPE  LOCAL  DEFAULTUNDEF 
1:   0 NOTYPE  WEAK   DEFAULTUNDEF 
_ITM_deregisterTMCloneTable
2:   0 NOTYPE  WEAK   DEFAULTUNDEF __gmon_start__
3:   0 FUNCGLOBAL DEFAULTUNDEF 
__libc_start_main@GLIBC_2.0 (2)
4:   0 FUNCGLOBAL DEFAULTUNDEF 
JLI_Launch@SUNWprivate_1.1 (3)
5:   0 NOTYPE  WEAK   DEFAULTUNDEF _Jv_RegisterClasses
6:   0 NOTYPE  WEAK   DEFAULTUNDEF _ITM_registerTMCloneTable
7: 17ac  4 OBJECT  GLOBAL DEFAULT   15 _IO_stdin_used

Any binary which references __libc_start_main@GLIBC_2.0 and which does
not export _IO_stdin_used in .dynsym (.symtab does not count) has this
issue, irrespective of the architecture.  The key point is support for
the glibc 2.0 ABI level (as indicated by the GLIBC_2.0 symbol version).
Architectures which joined in glibc 2.1 or later do not have this
fallback code (although the _IO_stdin_used marker symbol is sometimes
still there, quite unnecessarily).

So these two builds one does not have the issue.  The build is from May,
so it can't have Severin's fix yet
(java-1.8.0-openjdk-1.8.0.222.b04-1.el7, in case you wonder), and the
i686 build does show the issue.

Thanks,
Florian

Re: [8u] 8226392: Launcher should not enable legacy stdio streams on GNU/Linux (glibc)

[Florian Weimer]

* Severin Gehwolf:

> Could I please get reviews for this 8u only change? The JDK 8u build
> logic for launcher files includes linker version script files (a.k.a
> mapfiles). The script file for x86 (32bit) marks symbol _IO_stdin_used
> as local. When the symbol is not visible to the dynamic loader, glibc
> will use a legacy stdio implementation instead. This is a historic, x86
> (32bit) only glibc issue, I've been told.

It may impact other historic GNU/Linux targets, but I don't think they
have OpenJDK ports.  POWER and Z are 64-bit only, I assume.

Thanks,
Florian

Re: PING: RFR(s): (new approach) 8223777: In posix_spawn mode, failing to exec() jspawnhelper does not result in an error

[Florian Weimer]

* Thomas Stüfe:

> Hi Florian,
>
> Interesting, but in this case it is not posix_spawn, but plain
> fork(). The VM does this, the exec calls come from us, not the
> libc. See childproc.c .

Ah.  Others have unearthed the genealogy.  Thanks for that.

I wonder if in a Java context, things are slightly different because it
is conceivable that someone would write a shell script from Java and
then execute it directly, as a program, without setting the executable
bit.

On the glibc side, I don't think we've seen much fallout from our
removal of the shell fallback, but then we did so only for new binaries.
(Old binaries linked against an older glibc version would still perform
the shell fallback.)

Thanks,
Florian

Re: PING: RFR(s): (new approach) 8223777: In posix_spawn mode, failing to exec() jspawnhelper does not result in an error

[Florian Weimer]

* Thomas Stüfe:

> Then I ran an strace over it and saw this:
>
> 5332 [pid  3911] execve("./sleep2", ["./sleep2"], [/* 78 vars */]  ...>   
>   
> ..  
> 5342 [pid  3911] <... execve resumed> )  = -1 ENOEXEC (Exec format error)
> 5343 [pid  3911] execve("/bin/sh", ["/bin/sh", "./sleep2"], [/* 78 vars */] 
> 
> 5347 [pid  3911] <... execve resumed> )  = 0
>
> So, if the first exec fails for whatever reason, we try again, passing the 
> executable file name
> as argument to the system shell. This does not feel right? Do you know why we 
> do this? 

What's your glibc version?  Unless it's ancient, this is probably this
bug:

  

I don't know why this behavior was part of the initial implementation of
posix_spawn.  It was subsequently removed for new binaries, but came
back as a regression (the bug I referenced).

Thanks,
Florian

Re: Thread stack size issue related to glibc TLS bug

[Florian Weimer]

* Martin Buchholz:

> Very big picture - if we want to banish stack overflows forever, we would
> need to migrate the industry to split runtime stacks, which would add a bit
> of runtime overhead to every native function call.  No one is heroic enough
> to make progress towards that.  Maybe developers of new OSes need to read
> this thread?

For an infinite recursion, you would still get an OutOfMemoryError, only
much later and potentially after taking down the desktop environment.

Furthermore, if the thread stack is not committed memory, then
try-finally blocks will not work reliably because any function call may
attempt to enlarge the stack and fail to do so.

> One strong hint is that glibc uses that when it needs to create its own
> helper thread with minimal stack.
> But why-oh-why not expose that for use by others?  Other software like Java
> or Rust has the same needs!

Creating a stack with __pthread_get_minstack() bytes doesn't mean the
thread can do anything useful.  For example, the thread will not on a
kernel with a vDSO compiled with -fstack-check.  You can can't call
(f)printf on a file stream which is unbuffered.  The thread may not be
able to receive any signals whatsoever.  And so on.  That's why it's not
a public interface.

I suggested to use this interface only as a workaround for the TLS
allocation issue.

Thanks,
Florian

Re: Thread stack size issue related to glibc TLS bug

[Florian Weimer]

* Jiangli Zhou:

> Hi Florian,
>
> On Fri, May 24, 2019 at 2:46 AM Florian Weimer  wrote:
>>
>> * Jiangli Zhou:
>>
>> > [3] change: http://cr.openjdk.java.net/~jiangli/tls_size/webrev/
>> > (contributed by Jeremy Manson)
>>
>> _dl_get_tls_static_info is an internal symbol (it carries a
>> GLIBC_PRIVATE symbol version).  Its implementation can change at any
>> time.  Please do not do this.
>
> Point taken. Thanks.
>
> It appears that asan is already carrying the same type of fix:
>
> https://github.com/gcc-mirror/gcc/blob/master/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc
>
> As the issue has not been able to be addressed in the glibc layer, all
> the others have to workaround it (and possibly by using the glibc
> private APIs, _dl_get_tls_static_info or __pthread_get_minstack). That
> would cause more dependencies on the private APIs.

_dl_get_tls_static_info changed ABI on i386 in glibc 2.27 already, so
you really don't want to use that (in case someone backported that
cleanup to an earlier version, although that might be bit unlikely).

The sanitizers are special and have a much shorter shelf life than the
OpenJDK binaries.

> One alternative (besides fixing glibc) may be making
> _dl_get_tls_static_info public. Would that be possible?

For __pthread_get_minstack, I can add a comment to the glibc sources
that if the ABI changes (or if TLS allocations are no longer considered
part of the stack), we should rename the function.  Then, as long as you
use a weak reference or dlsym (the latter is preferable for the sack of
RPM-based distributions which require special steps to reference
GLIBC_PRIVATE symbols directly), old binaries would keep working if we
make further changes.

Since _dl_get_tls_static_info already changed ABI once, I really can't
recommend its use.  Especially if you can work with
__pthread_get_minstack instead.

The value of PTHREAD_STACK_MIN is rather problematic on x86-64 (for the
reasons I explained earlier), but it's not likely we are going to change
the value of the constant any time soon.  It's more likely that we are
going to work around the AVX-512 register file issue by providing
exactly four usable stack pages with PTHREAD_STACK_MIN, and not less
than two as we did until recently.  So you can assume that it's indeed
possible to use PTHREAD_STACK_MIN and sysconf (_SC_PAGESIZE) to compute
the static TLS area size.

Thanks,
Florian

Re: Thread stack size issue related to glibc TLS bug

[Florian Weimer]

* Jiangli Zhou:

> Hi Florian,
>
> Thanks for the feedback!
>
> On Fri, May 24, 2019 at 3:13 AM Florian Weimer  wrote:
>>
>> * David Holmes:
>>
>> > My thoughts haven't really changed since 2015 - and sadly neither has
>> > there been any change in glibc in that time. Nor, to my recollection,
>> > have there been any other reported issues with this.
>>
>> The issue gets occasionally reported by people who use small stacks with
>> large initial-exec TLS consumers (such as jemalloc).  On the glibc side,
>> we aren't entirely sure what to do about this.  We have recently tweaked
>> the stack size computation, so that in many cases, threads now receive
>> an additional page.  This was necessary to work around a kernel/hardware
>> change where context switches started to push substantially more data on
>> the stack than before, and minimal stack sizes did not work anymore on
>> x86-64 (leading to ntpd crashing during startup, among other things).
>>
>> The main concern is that for workloads with carefully tuned stack sizes,
>> revamping the stack size computation so that TLS is no longer
>> effectively allocated on the stack might result in address space
>> exhaustion.  (This should only be a concern on 32-bit architectures.)
>
> Could it to be addressed for 64-bit (first) at foreseeable future?

Yes, I'd happily review a patch if we had one.

>> Even if we changed this today (or had changed it in 2015), it would take
>> a long time for the change to end up with end users, so it's unclear how
>> much help it would be.
>>
>> Maybe OpenJDK can add a property specifying a stack size reserve, and
>> htis number is added to all stack size requests?  This will at least
>> allow users to work around the issue locally.
>
> One issue is that user may not know the property should be used and
> what would be the proper reserved size when run into the stack size
> issue related to TLS. The stack size issue could be hard for average
> users to diagnose.

You could print the requested stack size, stack size reserve, and actual
stack size in the StackOverflowError message.  The fact that there is a
reserve (which would be zero by default) could serve as a hint and
quickly lead to the discovery of the tunable.  At least I hope that.

Thanks,
Florian

Re: Thread stack size issue related to glibc TLS bug

[Florian Weimer]

* David Holmes:

> My thoughts haven't really changed since 2015 - and sadly neither has
> there been any change in glibc in that time. Nor, to my recollection,
> have there been any other reported issues with this.

The issue gets occasionally reported by people who use small stacks with
large initial-exec TLS consumers (such as jemalloc).  On the glibc side,
we aren't entirely sure what to do about this.  We have recently tweaked
the stack size computation, so that in many cases, threads now receive
an additional page.  This was necessary to work around a kernel/hardware
change where context switches started to push substantially more data on
the stack than before, and minimal stack sizes did not work anymore on
x86-64 (leading to ntpd crashing during startup, among other things).

The main concern is that for workloads with carefully tuned stack sizes,
revamping the stack size computation so that TLS is no longer
effectively allocated on the stack might result in address space
exhaustion.  (This should only be a concern on 32-bit architectures.)

Even if we changed this today (or had changed it in 2015), it would take
a long time for the change to end up with end users, so it's unclear how
much help it would be.

Maybe OpenJDK can add a property specifying a stack size reserve, and
htis number is added to all stack size requests?  This will at least
allow users to work around the issue locally.

If we change the accounting in glibc, we will have to add a similar
tunable on the glibc side, too.

Thanks,
Florian

Re: Thread stack size issue related to glibc TLS bug

[Florian Weimer]

* Jiangli Zhou:

> [3] change: http://cr.openjdk.java.net/~jiangli/tls_size/webrev/
> (contributed by Jeremy Manson)

_dl_get_tls_static_info is an internal symbol (it carries a
GLIBC_PRIVATE symbol version).  Its implementation can change at any
time.  Please do not do this.

Thanks,
Florian

Re: RFR - JDK-8203444 String::formatted (Preview)

[Florian Weimer]

* Jim Laskey:

> Good point. To make sure I fully understand what you are stating,
>
> - The argument for having an instance method is reasonable to achieve
> "flowiness".

Right.

> - However, only one version is necessary or desired, that is, "public
> String formatted(Object... args)".

Yes, this appears to be the common use case to me.  I doubt the overload
with its ambiguity is sufficiently widely used to be worth the potential
confusion it can create.  Adding the method with the Locale parameter
under a different name would be okay (except that I'm not sure how
widely it is going to be used).

My totally random set of Java classes (mostly from Fedora and
downstream) shows this:


SELECT jr.descriptor, COUNT(*)  descriptor
  FROM symboldb.java_reference jr
  WHERE jr.class_name = 'java/lang/String' AND jr.name = 'format'
  GROUP BY jr.descriptor;
  
 descriptor  | 
descriptor 
-+
 (Ljava/lang/String;[Ljava/lang/Object;)Ljava/lang/String;   |  
 9220
 (Ljava/util/Locale;Ljava/lang/String;[Ljava/lang/Object;)Ljava/lang/String; |  
  436
(2 rows)

So for the existing String::format method, the version without the
explicit Locale argument is much more commonly used.

> - In cases where Locale needs to be overridden, then either use
> "String.format(Locale lc, String fmt, Object... args)" or globally set
> "Locale.setDefault(Locale lc)".

I wouldn't recommend changing global state in this way.

Thanks,
Florian

Re: RFR - JDK-8203444 String::formatted (Preview)

[Florian Weimer]

* Jim Laskey:

> All discussion is valid. I agree the ambiguity is tricky, but can be
> resolved by using explicit locale.
>
> Example:
>
>   "%s".formatted(Locale.getDefault(), Locale.JAPAN);
>
> This guarantees the "public String formatted(Locale l,
> Object... args)" form is chosen with the second Locale as an argument.

There is also the cognitive overhead.  I think the key question is
whether this is so bad:

  String.format(Locale.US, """
  %s
  """, Locale.JAPAN);

Then perhaps we wouldn't need the formatted method which takes a Locale
object.

Thanks,
Florian

Re: RFR - JDK-8203444 String::formatted (Preview)

[Florian Weimer]

* Jim Laskey:

> Please do a code review of the new String::formatted instance method. This 
> method was originally introduced under the name "format"
> in conjunction with Raw String Literals. The method is being reintroduced in 
> conjunction with Text Blocks and renamed to avoid method
> resolution conflicts against the static method String::format.
>
> Example of use:
>
> String name = "Smith, Pat";
> String address = "123 Maple St., Anytown, Anywhere";
> String phone = "999-555-1234";
> String client = """
> Name: %s
> Address: %s
> Phone: %s
> """.formatted(name, address, phone); 

I'm a bit concerned by the ambiguity between the version with and
without the Locale argument.  Not sure if this is the kind of feedback
you are looking for.

(String::format does not have this ambiguity and had been able to avoid
it easily, so it's not a good guide here.)

Thanks,
Florian

Re: RFR(s): (new approach) 8223777: In posix_spawn mode, failing to exec() jspawnhelper does not result in an error

[Florian Weimer]

* Thomas Stüfe:

> Hi all,
>
> (old mail thread: 
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-May/060139.html)
>
> May I please have your reviews and opinions for the following bug fix:
>
> issue: https://bugs.openjdk.java.net/browse/JDK-8223777
> cr: 
> http://cr.openjdk.java.net/~stuefe/webrevs/8223777-posix_spawn-no-exec-error-alternate-impl/webrev.00/webrev/
>
> ---

I wonder if you should use 0 as the value for CHILD_IS_ALIVE.  All
integers could be valid errno values.  But if you check for errors from
the restartableWrite call (and call _exit), I don't think there is a
risk of confusion, so the value does not have to be distinct.

Thanks,
Florian

Re: RFR(s): 8223777: In posix_spawn mode, failing to exec() jspawnhelper does not result in an error

[Florian Weimer]

* Thomas Stüfe:

> Since we deal with many libc variants, not only glibc, fixing
> posix_spawn just in glibc may not be sufficient, at least not for a
> long time.

I think Solaris already has the required functionality, so it's not
entirely glibc-specific.

> But if you would fix glibc and give it a error-reporting back channel
> for exec() this would already help a lot. I dug up
> https://sourceware.org/bugzilla/show_bug.cgi?id=18433 which sounded
> pretty hopeless.

Well, this one at least is fixed.  But it will take some time to
propagate to distributions, of course.  If an issue really bugs you, it
might help to open backport requests with distribution maintainers.

> I have long given up trying to report bugs to glibc maintainers :(

Sorry to hear that.  I think there's value in actually fixing the buggy
component, and not working around breakage elsewhere.

Thanks,
Florian

Re: RFR(s): 8223777: In posix_spawn mode, failing to exec() jspawnhelper does not result in an error

[Florian Weimer]

* Thomas Stüfe:

> Right now I am worried more about things I cannot determine yet. Where
> before we would wait for the pipe to get broken, now we have a read
> call on the parent side, a write call on the child side, which both
> must succeed. Could they fail sporadically, e.g. due to EINTR? I know
> this sounds very vague but around this API I am super careful.

EINTR should only arrive if there's a signal handler, otherwise the
signal is either ignored or terminates the process.  I don't think
jspawnhelper installs any.  If the write fails, jspawnhelper can just
exit, and it will look like as if it had never launched (resulting in an
error).  The write-after-exec-error case is more problematic than that.

I'm working on this from the other end—adding functionality to glibc, so
that we can eliminate jspawnhelper.  But that's a more long-term effort,
of course.

Thanks,
Florian

Re: RFR(s): 8223777: In posix_spawn mode, failing to exec() jspawnhelper does not result in an error

[Florian Weimer]

* David Lloyd:

> Pipe communication isn't very costly AFAIK.  The added complexity
> would be waiting for either a thing to appear on the pipe indicating
> success or else a waitpid/waitid+WNOHANG for exit code 127.  But
> writing a thing to the pipe won't help determine if the subsequent
> exec succeeded, which is really the interesting bit.

Please have a look at the code.  It's already using CLOEXEC to cover
the execve call (src/java.base/unix/native/libjava/ProcessImpl_md.c):

switch (readFully(fail[0], , sizeof(errnum))) {
case 0: break; /* Exec succeeded */
case sizeof(errnum):
waitpid(resultPid, NULL, 0);
throwIOException(env, errnum, "Exec failed");
goto Catch;
default:
throwIOException(env, errno, "Read failed");
goto Catch;
}

Instead of 0/4 bytes, the outcome could be 0/4/8 bytes, corresponding to
jspawnhelper exec failure, complete success, and exec failure in
jspawnhelper.

The run-time cost would be the additional pipe write in jspawnhelper.
There shouldn't even be another ping-pong between the processes.

Thanks,
Florian

Re: RFR(s): 8223777: In posix_spawn mode, failing to exec() jspawnhelper does not result in an error

[Florian Weimer]

* Thomas Stüfe:

> This is impossible to fix completely - see Martin's comment about the
> impossibility to foresee whether an exec() will succeed without actually
> exec()ing. But at least we can test the execute permissions on the
> jspawnhelper. Which this fix does. This fixes Ubuntu 16.4 (Now, I get an
> IOException if jspawnhelper is not executable).

I think jspawnhelper could write something to the pipe to show that it
has started.  If you never get that notification, you know that
jspawnhelper hasn't run, even if posix_spawn has succeeded.

(This fix will also help qemu-user and WSL, which implement vfork as
fork and interfere with the internal error reporting from posix_spawn.)

Thanks,
Florian

Formatting rules for exception messages

[Florian Weimer]

Are there any guidelines for formatting exception messages?

In particular, I'm interested in the case when the exception message
is a (clipped) sentence.  Is it supposed to start with a capital
letter?

If the message refers to a parameter name, the spelling should reflect
the name exactly, of course.  There seems to be a slight bias towards
capitalization, based on a few greps.  Consider this:

$ LC_ALL=C grep -r 'Exception("[A-Z][^" ]*ing '  src/*/share/classes/ | wc -l
159
$ LC_ALL=C grep -r 'Exception("[a-z][^" ]*ing '  src/*/share/classes/ | wc -l
73

Re: java.lang.CharSequence#compare - lexicographical ordering of custom Comparable CharSequence(s)

[Florian Weimer]

* Jason Mehrens:

> The initial implementation was only optimized to call into
> String.compare if the arguments were string [1].  I proposed the
> current code a general form to catch java.nio.CharBuffer and any new
> JDK implementations of CharSequence + Comparable.
>
> Naively, I lean towards "- CharSequence interface specification
> should be extended to require Comparable CharSeqeunces to implement
> lexicographical ordering".

There are natural lexicographical orderings, unfortunately.  One is
according to UCS-2 (currently implemented by String and specified by
CharSequence.compare), and the other one according UTF-16, for
compatibility with the lexicographical ordering according to Unicode
codepoints (which is also compatible with lexicographical ordering of
bytes after UTF-8 encoding).

There must be implementations of CharSequence out there which
implement UTF-16 ordering.

Re: JDK 13 RFR of JDK-8217000: Refactor Class::methodToString

[Florian Weimer]

* Joe Darcy:

> - sb.append(Stream.of(typeparms).map(Class::typeVarBounds).
> -  collect(Collectors.joining(",", "<", ">")));
> +    sb.append(Arrays.stream(typeparms)
> +  .map(Class::typeVarBounds)
> +  .collect(Collectors.joining(",", "<", ">")));
>  }

I realize that this is a pre-existing issue, but isn't this approach a
bit awkward?  Creating a temporary string just to put it into a
StringBuilder?

Thanks,
Florian

Re: OpenJDK fails to build with GCC when the #include inside zip.cpp comes from a non-sysroot path

[Florian Weimer]

* David Holmes:

> This should really be being discussed on core-libs-dev.

Okay, moving the conversation.

>> diff -r 70a423caee44 src/share/native/com/sun/java/util/jar/pack/zip.cpp
>> --- a/src/share/native/com/sun/java/util/jar/pack/zip.cppTue Oct 09 
>> 08:33:33 2018 +0100
>> +++ b/src/share/native/com/sun/java/util/jar/pack/zip.cppWed Nov 28 
>> 22:13:12 2018 -0500
>> @@ -415,9 +415,7 @@
>>   ((uLong)h << 11) | ((uLong)m << 5) | ((uLong)s >> 1);
>>   }
>>   -#ifdef _REENTRANT // solaris
>> -extern "C" struct tm *gmtime_r(const time_t *, struct tm *);
>> -#else
>> +#if !defined(_REENTRANT) // linux
>>   #define gmtime_r(t, s) gmtime(t)
>>   #endif
>>   /*
>
> Under the theme "two wrongs don't make a right" the use of _REENTRANT
> here is totally inappropriate AFAICS. It seems to be a misguided
> attempt at determining whether we need the thread-safe gmtime_r or not
> - and the answer to that should always be yes IMHO.
>
> We define _REENTRANT for:
> - linux
> - gcc
> - xlc
>
> So the original code will define:
>
> extern "C" struct tm *gmtime_r(const time_t *, struct tm *);
>
> for linux (and AIX with xlc?) but not Solaris, OS X or Windows.
>
> But Solaris has gmtime_r anyway. So the existing code seems a really
> convoluted hack. AFAICS we have gmtime_r everywhere but Windows (where
> gmtime is already thread-safe). So it seems to me that all we should
> need here is:
>
> -#ifdef _REENTRANT // solaris
> -extern "C" struct tm *gmtime_r(const time_t *, struct tm *);
> -#else
> +#ifdef _MSC_VER // Windows
>  #define gmtime_r(t, s) gmtime(t)
>  #endif

That looks much cleaner.

Thanks,
Florian

Re: RFR(S)JDK-8214074: Ghash optimization using AVX instructions

[Florian Weimer]

* Smita Kamath:

> I'd like to contribute an optimization for GHASH Algorithm using AVX
> Instructions. I have tested this optimization on SKX x86_64 platform
> and it shows ~20-30% performance improvement for larger message sizes
> (for example 8k).

Performance improvement against what?  The pure Java implementation?  I
find this a bit surprising.  I would have expected a much larger
performance improvement from the use of CLMUL instructions.

Thanks,
Florian

Re: Time-zone database issues

[Florian Weimer]

* Stephen Colebourne:

> On Tue, 23 Oct 2018 at 09:44, Florian Weimer  wrote:
>> * Stephen Colebourne:
>>
>> > No, the day-of-month and day-of-week would remain the same, as the
>> > time is relative to those dates.
>>
>> My expectation is that the values returned by the other methods would
>> change for a getLocalTime() that provides a normalized return value
>> because the transition rule in normalized time is different (i.e., last
>> Saturday in a month turns into something else).
>
> As mentioned in the original mail, it is not always possible to
> normalize the values back to sane values. Trying to do so would result
> in a worse outcome than the proposed change (more deprecations and
> more chance of users hitting compatibility problems).
>
> Current response (until tzdb made their recent change):
> - getDayOfWeek() = Sun
> - getLocalTime() = 01:00
>
> Proposed response:
> - getDayOfWeek() = Sat
> - getLocalTime() = 01:00 (deprecated)
> - getLocalTimeDuration() = 25:00

Hmm.  I'm no longer sure if normalization is always possible.  If the
rule says, “last Saturady in the month at 25:00”, there is no normalized
equivalent because every normalized rule can only express dates that
always fall into the same month, as far as I can see.

If this is correct, I think offering normalized and non-normalized
interfaces is futile.

Does it even make sense to expose this level of detail?  Many transition
rules cannot be expressed in terms of the Gregorian calendar anyway.

Thanks,
Florian

Re: 6850720: Allow POSIX_SPAWN to be used for ProcessImpl on Linux

[Florian Weimer]

* Thomas Stüfe:

>> The main problem for vfork in application code is that you need to *all*
>> disable signals, even signals used by the implementation.  If a signal
>> handler runs by accident while the vfork is active, memory corruption is
>> practically guaranteed.  The only way to disable the signals is with a
>> direct system call; sigprocmask/pthread_sigmask do not work.
>>
>> Does your implementation do this?
>
> I understand. No, admittedly not. But we squeeze the vulnerable time
> window to the minimal possible:
>
> if (vfork() == 0) exec(..);
>
> which was a large step forward from the stock Ojdk solution.
>
> While not completely bullet proof, I saw not a single instance of an
> error in all these years (I understand those errors would be very
> intermittent and difficult to attribute to vfork+signalling, so we may
> have missed some).

Well, such tight races never matter until they suddenly do.  I'm always
amazed how races previously thought impossible reproduce reliably with
high probability, given the right circumstances.  The highlight so far
was a single-instruction race (not even a data race) in our condition
variable implementation, which broke our iSCSI implementation.

>> > The current posix_spawn() implementation was added to glibc with glibc
>> > 2.24. So, what was the state of posix_spawn() before that version? Is
>> > it safe to use, does it do the right thing, or will we encounter
>> > regressions?
>>
>> It uses fork by default.  It can be told to use vfork, via
>> POSIX_SPAWN_USEVFORK, but then it is buggy.  For generic JDK code, this
>> seems hardly appropriate.
>
> Are you sure about this? The coding I saw in  glibc < 2.24 was that it
> would use vfork if both attributes and file actions were NULL, which
> should be the case with the OpenJDK and jspawnhelper.

Oh, right.  All the more reason to backport the glibc 2.24 change. 8-)

Thanks,
Florian

Re: Time-zone database issues

[Florian Weimer]

* Stephen Colebourne:

> No, the day-of-month and day-of-week would remain the same, as the
> time is relative to those dates.

My expectation is that the values returned by the other methods would
change for a getLocalTime() that provides a normalized return value
because the transition rule in normalized time is different (i.e., last
Saturday in a month turns into something else).

Thanks,
Florian

Re: Time-zone database issues

[Florian Weimer]

* Stephen Colebourne:

> Fixing the parser to handle values like 25:00 would be relatively
> easy. However, these rules are also exposed in the public API of
> java.time.zone.ZoneOffsetTransitionRule [3]. Currently this class has
> methods `getLocalTime()` and `isMidnightEndOfDay()`. These would need
> to be deprecated and replaced by a new method `getLocalTimeDuration()`
> (or some other name) that returns an instance of `Duration`.

I think getLocalTimeDuration() would have to be paired with new methods
getDayOfMonthIndicator(), getDayOfWeek().  The values returned by these
methods will have to change between normalized and non-normalized
transition times.

The branch appears to be here:

  

Thanks,
Florian

Re: 6850720: Allow POSIX_SPAWN to be used for ProcessImpl on Linux

[Florian Weimer]

* Thomas Stüfe:

> So far I have not read a single technical reason in this thread why
> vfork needs to be abandoned now - apart from it being obsolete. If you
> read my initial thread from September, you know that I think we have
> understood vfork's shortcomings very well, and that our (SAPs)
> proposed patch shows that they can be dealt with. In our port, our
> vfork+exec*2 is solid since many years, without any issues.

The main problem for vfork in application code is that you need to *all*
disable signals, even signals used by the implementation.  If a signal
handler runs by accident while the vfork is active, memory corruption is
practically guaranteed.  The only way to disable the signals is with a
direct system call; sigprocmask/pthread_sigmask do not work.

Does your implementation do this?

> The current posix_spawn() implementation was added to glibc with glibc
> 2.24. So, what was the state of posix_spawn() before that version? Is
> it safe to use, does it do the right thing, or will we encounter
> regressions?

It uses fork by default.  It can be told to use vfork, via
POSIX_SPAWN_USEVFORK, but then it is buggy.  For generic JDK code, this
seems hardly appropriate.

> My Ubuntu 16.04 box runs glibc 2.23. Arguably, Ubuntu 16.04 is quite a
> common distro. I have to check our machines at work, but I am very
> sure that our zoo of SLES and RHEL servers do not all run glibc>=2.24,
> especially on the more exotic architectures.

In glibc, the vfork-based performance does not bring in any new ABIs, so
it is in theory backportable.  The main risk is that the vfork
optimization landed in glibc 2.24, and the PID cache was removed in
glibc 2.25.  vfork with the PID cache was really iffy, but I would not
recommend to backport the PID cache removal.  But Debian 9/stretch uses
glibc 2.24, and I think that shows that the vfork optimization with the
PID cache should be safe enough.  (Of course you need to remove the
assert that fires if the vfork does not actually stop the parent process
and is implemented as a fork; the glibc implementation still works, but
with somewhat degraded error checking.)

How far back would you want to see this changed?  Debian jessie and Red
Hat Enterprise Linux 6 would be rather unlikely.  If you want to target
those, your only chance is to essentially duplicate the glibc
implementation in OpenJDK.

Thanks,
Florian

Re: 6850720: Allow POSIX_SPAWN to be used for ProcessImpl on Linux

[Florian Weimer]

* David Lloyd:

> Sure, but I don't really see this as necessary if glibc is already
> following the vfork-like path.  Another thing to know is that at least
> in the 2.26 sources I have checked out, the POSIX_SPAWN_USEVFORK flag
> is completely ignored.  See also [2].

Right, the manual pages are outdated.

For applications, there is never a good reason to use
POSIX_SPAWN_USEVFORK because the glibc implementation is either buggy
when this flag is used, or the flag does nothing.  The bugs may matter
to applications using OpenJDK, so I don't think you can set the flag
within OpenJDK.  So the only thing you can do here is to use posix_spawn
*without* POSIX_SPAWN_USEVFORK, and advise OS vendors to backport the
glibc changes if they have customers that are impacted by the lack of
process creation performance (or OOM during process creation).

Another possibility would be to emulate what glibc's fixed, fork-based
posix_spawn does, but this requires writing some machine code (for
vfork/clone) and issuing direct system calls to bypass some abstractions
in glibc (for setprocmask).

> [2] 
> https://github.com/bminor/glibc/commit/ccfb2964726512f6669fea99a43afa714e2e6a80

Note that this neither the canonical glibc source code location, nor
is the code actually used on Linux. 8-)

Thanks,
Florian

Re: Adding new IBM extended charsets

[Florian Weimer]

On 07/06/2018 02:23 PM, Nasser Ebrahim wrote:

Hi Florian,

Thank you for your response. iconv is platform dependent and not good for
the platform agnostic nature of Java. Also, many charsets in Java are not
available across platforms. I believe Java decided to have its own
charsets due to those reasons so that it can work seamlessly on any
supported platforms.


But then the tables will occasionally be different from what the 
platform uses for conversation.  Wouldn't be compatibility with the rest 
of the system installation be preferred in this context?


Thanks,
Florian

Re: RFR: 8207851 JEP Draft: Support ByteBuffer mapped over non-volatile memory

[Florian Weimer]

* Andrew Dinn:

>> +// TODO - remove the following defines
>> +// for testing we need to define the extra MAP_XXX flags used for
>> +// persistent memory. they will eventually get defined by the
>> +// system and included via sys/mman.h
>> 
>> Do you really want to change implementation behavior based on which
>> glibc headers (or kernel headers) were used to build the JDK?  It's
>> likely more appropriate to define the constants yourself if they are
>> not available.  There is some architecture variance for the MAP_*
>> constants, but in case of MAP_SHARED_VALIDATE, even HPPA has the same
>> definition.
>
> No, I guess I don't really want to change implementation behaviour based
> on which glibc headers (or kernel headers) were used to build the JDK.
> So, I think I need to modify this so the constants are always defined by
> FileChannelImpl.c
>
> I have followed the Intel libpmem code in defining the values for these
> constants. Can you provide details of the 'architecture variance' you
> refer to above?

Just a quick note on that: Some of the constants in  vary
between the Linux architectures for historic reasons.  However, for
MAP_SHARED_VALIDATE, the constant is consistent across all
architectures supported by glibc and in the mainline kernel.

You could add something like this to be on the safe side:

#ifdef MAP_SHARED_VALIDATE
#if MAP_SHARED_VALIDATE != 3
#error Unexpected value for MAP_SHARED_VALIDATE
#endif
#else /* !MAP_SHARED_VALIDATE */
#define MAP_SHARED_VALIDATE 3
#endif

But I doubt that this is required for this constant.

Re: Long chains created by direct Buffer::slice

[Florian Weimer]

* Paul Sandoz:

> I created this issue:
>
>   https://bugs.openjdk.java.net/browse/JDK-8208362

Thanks.

> The suggested fix requires a tweak though since an instance of a
> DirectBuffer interface is passed. This is required because views
> over direct ByteBuffers can be created.

Noticed that as well.

> diff -r 448cd909c9e2 
> src/java.base/share/classes/java/nio/Direct-X-Buffer.java.template
> --- a/src/java.base/share/classes/java/nio/Direct-X-Buffer.java.template  
> Thu Jul 26 11:53:59 2018 -0700
> +++ b/src/java.base/share/classes/java/nio/Direct-X-Buffer.java.template  
> Thu Jul 26 16:46:20 2018 -0700
> @@ -194,7 +194,8 @@
>  #if[byte]
>  cleaner = null;
>  #end[byte]
> -att = db;
> +Object attachment = db.attachment();
> +att = (attachment == null ? db : attachment);

This is essentially what I put through jtreg (jdk_core), without any
obvious issues, but I had not time yet to file a bug and create a
webrev.

The parenthesis seem unnecessary.  If this is an official JDK coding
style, it is not widely used.

Re: RFR: 8207851 JEP Draft: Support ByteBuffer mapped over non-volatile memory

[Florian Weimer]

* Andrew Dinn:

> Comments and suggestions for improvement would be very welcome.

There are a bunch of typos in the JEP (“susbet”, “Rntime”).
Formatting of the headlines looks wrong to me, too.

On the implementation side, I find this a bit concerning:

+// TODO - remove the following defines
+// for testing we need to define the extra MAP_XXX flags used for
+// persistent memory. they will eventually get defined by the
+// system and included via sys/mman.h

Do you really want to change implementation behavior based on which
glibc headers (or kernel headers) were used to build the JDK?  It's
likely more appropriate to define the constants yourself if they are
not available.  There is some architecture variance for the MAP_*
constants, but in case of MAP_SHARED_VALIDATE, even HPPA has the same
definition.

How does the MappedByteBuffer::force(long, long) method interact with
the memory model?

Re: Adding new IBM extended charsets

[Florian Weimer]

On 07/04/2018 02:41 PM, Nasser Ebrahim wrote:

Please share your thoughts on your preferred option and list out any other
options which I missed out. Thank you for your time.


Could you use the platform iconv implementation instead?  That would 
avoid shipping the tables in the JDK.


Thanks,
Florian

Re: Promptly freeing the per-thread cached direct buffers when a thread exits

[Florian Weimer]

* Tony Printezis:

> There are a few obvious ways to mitigate this, e.g., cause a Full GC /
> concurrent GC cycle at regular intervals. However, the best solution IMHO
> is to explicitly free any direct buffers that are still in the cache when a
> thread exits.

Why is this safe?  Couldn't these direct byte buffers be used with a
custom channel that leaks them to another thread?

Re: The store for byte strings

[Florian Weimer]

* John Rose:

> In https://bugs.openjdk.java.net/browse/JDK-8161256 I discuss
> this nascent API under the name "ByteSequence", which is analogous
> to CharSequence, but doesn't mention the types 'char' or 'String'.

Very interesting.

What's the specification for toString() and hashCode()?

One problem of retrofitting a custom ByteString into a CharSequence is
that CharSequence reuses toString() in a fairly central fashion, and
it's hard to reconcile this with byte-based length() and charAt()
methods unless ISO-8859-1 encoding is used.

If this feature is supposed to land before JEP 218?  If not, how does
ByteSequence differ from List?

> If the ByteSequence views are value instances, they can be created
> at a very high rate with little or no GC impact.  Generic algorithms
> would still operate on them 

I'm not up-to-date with those upcoming changes.  Would the nature as
value instances be expressed as part of the ByteSequence interface
type?

> If the API is properly defined it can be inserted directly into
> existing types like ByteBuffer.  Doing this will probably require us
> to polish ByteBuffer a little, adding immutability as an option and
> lifting the 32-bit limits.  It should be possible to "freeze" a
> ByteBuffer or array and use it as a backing store that is reliably
> immutable, so it can be handed to zero-copy algorithms that work
> with ByteSequences.

Such freezing is incompatible with mapped byte buffers, right?  Even
if the implementation prevents changes at the VM/process level,
changes on the file system could well become visible.  Do you expect
to make freezing an optional operation (probably not a good idea), or
copy the contents of the mapping to the heap (which is probably not
too bad, considering that a shared byte[] could also result in
arbitrarily large copies).

> Independently, I want to eventually add frozen arrays, including
> frozen byte[] arrays, to the JVM, but that doesn't cover zero-copy use
> cases; it has to be an interface like CharSequence.

Well, there is already the VarHandle approach for that.  But it's not
a particularly rich interface and very far away from strings.

> So the option I prefer is not on your list; it would be:
>
> (h) ByteSequence interface with retrofits to ByteBuffer, byte[], etc.
>
> This is more flexible than (f) the concrete ByteString class.  I think
> the ByteString you are thinking of would appear as a non-public class
> created by a ByteSequence factory, analogous to List::of.

Yes, this seems reasonable.  It's a bit of a drawback that the
immutable nature of a value cannot be expressed in the type system (so
that you have to remember to use ByteSequence::of to get a view to an
immutable object), but at least it's consistent with collections.

The store for byte strings

[Florian Weimer]

Lately I've been thinking about string representation.  The world
turned out not to be UCS-2 or UTF-16, after all, and we often have to
deal with strings generally encoded as ASCII or UTF-8, but we aren't
always encoded this way (and there might not even be a charset
declaration, see the ELF spec).

(a) byte[] with defensive copies.
Internal storage is byte[], copy is made before returning it to
the caller.  Quite common across the JDK.

(b) byte[] without defensive copies.
Internal storage is byte[], and a reference is returned.  In the
past, this could be a security bug, and usually, it was adjusted
to (a) when noticed.  Without security requirements, this can be
quite efficient, but there is ample potential for API misuse.

(c) java.lang.String with ISO-8859-1 decoding/encoding.
Sometimes done by reconfiguring the entire JVM to run with
ISO-8859-1, usually so that it is possible to process malformed
UTF-8.  The advantage is that there is rich API support, including
regular expressions, and good optimization.  There is also
language support for string literals.

(d) java.lang.String with UTF-8 decoding/encoding and replacement.
This seems to be very common, but is not completely accurate
and can lead to subtle bugs (or completely non-processible
data).  Otherwise has the same advantages as (c).

(e) Various variants of ByteBuffer.
Have not seen this much in practice (outside binary file format
parsers).  In the past, it needed deep defensive copies on input
for security (because there isn't an immutably backed ByteBuffer),
and shallow copies for access.  The ByteBuffer objects themselves
are also quite heavy when they can't be optimized away.  For that
reason, probably most useful on interfaces, and not for storage.

(f) Custom, immutable ByteString class.
Quite common, but has cross-library interoperability issues,
and a full complement of support (matching java.lang.String)
is quite hard.

(g) Something based on VarHandle.
Haven't seen this yet.  Probably not useful for storage.

Anything that I have missed?

Considering these choices, what is the expected direction on the JDK
side for new code?  Option (d) for things generally ASCII/UTF-8, and
(b) for things of a more binary nature?  What to do if the choice is
difficult?

Re: RFR 8195059: Update java.net Socket and DatagramSocket implementations to use Cleaner

[Florian Weimer]

* Roger Riggs:

> Updated in place.
>    http://cr.openjdk.java.net/~rriggs/webrev-net-cleanup-8195059/

Doesn't this leak the file descriptor of SocketCleanable.register
throws?

Re: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers

[Florian Weimer]

* Peter Levart:

> Simply saying that "vm is in a more critical status than a inflater is 
> getting leaked out" is, in my opinion, covering the problem under the 
> rug. The VM is not in critical state - the program is. VM is robust 
> enough to recover from OOMEs. The program might be in critical status 
> (i.e. in inconsistent state) partly because of not accounting for such 
> OOME situations. By taking care of them, the program has a better chance 
> of recovering from such situation(s).

On the other hand, memory leaks on OOM are extremely common, and for
the Inflater/ZStreamRef case, it might not be that critical to get
this absolutely airtight (particularly if the fix has non-trivial
concurrency implications).

> Handling native resources is one place where I think it is beneficial to 
> complicate things in order to make native resource leaks (im/less 
> )possible. The other such place is synchronization primitives. We must 
> admit that finalization does have this benefit that it makes it hard to 
> construct code that would allocate the native resource before cleanup 
> registration (which is performed as part of Object., while logic 
> to allocate native resource is usually invoked from subclass 
> constructor). To achieve the same robustness with Cleaner API, one has 
> to be careful to either perform registration upfront and then allocate 
> native resource or arrange for back-out in case of trouble.

If you allocate multiple resources, you probably need to apply the
same level of care with finalizers.  And the difficulty there lies in
implementing a close() operating which releases resources and inhibits
the effect of finalization.

Re: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers

[Florian Weimer]

* Xueming Shen:

> It might be possible to try-catch to expect Cleaner.register might
> fail, but I doubt it's really necessary here and it is the
> recommended usage of Cleaner?

That is actually why I posted this. 8-)

For similar functionality in other languages, it is often necessary to
perform all the required allocations for cleanup registration upfront,
then allocate the resource, and finally install it in the cleanup
handler.

A straightforward translation to the Cleaner facility and the
CleaningExample in the documentation would not allocate the
state-to-be-cleaned in the State constructor, but in a separate method
which is called after Cleaner.register().  But I don't know if this
could run into any concurrency issues.

Re: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers

[Florian Weimer]

* Xueming Shen:

> I removed the ln#657-#663, and updated the @apiNote in deflter, inflater
> and zipfile accordingly, mainly combined your comment and the approach
> for the fis/fo. they are "simpler" and straightforward now, at least for me.
>
> https://bugs.openjdk.java.net/browse/JDK-8187485
> http://cr.openjdk.java.net/~sherman/8185582/webrev

In ZipFile:

387 Inflater inf = getInflater();
388 InputStream is = new ZipFileInflaterInputStream(in, inf, 
(int)size,
389  () -> releaseInflater(inf));
390 synchronized (streams) {
391 streams.add(is);
392 }

Doesn't this leak the inflater if Cleaner.register() and thus the
ZipFileInflaterInputStream constructor throw?

Re: Replacement for sun.misc.Unsafe.allocateInstance(Class?) ?

[Florian Weimer]

On 08/01/2015 10:57 AM, Andrew Haley wrote:

 However, the security problems are great. I haven't heard any
 suggestion about how to expose this feature to user-created libraries
 without breaking Java security, and I suspect there may be none.

Are the problems greater than those of general reflection after
setAccessible(true)?  I don't think so.  I think the main objection
would be philosophical (against adding yet more trapdoors).  I respect
that—but at the same time, there does not seem to be a core technical
requirement why a suitable API with a proper permission check could not
be added to the JDK.

-- 
Florian Weimer / Red Hat Product Security

Re: ProcessBuilder support for pipelines

[Florian Weimer]

On 07/28/2015 02:48 AM, Martin Buchholz wrote:
 In the past, when I contemplated doing this, I generally thought that there
 wasn't enough value in such a feature for the effort, given that one can
 start a subprocess shell that supports pipelines.  Does this feature pull
 its weight?

Feeding program arguments to command spawned by a shell is difficult.
There is also no good way to obtain the exit status of the first command
in the pipe.  The last command in the pipe determines the shell exit
status, and earlier commands are ignored.

-- 
Florian Weimer / Red Hat Product Security

Re: StackOverflowError on HashMap.hashCode() due to self reference

[Florian Weimer]

On 07/17/2015 09:25 PM, Louis Wasserman wrote:
 The Javadoc of Map already specifies:
 
  Note: great care must be exercised if mutable objects are used as map
 keys. The behavior of a map is not specified if the value of an object is
 changed in a manner that affects equals comparisons while the object is a
 key in the map. *A special case of this prohibition is that it is not
 permissible for a map to contain itself as a key. While it is permissible
 for a map to contain itself as a value, extreme caution is advised:
 the **equals and hashCode methods
 are no longer well defined on such a map.*

The problematic entry had the self-reference in the value, not the key,
so this explanation does not really apply here.

But java.util.Collections does mention this problem:

“
Some collection operations which perform recursive traversal of the
collection may fail with an exception for self-referential instances
where the collection directly or indirectly contains itself. This
includes the clone(), equals(), hashCode() and toString() methods.
Implementations may optionally handle the self-referential scenario,
however most current implementations do not do so.
”

-- 
Florian Weimer / Red Hat Product Security

Re: stop using mmap for zip I/O

[Florian Weimer]

On 03/03/2015 05:32 AM, John Rose wrote:

 Most Java codes use FileOutputStream to write a file.  We could change its
 behavior to delete its output file instead of truncating.  This could be 
 fine-tuned
 by various knobs (properties, callbacks, etc.).  Then if the offending code 
 uses
 Java to write a file, it would no longer tickle this class of bugs.

On some systems, this may introduce a security vulnerability if the file
is in a shared directory for temporary files.

Regarding the original mapping problem, Linux prevents this failure
scenario for executable files (the ETXTBUSY error code).  The triggering
conditions for that are a bit bizarre.  It seems this only applies to
the main executable file, and not objects which are mapped subsequently.
 This is not too surprising because other options would allow
unprivileged users to prevent modification of any file.

-- 
Florian Weimer / Red Hat Product Security

Re: JEP 238: Multi-Version JAR Files

[Florian Weimer]

On 02/27/2015 06:16 PM, Paul Sandoz wrote:
 On Feb 27, 2015, at 4:47 PM, Florian Weimer fwei...@redhat.com wrote:
 I really don't think this tooling support will provide sufficient
 enticement to developers to maintain separate 7/8/9 source branches of
 their libraries.  Isn't that the main obstacle, and not the way the bits
 are delivered?

 
 What if all the source for 7/8/9 bits were under one project?

Tool support for that is still worse than for separate branches of the
same project.

In general, VCS support for branches is still quite poor because the
sensible thing to do is to develop a feature on the master branch and
then backport it to older release branches as needed.  But such
selective backwards-merging very much lacks VCS support because the
least common ancestor logic build into almost all current tools does not
apply in this scenario.  From a tool perspective, developing the feature
in the oldest relevant release and then merging forward is the only
supported way.  But it's usually bad engineering because you want new
features to use current facilities of the code base.  So the only thing
you have left is to cherry-pick individual patches (I think thats what
“hg transplant” does in Mercurial).

Anyway, you lose the tiny bit of tool support you have for backporting
if you have everything in the same source repository.

 For example, say there is a source file:
 
   src/main/java/foo/Foo.java
 
 whose content is:
 
   import sun.misc.BASE64Decoder;
 
   public class Foo {
 // does something with sun.misc.BASE64Decoder 
   }
 
 There might be another source file located in the 8 area that overrides that 
 in the unversioned area:
 
   src/main-8/java/foo/Foo.java
 
 whose content is:
 
   import java.util.Base64;
 
   public class Foo {
 // does something equivalent with java.util.Base64
   }

This really screams preprocessor. :-(

 The public contract of Foo should remain identical across the major Java 
 platform dependent versions, in a more strict sense the public signatures in 
 the byte code should be identical (the jar tool has been modified to check 
 this).

If that's the goal, something like Retroweaver seems far more
appropriate than forcing library authors to maintain separate sources.

Multi-version JAR files seem useful because they will eventually allow
evolution of the class file format.  But I think the current use case
isn't really there, I'm afraid.

-- 
Florian Weimer / Red Hat Product Security

Re: JEP 238: Multi-Version JAR Files

[Florian Weimer]

On 02/12/2015 09:52 PM, Paul Sandoz wrote:
 Hi
 
 In connection with the JEP there is also a design document to help the 
 discussion:
 
   http://cr.openjdk.java.net/~psandoz/jdk9/MultiVersionJar-8u60-9-design.md
 
 We are especially interesting in hearing feedback from library developers, 
 tool/IDE developers, and anyone doing funky stuff with class loading and JAR 
 files.

I'm wondering how you propose to build such JAR files.  Do you think
library developers will maintain two separate branches, compile one with
JDK 8, the other one with JDK 9, and then use some (not yet existing?)
tool to merge the output into a single JAR?  Is such automated merging
even possible if the bytecode was compiled with different javac versions?

What about presenting Javadoc in a useful fashion?

-- 
Florian Weimer / Red Hat Product Security

Re: JEP 238: Multi-Version JAR Files

[Florian Weimer]

On 02/27/2015 03:20 PM, Alan Bateman wrote:
 On 27/02/2015 13:27, Florian Weimer wrote:
 :
 I'm wondering how you propose to build such JAR files.  Do you think
 library developers will maintain two separate branches, compile one with
 JDK 8, the other one with JDK 9, and then use some (not yet existing?)
 tool to merge the output into a single JAR?
 
 This submitted JEP is good reading and gives an idea how how to compile
 to older JDKs:
   http://openjdk.java.net/jeps/8058150

Maybe I misunderstood the multi-version JAR files proposal, but I
thought that the assumption there is that there are actual *source*
files which use newer features of the platform.

I really don't think this tooling support will provide sufficient
enticement to developers to maintain separate 7/8/9 source branches of
their libraries.  Isn't that the main obstacle, and not the way the bits
are delivered?

-- 
Florian Weimer / Red Hat Product Security

Re: [PATCH] CipherStream produces new byte array on every update or doFinal operation

[Florian Weimer]

On 02/17/2015 01:53 PM, Dai Nakanishi wrote:
 +} catch (ShortBufferException e) {
 +obuffer = null;
 +throw new IOException(e);
  }

This doesn't look right to me.  You need to enlarge the buffer and retry.

If you really want to avoid allocations, you should use the destination
buffer passed to the read() function if the slice end is equal to the
array end.  I expect that this is the usual case.

By the way, I think such review requests should be sent to security-dev,
not core-libs-dev.

-- 
Florian Weimer / Red Hat Product Security

Re: RFR: 8073093: AARCH64: C2 generates poor code for ByteBuffer accesses

[Florian Weimer]

On 02/17/2015 11:22 AM, Andrew Haley wrote:
 You'll still have to allocate a wrapping ByteBuffer object to use them.
  I expect that makes them unattractive in many cases.
 
 Hmm.  I'm having a hard time trying to understand why.  If you need to
 do a lot of accesses the allocation of the ByteBuffer won't be
 significant; if you don't need to do a lot of accesses it won't
 matter either.

The typical use case I have in mind is exemplified by
com.sun.crypto.provider.GHASH(processBlock(byte[] data, int ofs):

 174 private void processBlock(byte[] data, int ofs) {
 175 if (data.length - ofs  AES_BLOCK_SIZE) {
 176 throw new RuntimeException(need complete block);
 177 }
 178 state0 ^= getLong(data, ofs);
 179 state1 ^= getLong(data, ofs + 8);
 180 blockMult(subkeyH0, subkeyH1);
 181 }

That is, the byte array is supplied by the caller, and if we wanted to
use a ByteBuffer, we would have to allocate a fresh one on every
iteration.  In this case, neither of the two alternatives you list apply.

-- 
Florian Weimer / Red Hat Product Security

Re: RFR: 8073093: AARCH64: C2 generates poor code for ByteBuffer accesses

[Florian Weimer]

On 02/14/2015 01:09 AM, John Rose wrote:
 These queries need to go into Unsafe.
 We also need Unsafe.getIntMisaligned, etc., which wire through to whatever 
 second-best mechanism the platform offers.

The safe variants should go into the java.lang.Integer etc. classes
IMHO.  Even the JDK has quite a few uses for them (particularly the big
endian variant).  Putting that into Unsafe only encourages further use
of Unsafe from application code.

-- 
Florian Weimer / Red Hat Product Security

Re: RFR: 8073093: AARCH64: C2 generates poor code for ByteBuffer accesses

[Florian Weimer]

On 02/14/2015 11:10 PM, Dean Long wrote:

 Even if linux-aarch64 always allows unaligned, checking only for
 aarch64 is not future-proof
 because it doesn't take the OS into account.

Surely a simple test case is sufficient to ensure that the platform
supports misaligned accesses?  Then new ports will see the failure
immediately and can tweak the code.

-- 
Florian Weimer / Red Hat Product Security

Re: RFR: 8073093: AARCH64: C2 generates poor code for ByteBuffer accesses

[Florian Weimer]

On 02/17/2015 11:00 AM, Andrew Haley wrote:
 On 02/17/2015 09:39 AM, Florian Weimer wrote:
 On 02/14/2015 01:09 AM, John Rose wrote:
 These queries need to go into Unsafe.
 We also need Unsafe.getIntMisaligned, etc., which wire through to whatever 
 second-best mechanism the platform offers.

 The safe variants should go into the java.lang.Integer etc. classes
 IMHO.  Even the JDK has quite a few uses for them (particularly the
 big endian variant).  Putting that into Unsafe only encourages
 further use of Unsafe from application code.
 
 They'll all be visible as ByteBuffer methods, which should be enough
 for application code, shouldn't it?  I'm not sure how much sense it
 makes to put them into java.lang.Integer etc.

You'll still have to allocate a wrapping ByteBuffer object to use them.
 I expect that makes them unattractive in many cases.

Hmm, maybe I should propose a patch for DataInputStream and see how it's
received. :-)

-- 
Florian Weimer / Red Hat Product Security

Re: Time to retire System.runFinalizersOnExit?

[Florian Weimer]

On 01/30/2015 06:03 PM, Mandy Chung wrote:
 I see a few callers in Fedora in test suites, but only hsqldb as an
 actual user in the installed JAR files.
 
 Do you see any issue converting them?

I haven't tried.  I assume that any changes implemented will actually
increase resiliency of the test suite.  From that perspective, throwing
UnsupportedOperationException is better than just silently doing nothing.

-- 
Florian Weimer / Red Hat Product Security

Re: ByteBuffer.wrap(array).getInt(offset)

[Florian Weimer]

On 08/20/2014 06:43 PM, Andrew Haley wrote:
 On 08/20/2014 04:10 PM, Florian Weimer wrote:
 Is there already a way to compute the expression in the subject without 
 the ByteBuffer allocation?  I saw quite a few equivalent formulations in 
 the OpenJDK sources, and perhaps it's time to add a standardized 
 solution.
 
 Isn't this really calling for intrinsification of ByteBuffers?  With that,
 C2 would know that the ByteBuffer doesn't escape, and could collapse the
 lot into a load.

It's a lot of heavy machinery for something that should be quite simple.
 I also expect quite a few users during VM startup.

-- 
Florian Weimer / Red Hat Product Security

Re: JDK 9 RFR of JDK-8071434: doc updates for java.lang.Object

[Florian Weimer]

On 01/29/2015 09:53 PM, joe darcy wrote:
 + * As much as is reasonably practical, the hashCode method defined
 + * by class {@code Object} does return distinct integers for
 + * distinct objects. (The hashCode may or may not be implemented
 + * as some function of an object's memory address at some point
 + * in time.)

Is it worth mentioning the performance cost of the default
implementation, for typical VMs?

-- 
Florian Weimer / Red Hat Product Security