Re: RFR: JDK-8212091 : Move native code under platform specific folders and files

[Magnus Ihse Bursie]

On 2019-02-15 04:31, Alexander Matveev 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).


- Moved native code under platform specific folder.
- Removed most usage on #ifdefs for WINDOWS, LINUX, MAC and POSIX.
- MAC define is still used in JavaVirtualMachine.cpp and Package.cpp 
for Mac specific code to filter out some arguments. I decided to keep 
it as is for now, since Mac specific code is small.
- Defines are used in Platform.cpp to initialize platform specific 
classes.

- Removed all pragma warning and fixed all compilation warnings.
- Removed unused code.

[1] https://bugs.openjdk.java.net/browse/JDK-8212091

[2] http://cr.openjdk.java.net/~almatvee/8212091/webrev.00/
The JDK standard is to use "unix", not "posix", for the shared 
functionality between linux/solaris/macosx. You can keep the name 
"PosixPlatform.*" if you want, though; the important thing is the 
directory name.


Also, if you do that, you do not need any changes to 
make/lib/Lib-jdk.jpackage.gmk, since that will be automatically 
understood by the build system.


It looks from the webrev that you have "moved" the files by doing "hg 
add" and "hg remove". Please use "hg move" instead -- this will keep 
history intact, and it allows reviewers to see if you have also made 
changes to the moved files.


(If you do have modified the moved file, reverting a "hg add+hg remove" 
process is a bit more tricky -- you need to do "hg forget" on the new 
file, rename it to something else (otherwise "hg move" will complain), 
"hg revert" the old file back in place, do a "hg move" from the old to 
the new, and then copy the modified, renamed file back over the target 
new file again.)


/Magnus


Thanks,
Alexander

RFR: JDK-8212091 : Move native code under platform specific folders and files

[Alexander Matveev]

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).


- Moved native code under platform specific folder.
- Removed most usage on #ifdefs for WINDOWS, LINUX, MAC and POSIX.
- MAC define is still used in JavaVirtualMachine.cpp and Package.cpp for 
Mac specific code to filter out some arguments. I decided to keep it as 
is for now, since Mac specific code is small.

- Defines are used in Platform.cpp to initialize platform specific classes.
- Removed all pragma warning and fixed all compilation warnings.
- Removed unused code.

[1] https://bugs.openjdk.java.net/browse/JDK-8212091

[2] http://cr.openjdk.java.net/~almatvee/8212091/webrev.00/

Thanks,
Alexander

Re: RFR: 8216363: NullPointerException in java.util.logging.Handler#isLoggable

[Mandy Chung]

On 2/14/19 12:39 PM, Daniel Fuchs wrote:

Let me redo my fix and see if the JCK complains.


I'd be surprised if there is a JCK test expecting NPE. Will see.

I suggest to update @param record to say "a LogRecord or null"
to be explicit.

Mandy

Re: RFR: 8216363: NullPointerException in java.util.logging.Handler#isLoggable

[Daniel Fuchs]

Hi Mandy,

On 14/02/19 20:20, Mandy Chung wrote:

Fixing the implementation of Handler::isLoggable to return false
if null to match the specification seems similar risk to changing
the spec.  What do you think?


How so? I mean - if no line of code is changed, then surely we can't
break existing code? But on second thought I believe I may
have made the wrong choice her. See below...



In addition, logging tends to be gracious as it's a
cross-cutting concern and not to disturb the application runtime
until it's a runtime issue.  So I think isLoggable accepting null
may be by design.


I agree that returning false would be a much better implementation
than throwing NPE. StreamHandler already does that. The only
concrete implementation (in the JDK) of Handler that doesn't
return false is the memory handler.


Handler.publish(LogRecord) specifies to ignore null record and
the implementation calls Handler.isLoggable(record). If the spec
is changed to throw NPE, Handler::publish will need to change
too.  This impacts all APIs that accept null LogRecord while
assuming isLoggable(null) returns false.


That is a good point.


I think normal logging purpose would pass non-null records
otherwise this issue should have been reported long ago.


Well... That and the fact that FileHandler, ConsoleHandler,
and friends are all StreamHandler (and return false).

Maybe you're right - and changing the implementation
is the right thing to do.

Let me redo my fix and see if the JCK complains.

Thanks for the feedback!

best regards,

-- daniel

Re: RFR: 8216363: NullPointerException in java.util.logging.Handler#isLoggable

[Mandy Chung]

Hi Daniel,

I wonder how a null LogRecord can be passed to Handler::isLoggable
in the code path during logging.

The package summary specifies that NPE will be thrown for null
argument unless it's explicitly specified.  Handler.isLoggable
does specify to return false if LogRecord is null.  If you change
the spec to throw NPE if record is null, this impacts all callers
of isLoggable(record) if any of them assumes it returns false
if not.  In addition, logging tends to be gracious as it's a
cross-cutting concern and not to disturb the application runtime
until it's a runtime issue.  So I think isLoggable accepting null
may be by design.

Handler.publish(LogRecord) specifies to ignore null record and
the implementation calls Handler.isLoggable(record). If the spec
is changed to throw NPE, Handler::publish will need to change
too.  This impacts all APIs that accept null LogRecord while
assuming isLoggable(null) returns false.

I think normal logging purpose would pass non-null records
otherwise this issue should have been reported long ago.

Fixing the implementation of Handler::isLoggable to return false
if null to match the specification seems similar risk to changing
the spec.  What do you think?

Mandy

On 2/14/19 11:10 AM, Daniel Fuchs wrote:

Hi,

Please find below a doc fix for:

8216363: NullPointerException in java.util.logging.Handler#isLoggable
https://bugs.openjdk.java.net/browse/JDK-8216363

webrev:
http://cr.openjdk.java.net/~dfuchs/webrev_8216363/webrev.00/index.html

java.util.logging.Handler specifies that Handler.isLoggable(null)
returns false. Unfortunately, the implementation throws NPE instead.

For the sake of backward compatibility - it might be more
prudent to simply alter the API specification to document
the behavior of the default implementation, while allowing
subclasses to change that behavior. In fact, one already
does: StreamHandler::isLoggable(LogRecord) already returns
false and doesn't throw NPE.


best regards,

-- daniel

Re: JDK-6982173: Small problem causing thousands of wasted CPU hours

[Martin Buchholz]

On Wed, Feb 13, 2019 at 6:45 PM Stuart Marks 
wrote:

>
> If we all agree on this, I'll proceed with working up a changeset for
> JDK-6394757.[3] It's time to fix this one.
>

Thanks for taking on the fixing of this unfixable problem.
It's important to do lots of documentation/guidance work, esp. a release
note.
Doug and I will take care of anything that needs doing  in jsr166.

In the real world, if performance is important, one might want to sort both
collections (or use collections that are naturally sorted), then do a
two-finger walk through both collections, funneling the results into a
target collection.
Which is N*log(N) + M*log(M) + N + M
With enough work, that might be library-fiable.
Hm ...
Imagine ConcurrentSkipListSet.removeAll(aNavigableSet) ...
Then by two-finger through both collections, advancing using
higher/ceiling, you should get something like O(min(N*log(N), M*log(M)))
If the argument is not sorted, then it is likely to be worth the effort of
copying it into a sorted set (TreeSet with same comparator?).
Software is hard.

Re: RFR: 8216363: NullPointerException in java.util.logging.Handler#isLoggable

[Lance Andersen]

Hi Daniel,
> On Feb 14, 2019, at 2:22 PM, Daniel Fuchs  wrote:
> 
> Hi Lance,
> 
> On 14/02/2019 20:18, Lance Andersen wrote:
>> Documenting the current behavior as it has been makes the most sense to me 
>> as well
>> Have you created a CSR yet? if so I will add myself as a reviewer?
> 
> I haven't created a CSR yet.
> 
> This is a small low-priority fix - so it seemed preferable
> to wait for review comments before starting the CSR process.

Gotcha,  I think your proposed wording is fine.
> 
> Thanks for volunteering for the CSR review, I'll ping you when
> I have it ;-)

You are very welcome!

> 
> best regards,
> 
> -- daniel
> 
> 

Best
Lance

 
  

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

Re: RFR: 8216363: NullPointerException in java.util.logging.Handler#isLoggable

[Daniel Fuchs]

Hi Lance,

On 14/02/2019 20:18, Lance Andersen wrote:
Documenting the current behavior as it has been makes the most sense to 
me as well


Have you created a CSR yet? if so I will add myself as a reviewer?



I haven't created a CSR yet.

This is a small low-priority fix - so it seemed preferable
to wait for review comments before starting the CSR process.

Thanks for volunteering for the CSR review, I'll ping you when
I have it ;-)

best regards,

-- daniel

Re: RFR: 8216363: NullPointerException in java.util.logging.Handler#isLoggable

[Lance Andersen]

Hi Daniel
> On Feb 14, 2019, at 2:10 PM, Daniel Fuchs  wrote:
> 
> Hi,
> 
> Please find below a doc fix for:
> 
> 8216363: NullPointerException in java.util.logging.Handler#isLoggable
> https://bugs.openjdk.java.net/browse/JDK-8216363
> 
> webrev:
> http://cr.openjdk.java.net/~dfuchs/webrev_8216363/webrev.00/index.html
> 
> java.util.logging.Handler specifies that Handler.isLoggable(null)
> returns false. Unfortunately, the implementation throws NPE instead.
> 
> For the sake of backward compatibility - it might be more
> prudent to simply alter the API specification to document
> the behavior of the default implementation, while allowing
> subclasses to change that behavior. In fact, one already
> does: StreamHandler::isLoggable(LogRecord) already returns
> false and doesn't throw NPE.

Documenting the current behavior as it has been makes the most sense to me as 
well

Have you created a CSR yet? if so I will add myself as a reviewer?

> 
> 
> best regards,
> 
> -- daniel

 
  

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

RFR: 8216363: NullPointerException in java.util.logging.Handler#isLoggable

[Daniel Fuchs]

Hi,

Please find below a doc fix for:

8216363: NullPointerException in java.util.logging.Handler#isLoggable
https://bugs.openjdk.java.net/browse/JDK-8216363

webrev:
http://cr.openjdk.java.net/~dfuchs/webrev_8216363/webrev.00/index.html

java.util.logging.Handler specifies that Handler.isLoggable(null)
returns false. Unfortunately, the implementation throws NPE instead.

For the sake of backward compatibility - it might be more
prudent to simply alter the API specification to document
the behavior of the default implementation, while allowing
subclasses to change that behavior. In fact, one already
does: StreamHandler::isLoggable(LogRecord) already returns
false and doesn't throw NPE.


best regards,

-- daniel

Re: JDK-6982173: Small problem causing thousands of wasted CPU hours

[Alan Snyder]

Right, I see that now.

Care must be exercised if this method is used on collections that
do not comply with the general contract for {@code Collection}.

So, what does this mean? Are we catering to incorrect implementations?



> On Feb 13, 2019, at 9:07 PM, Stuart Marks  wrote:
> 
> On 2/13/19 7:22 PM, Alan Snyder wrote:
>> If we take this route, how about changing the parameter type to Iterable?
> 
> Won't work. Where I've ended up is that we need to iterate over "this" 
> collection and, for each element, call contains() on the parameter. The 
> AbstractCollection.removeAll() implementation does something like this:
> 
>removeAll(Collection c) {
>for (Iterator it = iterator(); it.hasNext();) {
>if (c.contains(it.next())) {
>it.remove();
>}
>}
>}
> 
> Since we call contains() on the parameter, it has to be a Collection.
> 
> s'marks
> 

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

Hi Roger,

> Maybe 10 years ago, when native was the only solution.
> Now there are tools to analyze bytecode in java.
I'm working on a Java implementation. 

> > Peter Levart proposed to initialize the message with a sentinel instead.
> > I'll implement this as a proposal.
> That's an extra overhead too, any assignment is.
Yes, any code requires some run time. But I think this really is negligible
in comparison to generating the backtrace, which happens on every
exception.  But I'm fine with the 'always recompute, no serialization' 
solution. If it turns out to be limited, it still can be changed easily.

> > I guess we can go without. 
> > It would be possible to construct a case where two threads
> > do getMessage() on the same NPE Object but the string is not 
> > the same.
> Really!, if the bci is the same, the bytecode is the same, what could be 
> different
> that would change the data used to create the exception?
e.getMessage().equals(e.getMessage()) will hold, but
e.getMessage() != e.getMessage().

I just implemented the two variants of computing the message, they 
basically differ in NullPointerException.java:
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/03-PeterLevart/
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/04-RogerRiggs/

I also cleaned up the parameters passed as proposed.

> > We uses this code since 2006, it needed only few changes.  I would like to 
> > contribute a follow up for hidden frames of lambdas.
> Valhalla and evolution of the byte code needs to be considered.
> Just because its worked for years does not mean it's the best approach
> today.  Dropping it in now means maintaining it in its current form
> or needing to re-write it later.
Well, yes, that is true. For any change.  For any project.  We have heard
this argument for many of our changes. I don't really think it's a good
argument. As I understand Valhalla is not targeted for jdk13, and 
holding up changes for some future projects not really is the process
of OpenJDK, is it? 

> > > The change to the jtreg/vmTestbase/jit/t/t104/t104.gold file raises a
> > > question
> > > about how useful the information is,  developers should not need to know
> > > about "slot 1". 
> > Developers should work with class files with debug information and then
> > they would get the name printed. If exceptions are thrown in production
> > code, any information is helpful.  Should I just write "a local"?
> Go back to who you think is going to benefit from it, it seems targeted
> at a fairly novice developer, so exposing low level details may be more
> confusing that just giving a line number and NPE.
Especially on our improved NPE messages we always got good feedback
from the developers within SAP. And there are quite some experienced
people.  Any message requires some background to be helpful.  And I
like to get any information I can have in first place. Attaching the 
debugger often is a big overhead. E.g., I look at about 50 failing jtreg
tests every day, I don't want to get each into the debugger every time
in the setup where it was running to see what is wrong ... 

E.g., com/sun/java/swing/plaf/windows/AltFocusIssueTest.java
is failing in a headless environment with an NPE on this line:
SwingUtilities.invokeAndWait(() -> frame.dispose());
With this change it said "while trying to invoke the method 
javax.swing.JFrame.dispose(()V) of a null object loaded from static field 
AltFocusIssueTest.frame" and I figured it's the fact we run headless that 
makes the test fail without even looking at the code.  

Best regards,
  Goetz








From: Roger Riggs  
Sent: Dienstag, 12. Februar 2019 17:03
To: Lindenmaier, Goetz ; Java Core Libs 

Cc: hotspot-runtime-...@openjdk.java.net
Subject: Re: RFR(L): 8218628: Add detailed message to NullPointerException 
describing what is null.

Hi,
On 02/12/2019 08:14 AM, Lindenmaier, Goetz wrote:
Hi Roger, 

thanks for looking at my change!

A few higher level issues should be considered, though the details of
the webrev captured my immediate attention.

Is this the right feature
Yes!!
Maybe, that's what debuggers are for.



 and is this the right level of implementation (C++/native)?
See below.
Maybe 10 years ago, when native was the only solution.
Now there are tools to analyze bytecode in java.



 From a security perspective, adding field names to exceptions exposes
information to callers that they could not otherwise get and that breaks
encapsulation.
That needs to be evaluated.
I can not comment on that. How can I trigger an evaluation of this?
Who needs to evaluate this?

I think there are ways to make this easier to implement and be easier to
maintain and perform better.

NullPointerException:
28: unused import of Field
Removed

64: The lazyComputeMessage boolean should be inverted so it does not
require
    initialization, false is the default, anything else adds overhead.
    And it may not be needed.
Peter Levart proposed to initialize the message with a sentinel 

Re: RFR 8213031: (zipfs) Add support for POSIX file permissions

[Alan Bateman]

On 13/02/2019 22:26, Langer, Christoph wrote:

:

Also the "Zip" view of file attributes will need to be fleshed out more
(the view name for example).

I don't know if that's really necessary as the "Zip" view currently is internal 
to jdk.zips and I don't propose to export it.
Not exporting it is fine but are you planning to document 
"zip:storedPermissions"? The version I looked at did document 
"storedPermissions" which amounts to zipfs documenting an interface.






I'm not sure about using ${user.name} and "" as default.
Have you looked at using the zip file owner/group (or owner/owner on
Windows) as the default?

Hm, I guess for Unix that'd be ok. But how would I get to the owner of a file 
in Window's default file system?
The zip provider can use Files.getOwner and that can be used for group 
when PosixFileAttributeView isn't supported by the file system 
containing the zip file.



:
I'll try to address these points in a next iteration.

Great, I think the finial line on this feature isn't too far away.

-Alan

Re: RFR: JDK-8216558: Lookup.unreflectSetter(Field) fails to throw IllegalAccessException for final fields

[Adam Farley8]

Hi Mandy,

Apologies for the delay.

Could you review this cdiff as a proposal for the jtreg test?

Made sense to modify the existing test set for MethodHandle rather than 
add a new one.



test/jdk/java/lang/invoke/MethodHandlesGeneralTest.java

*** 830,844 
--- 830,860 
  public void testUnreflectSetter() throws Throwable {
 CodeCacheOverflowProcessor.runMHTest(this::testUnreflectSetter0);
  }
 
  public void testUnreflectSetter0() throws Throwable {
+ //First we test the newer "static final field" behaviour.
+ testUnreflectSetterStaticFinalField();
+ //Now we test the more proven functionality.
  if (CAN_SKIP_WORKING)  return;
  startTest("unreflectSetter");
  testSetter(TEST_UNREFLECT);
  }
 
+ static final int staticFinalField = 0;
+ public void testUnreflectSetterStaticFinalField() throws Throwable {
+ try {
+ Lookup l = MethodHandles.lookup();
+ Field f = 
MethodHandlesGeneralTest.class.getDeclaredField("staticFinalField");
+ f.setAccessible(true);
+ MethodHandle s = l.unreflectSetter(f);
+ throw new 
RuntimeException("MethodHandle.Lookup.unreflectSetter(Field) failed to 
throw IAE when Field is static and final.");
+ } catch (IllegalAccessException e) {
+ // IllegalAccessException expected.
+ }
+ }
+ 
  @Test
  public void testFindSetter() throws Throwable {
  CodeCacheOverflowProcessor.runMHTest(this::testFindSetter0);
  }
 




Best Regards

Adam Farley 
IBM Runtimes


Mandy Chung  wrote on 31/01/2019 18:58:25:

> From: Mandy Chung 
> To: Adam Farley8 
> Cc: core-libs-dev 
> Date: 31/01/2019 18:58
> Subject: Re: RFR: JDK-8216558: Lookup.unreflectSetter(Field) fails 
> to throw IllegalAccessException for final fields
> 
> 
> 
> On 1/31/19 4:18 AM, Adam Farley8 wrote:
> > Hi Mandy,
> > 
> > I've made the changes to the webrev and uploaded the corrected 
version.
> > 
> > I've also uploaded the zips in the csr and bug.
> > 
> > As for the test, I have mentioned that the test is attached to the 
bug. 
> > I attached the same test to the CSR.
> 
> I did look at that but it's not a jtreg regression test.  I assume
> you planned to convert it to jtreg test.  Anyway, the test attached
> in the bug report is the reproducer of the bug.
> 
> It would help the reviewer to include the regression test in
> the webrev along with the fix unless a test is not necessary.
> The OpenJDK developer guide gives a pretty good overview on the
> process [1] and please do ask if you have any question.
> 
> https://urldefense.proofpoint.com/v2/url?
> u=http-3A__openjdk.java.net_guide_changePlanning.html-23bug=DwIC-
> g=jf_iaSHvJObTbx-siA1ZOg=P5m8KWUXJf-
> 
CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho=uZYDsYv1Pe21YaYk_fuEwKg7Cc5a9sdsiGMPjDqgT5k=mvs_zaoLuTOUkmdByobfKcpvovlTEiHtNcz-
> UMc3d70=
> 
> > Are you saying that the test supplied is not a suitable test?
> 
> TestStaticFinal.java is a standalone test.  It needs to convert
> to a jtreg regression test.
> 
> The regression should throw an exception if Lookup::unreflectSetter
> returns MH on a static final field with and without accessible flag
> set.  However TestStaticFinal.java also exits with 0.  The regression
> test should pass with your bug and fail without the fix (throwing
> an exception).
> 
> This test verifies unreflectSetter on a static final field.  I
> suggest it also includes a case with instance final field with
> and without accessible flag set to show the exception for
> unreflectSetter what case allows setting final fields.
> 
> Mandy
> [1] https://urldefense.proofpoint.com/v2/url?
> u=http-3A__openjdk.java.net_guide_changePlanning.html-23bug=DwIC-
> g=jf_iaSHvJObTbx-siA1ZOg=P5m8KWUXJf-
> 
CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho=uZYDsYv1Pe21YaYk_fuEwKg7Cc5a9sdsiGMPjDqgT5k=mvs_zaoLuTOUkmdByobfKcpvovlTEiHtNcz-
> UMc3d70=
>  P.S. should be updated to refer to CSR that replaced CCC.
> 

Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 
741598. 
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU