RE: RFR: 8160225: java.time.format.DateTimeFormatter issues for month-of-year
Hi Naoto, Thank you for the response. You are correct. I ran the reproducer against JDK13 and JDK9, and they appear not to differentiate between L and M as long as the input is consistent. I will update the bug and revise the effort accordingly. Thanks!! -Original Message- From: Naoto Sato Sent: Tuesday, July 30, 2019 9:39 PM To: Thejasvi Voniadka ; core-libs-dev@openjdk.java.net; i18n-...@openjdk.java.net Subject: Re: RFR: 8160225: java.time.format.DateTimeFormatter issues for month-of-year Hi Thejasvi, M/L does not designate textual nor numeric. Thus I don't think that the suggested documentation fix is correct. Furthermore, although the exception in JDK8 looks like a bug, the test result with JDK9 looks correct to me. The month displayed as "04" is the result of ZonedDateTime.toString() so should not be localized. Naoto On 7/30/19 5:54 AM, Thejasvi Voniadka wrote: > Hi, > > Request your review of this simple change. > > JBS:https://bugs.openjdk.java.net/browse/JDK-8160225 > (java.time.format.DateTimeFormatter issues for month-of-year) > > Description:It is a simple documentation change. The DateTimeFormatter > expects the month format to be represented by "L" for number and "M" for text > (eg: "Jul" may be accepted by a format string "MMM"; "07" may be accepted by > a format string "LL", and so on). However, the documentation lists this > somewhat confusingly: > > "M/L month-of-year number/text 7; 07; Jul; July; J" > > A casual reader may interpret "M" as the numeric representation and "L" as > the textual representation of the month-of-year, whereas the actual behavior > of the API is the other way around. This patch fixes it. > > > Webrev:http://cr.openjdk.java.net/~vagarwal/8160225/webrev.0/ > > > >
Re: RFR: 8160225: java.time.format.DateTimeFormatter issues for month-of-year
Hi Thejasvi, M/L does not designate textual nor numeric. Thus I don't think that the suggested documentation fix is correct. Furthermore, although the exception in JDK8 looks like a bug, the test result with JDK9 looks correct to me. The month displayed as "04" is the result of ZonedDateTime.toString() so should not be localized. Naoto On 7/30/19 5:54 AM, Thejasvi Voniadka wrote: Hi, Request your review of this simple change. JBS:https://bugs.openjdk.java.net/browse/JDK-8160225 (java.time.format.DateTimeFormatter issues for month-of-year) Description:It is a simple documentation change. The DateTimeFormatter expects the month format to be represented by "L" for number and "M" for text (eg: "Jul" may be accepted by a format string "MMM"; "07" may be accepted by a format string "LL", and so on). However, the documentation lists this somewhat confusingly: "M/L month-of-year number/text 7; 07; Jul; July; J" A casual reader may interpret "M" as the numeric representation and "L" as the textual representation of the month-of-year, whereas the actual behavior of the API is the other way around. This patch fixes it. Webrev:http://cr.openjdk.java.net/~vagarwal/8160225/webrev.0/
Re: RFR: 8224974: Implement JEP 352
On 7/30/19 5:04 PM, Andrew Dinn wrote:
> JEP 352 has now been targeted for inclusion in JDK14. The latest webrev
> for the implementation JIRA has been rebased to apply to the current
> tree. Is it now ok to push this change set?
>
> JIRA: https://bugs.openjdk.java.net/browse/JDK-8224974
> webrev: http://cr.openjdk.java.net/~adinn/8224974/webrev.09
Is it okay to have a late code review? Here it goes:
=== General:
So if pre wbsync is no-op, why do we need to handle it everywhere? We seem to
be falling through all
the way to the stub to do nothing there, maybe we should instead cut much
earlier, e.g. when
inlining Unsafe.writeBackPresync0? Would it be better to not emit
CacheWBPreSync at all?
=== General:
IIRC, notproduct and PRODUCT defines are legacy, and should be avoided? develop
and ASSERT must be
the substitutes for this. See some discussion here:
https://bugs.openjdk.java.net/browse/JDK-8183287
=== src/hotspot/cpu/aarch64/aarch64.ad
src/hotspot/cpu/x86/x86.ad
This should probably be just "!VM_Version...". Braces around the statement
would not hurt either.
2196 if (VM_Version::supports_data_cache_line_flush() == false)
2197 ret_value = false;
=== src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp
src/hotspot/cpu/x86/macroAssembler_x86.cpp
Braces style mismatch, should be on the same line, as in the rest of the file:
5837 void MacroAssembler::cache_wb(Address line)
5838 {
5856 void MacroAssembler::cache_wbsync(bool is_pre)
5857 {
=== src/hotspot/cpu/x86/assembler_x86.cpp
It feels like these comments are redundant, especially L8630 and L8646 which
mention magic values
"6" and "7", not present in the code:
8624 // instruction prefix is 0x66
8627 // opcode family is 0x0f 0xAE
8630 // extended opcode byte is 7 == rdi
8640 // instruction prefix is 0x66
8643 // opcode family is 0x0f 0xAE
8646 // extended opcode byte is 6 == rsi
=== src/hotspot/cpu/x86/macroAssembler_x86.cpp
These comments feel redundant too. Well, maybe they should instead talk about
the choices the
subsequent code makes.
9915 // pick the correct implementation
9936 // pick the correct implementation
=== src/hotspot/cpu/x86/macroAssembler_x86.cpp
src/hotspot/cpu/x86/vm_version_x86.hpp
The docs say "The CLFLUSH instruction was introduced with the SSE2 extensions;
however, because it
has its own CPUID feature flag, it can be implemented in IA-32 processors that
do not include the
SSE2 extensions. Also, detecting the presence of the SSE2 extensions with the
CPUID instruction does
not guarantee that the CLFLUSH instruction is implemented in the processor."
Yet, we have:
9910 // 64 bit cpus always support clflush
9911 assert(VM_Version::supports_clflush(), "should not reach here on
32-bit");
505 #ifdef _LP64
506 // cpflush is always available on x86_64
507 result |= CPU_FLUSH;
508 #else
509 if (_cpuid_info.std_cpuid1_edx.bits.clflush != 0)
510 result |= CPU_FLUSH;
511 #endif
967 // 64 bit cpus always support clflush which writes back and evicts
968 // on 32 bit cpus support is recorded via a feature flag
980 static bool supports_clflush() { return true; }
I think the assert should just say "clflush should be available", and clflush
cpu flag detected for
64-bits too?
=== src/hotspot/cpu/x86/macroAssembler_x86.hpp
Accidental camelCase, while hotspot code expects snake_case:
1794 void cache_wbsync(bool isPre);
=== src/hotspot/cpu/x86/stubGenerator_x86_64.cpp
Any reason to avoid inline here, e.g. "__ cache_wb(Address(src, 0))"?
2921 const Address line(src, 0);
2922 __ cache_wb(line);
=== src/hotspot/cpu/x86/stubGenerator_x86_64.cpp
Is it really "cmpl" here, not "cmpb"? I think aarch64 code tests for byte.
2942 __ cmpl(is_pre, 0);
=== src/hotspot/share/classfile/vmSymbols.hpp
Excess space before "jdk_internal..." here:
1096 do_intrinsic(_writebackPostSync0,jdk_internal_misc_Unsafe,
writebackPostSync0_name, void_method_signature , F_RN) \
=== src/hotspot/share/opto/c2compiler.cpp
Why inject new cases here, instead of at the end of switch? Saves sudden
"break":
578 break;
579 case vmIntrinsics::_writeback0:
580 if (!Matcher::match_rule_supported(Op_CacheWB)) return false;
581 break;
582 case vmIntrinsics::_writebackPreSync0:
583 if (!Matcher::match_rule_supported(Op_CacheWBPreSync)) return false;
584 break;
585 case vmIntrinsics::_writebackPostSync0:
586 if (!Matcher::match_rule_supported(Op_CacheWBPostSync)) return false;
587 break;
=== src/hotspot/share/opto/library_call.cpp
Accidental camelCase here, should be snake_case:
257 bool inline_unsafe_writebackSync0(bool isPre);
2870 bool LibraryCallKit::inline_unsafe_writebackSync0(bool isPre) {
=== src/hotspot/share/prims/unsafe.cpp
Odd indenting near "CC":
1122 {CC "writeback0", CC "(" "J" ")V",
FN_PTR(Unsafe_WriteBack0)},
1123 {CC
Re: RFR: 8224974: Implement JEP 352
JEP 352 has now been targeted for inclusion in JDK14. The latest webrev
for the implementation JIRA has been rebased to apply to the current
tree. Is it now ok to push this change set?
JIRA: https://bugs.openjdk.java.net/browse/JDK-8224974
webrev: http://cr.openjdk.java.net/~adinn/8224974/webrev.09
n.b. by way of sanity test I pushed this to the submit repo and it came
back with no failed tests.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: Review Request: JDK-8209005: Lookup.unreflectSpecial fails for default methods when Lookup.findSpecial works
Think about aMethod is a protected method inherited from its superclass
T. To invoke aMethod, the receiver must be an instance of T or a
subclass of T.
Mandy
On 7/30/19 3:22 AM, Daniel Fuchs wrote:
Hi Mandy,
380 * {@link
java.lang.invoke.MethodHandles.Lookup#unreflectSpecial
lookup.unreflectSpecial(aMethod,this.class)}
381 * {@code T m(A*);}{@code (T)
aMethod.invoke(this, arg*);}
Is this exactly true? I mean - if `this` is an instance of
a subclass of `aMethod.getDeclaringClass()`, and if that
subclass overrides `aMethod`, then I would expect
`aMethod.invoke(this, arg*)` to execute the bytecode
of the method defined in the subclass.
If I'm not mistaken, the test does expect that the
bytecode in the super class is executed instead.
I suspect that `unreflectSpecial` can only be specified
in terms of `findSpecial`. But maybe I'm missing something.
I'm not too familiar with the intricacies of MethodHandle.
best regards,
-- daniel
On 26/07/2019 18:41, Mandy Chung wrote:
Daniel noticed that `unreflectSpecial` is missing in the "Lookup
Factory Methods" section in the class spec. In fact there are a
duplicated `lookup.unreflect(aMethod)` row that might originally be
for `unreflectSpecial`. I fix the javadoc in this patch:
http://cr.openjdk.java.net/~mchung/jdk14/8209005/webrev.01/
Mandy
On 7/25/19 1:12 PM, Mandy Chung wrote:
This patch fixes Lookup.unreflectSpecial to pass the declaring class
of Method being unreflected (rather than null) so that it can
accurately check if the special caller class is either the lookup
class or a superinterface of the declaring class.
Webrev:
http://cr.openjdk.java.net/~mchung/jdk14/8209005/webrev.00/index.html
The test runs in both unnamed module and named module to cover
JDK-8209078 which has been resolved by JDK-8173978.
thanks
Mandy
RFR: 8228778: JDK 13 L10n resource files update - msg drop 20
Hi, Please help to review the update of L10n resource files in JDK13 msg drop 20. Bug: https://bugs.openjdk.java.net/browse/JDK-8228778 Webrev: http://cr.openjdk.java.net/~ljiang/8228778/webrev/read/ Thanks, Leo
RFR: 8160225: java.time.format.DateTimeFormatter issues for month-of-year
Hi, Request your review of this simple change. JBS:https://bugs.openjdk.java.net/browse/JDK-8160225 (java.time.format.DateTimeFormatter issues for month-of-year) Description:It is a simple documentation change. The DateTimeFormatter expects the month format to be represented by "L" for number and "M" for text (eg: "Jul" may be accepted by a format string "MMM"; "07" may be accepted by a format string "LL", and so on). However, the documentation lists this somewhat confusingly: "M/L month-of-year number/text 7; 07; Jul; July; J" A casual reader may interpret "M" as the numeric representation and "L" as the textual representation of the month-of-year, whereas the actual behavior of the API is the other way around. This patch fixes it. Webrev:http://cr.openjdk.java.net/~vagarwal/8160225/webrev.0/
Issue with SSL handshake implementation
Hi, There may be some issue with the SSL handshake implementation. In sun.security.x509.X500Name.java, there's a static hashmap field named *internedOIDs*, used for caching X.500 attributes. Each time a new oid is encountered, jdk will cache them into the static hashmap. With a purposely crafted cert on the client side, it's possible to create some long oids, and letting the server save them permanently during SSL handshake, which will eat server memory, and cause OOM & DoS in the end. Is it necessary to make some changes to this? BTW, The issue was reported by one of our customers, with the following stacktrace. Although they got this on jdk8u212, I believe the same issue exists in jdk11 too. ``` http-bio-172.24.18.21-443-exec-4 at java.lang.OutOfMemoryError.()V (OutOfMemoryError.java:48) at java.util.Arrays.copyOf([Ljava/lang/Object;I)[Ljava/lang/Object; (Arrays.java:3181) at java.util.Vector.grow(I)V (Vector.java:269) at java.util.Vector.ensureCapacityHelper(I)V (Vector.java:249) at java.util.Vector.addElement(Ljava/lang/Object;)V (Vector.java:623) at sun.security.util.DerInputStream.readVector(I)[Lsun/security/util/DerValue; (DerInputStream.java:425) at sun.security.util.DerInputStream.getSequence(I)[Lsun/security/util/DerValue; (DerInputStream.java:331) at sun.security.x509.X500Name.parseDER(Lsun/security/util/DerInputStream;)V (X500Name.java:793) at sun.security.x509.X500Name.(Lsun/security/util/DerInputStream;)V (X500Name.java:306) at sun.security.x509.X509CertInfo.parse(Lsun/security/util/DerValue;)V (X509CertInfo.java:649) at sun.security.x509.X509CertInfo.(Lsun/security/util/DerValue;)V (X509CertInfo.java:167) at sun.security.x509.X509CertImpl.parse(Lsun/security/util/DerValue;)V (X509CertImpl.java:1804) at sun.security.x509.X509CertImpl.([B)V (X509CertImpl.java:195) at sun.security.provider.X509Factory.engineGenerateCertificate(Ljava/io/InputStream;)Ljava/security/cert/Certificate; (X509Factory.java:102) at java.security.cert.CertificateFactory.generateCertificate(Ljava/io/InputStream;)Ljava/security/cert/Certificate; (CertificateFactory.java:339) at sun.security.ssl.HandshakeMessage$CertificateMsg.(Lsun/security/ssl/HandshakeInStream;)V (HandshakeMessage.java:455) at sun.security.ssl.ServerHandshaker.processMessage(BI)V (ServerHandshaker.java:230) at sun.security.ssl.Handshaker.processLoop()V (Handshaker.java:1037) at sun.security.ssl.Handshaker.process_record(Lsun/security/ssl/InputRecord;Z)V (Handshaker.java:965) at sun.security.ssl.SSLSocketImpl.readRecord(Lsun/security/ssl/InputRecord;Z)V (SSLSocketImpl.java:1064) at sun.security.ssl.SSLSocketImpl.performInitialHandshake()V (SSLSocketImpl.java:1367) at sun.security.ssl.SSLSocketImpl.startHandshake(Z)V (SSLSocketImpl.java:1395) at sun.security.ssl.SSLSocketImpl.getSession()Ljavax/net/ssl/SSLSession; (SSLSocketImpl.java:2288) at org.apache.tomcat.util.net.jsse.JSSESocketFactory.handshake(Ljava/net/Socket;)V (JSSESocketFactory.java:293) at org.apache.tomcat.util.net.JIoEndpoint$SocketProcessor.run()V (JIoEndpoint.java:343) at java.util.concurrent.ThreadPoolExecutor.runWorker(Ljava/util/concurrent/ThreadPoolExecutor$Worker;)V (ThreadPoolExecutor.java:1149) at java.util.concurrent.ThreadPoolExecutor$Worker.run()V (ThreadPoolExecutor.java:624) at org.apache.tomcat.util.threads.TaskThread$WrappingRunnable.run()V (TaskThread.java:61) at java.lang.Thread.run()V (Thread.java:748) ```
Re: RFR: JDK-8228744: file associations broken on linux.
Looks good. - Alexey On 7/29/2019 3:19 PM, Andy Herrick wrote: Please review the jpackage fix for bug [1] at [2]. This is a fix for the JDK-8200758-branch branch of the open sandbox repository (jpackage). [1] https://bugs.openjdk.java.net/browse/JDK-8228744 [2] http://cr.openjdk.java.net/~herrick/8228744/ /Andy
