Re: [9] RFR 8074840: Resolve disabled warnings for libjli and libjli_static
Hi Mikael, Thank you so much for making these changes!. Looks generally good. ergo_i586.c: why ? MacOSX ? cmdtoargs.c: URK!. :-[ Thanks Kumar On 3/20/2015 11:02 AM, Mikael Vidstedt wrote: Please review the following change which fixes a number of native compilation warnings in files related to libjli. Bug: https://bugs.openjdk.java.net/browse/JDK-8074840 Webrev: http://cr.openjdk.java.net/~mikael/webrevs/8074840/webrev.01/webrev/ I built the code across all the OracleJDK platforms and there were no warnings on any of the platforms (in the files related to this change). I'm taking suggestions on specific tests to run. Some specifics: Unfortunately there is no intrinsic for cpuid in Solaris Studio, so I had to keep the inline asm code and the corresponding flag to disable the related warning (E_ASM_DISABLES_OPTIMIZATION). The alternative would be to move it out into a dedicated assembly file, but that seems like more trouble than it's worth given that the warning is harmless. I'm not entirely happy with the unsigned cast in parse_manifest.c:196, but unfortunately the windows prototype for read() takes an unsigned as its last argument, where All(tm) other platforms take a size_t. In this specific case 'len' will never be be more than END_MAXLEN = 0x + ENDHDR = 0x + 22 = 0x10015, meaning it will comfortably fit in an unsigned on the platforms we support. But if somebody has suggestions I'm all ears, doing the #ifdef dance is of course also an option. Note that the warnings were temporarily disabled as part of JDK-8074096 [1] until such time they could be fixed the proper way. Also note that this change supersedes the earlier change [2] Dmitry had out for review. The bug [3] corresponding to that webrev will be closed as a duplicate of this bug (JDK-8074839). Cheers, Mikael [1] https://bugs.openjdk.java.net/browse/JDK-8074096 [2] http://cr.openjdk.java.net/~dsamersoff/JDK-8073175/webrev.01/ [3] https://bugs.openjdk.java.net/browse/JDK-8073175
Re: RFR 8073056: Repeating annotations throws java.security.AccessControlException with a SecurityManager
On 03/28/2015 11:24 AM, Joel Borggrén-Franck wrote: Hi, On 27 Feb 2015, at 11:06, Peter Levart peter.lev...@gmail.com wrote: On 02/27/2015 10:27 AM, Joel Borggrén-Franck wrote: On 26 feb 2015, at 22:35, Peter Levart peter.lev...@gmail.com wrote: On 02/26/2015 10:27 PM, Peter Levart wrote: The m.setAccessible(true) for the methods is needed to access methods of non-public annotations, right? This call could be moved to AnnotationType constructor as there it will be performed only once per Method object. ...which will have the added benefit in that it will guarantee that only one MethodAccessor object per Method will ever be constructed instead of two… I don’t see this. setAccessible sets override in AccessibleObject, I don’t see a new MethodAccessor being generated here. My fault! I was mistakenly thinking of Field objects in the context of setAccessible(boolean). If you use a Field object in two modes it will create and retain two different FieldAccessors (because they are different implementations in case field is final). But I agree with you, and setting it as accessible in the AnnotationType constructor should arguably be more secure since then we know it isn’t shared since we just got our copy fresh from getDeclaredMethods(). That's a good point from maintainability perspective, yes, if someone some time decides to optimize the AnnotationType. I think it would be nice if AnnotationType documents that members() method returns Method objects that are pre-conditioned with setAccessible(true) and that users should not change this flag. I don’t want to do this in AnnotationType without a bigger overhaul that may be slightly incompatible and therefor should be 9 only. I do want to backport this fix to 8 however, so here is an alternative solution that should be safe and correct at the cost of extra allocation in the path for custom implementations of annotations (that should be rare). New webrev: http://cr.openjdk.java.net/~jfranck/8073056/webrev.02/ cheers /Joel Hi Joel, If you really must add new methods to Method, ReflectAccess, LangReflectAccess and ReflectionFactory then you could expose a method called invokeUnchecked that would skip access checks, so you don't have to copy the Method object just to get a one-time access to the possibly inaccessible method. But this is good as it is, since the code path is normally not used - it's there only for consistency's sake, right? Regards, Peter
Re: RFR 8073056: Repeating annotations throws java.security.AccessControlException with a SecurityManager
On 28 Mar 2015, at 13:42, Peter Levart peter.lev...@gmail.com wrote: On 03/28/2015 11:24 AM, Joel Borggrén-Franck wrote: Hi, On 27 Feb 2015, at 11:06, Peter Levart peter.lev...@gmail.com wrote: On 02/27/2015 10:27 AM, Joel Borggrén-Franck wrote: On 26 feb 2015, at 22:35, Peter Levart peter.lev...@gmail.com wrote: On 02/26/2015 10:27 PM, Peter Levart wrote: The m.setAccessible(true) for the methods is needed to access methods of non-public annotations, right? This call could be moved to AnnotationType constructor as there it will be performed only once per Method object. ...which will have the added benefit in that it will guarantee that only one MethodAccessor object per Method will ever be constructed instead of two… I don’t see this. setAccessible sets override in AccessibleObject, I don’t see a new MethodAccessor being generated here. My fault! I was mistakenly thinking of Field objects in the context of setAccessible(boolean). If you use a Field object in two modes it will create and retain two different FieldAccessors (because they are different implementations in case field is final). But I agree with you, and setting it as accessible in the AnnotationType constructor should arguably be more secure since then we know it isn’t shared since we just got our copy fresh from getDeclaredMethods(). That's a good point from maintainability perspective, yes, if someone some time decides to optimize the AnnotationType. I think it would be nice if AnnotationType documents that members() method returns Method objects that are pre-conditioned with setAccessible(true) and that users should not change this flag. I don’t want to do this in AnnotationType without a bigger overhaul that may be slightly incompatible and therefor should be 9 only. I do want to backport this fix to 8 however, so here is an alternative solution that should be safe and correct at the cost of extra allocation in the path for custom implementations of annotations (that should be rare). New webrev: http://cr.openjdk.java.net/~jfranck/8073056/webrev.02/ cheers /Joel Hi Joel, If you really must add new methods to Method, ReflectAccess, LangReflectAccess and ReflectionFactory then you could expose a method called invokeUnchecked that would skip access checks, so you don't have to copy the Method object just to get a one-time access to the possibly inaccessible method. IMO this would require quite a lot of investment validating all the possible ways it may be a security concern. I don’t think this is worth it given the very limited use, tough I find the idea interesting. But this is good as it is, since the code path is normally not used - it's there only for consistency's sake, right? Yes, that is my belief. I don’t know of any AnnotatedElements needing this except in testing, of course that isn’t to say there are none. cheers /Joel
Low hanging fruit in JBS for the Adoption Group Hackdays to tackle?
Hi all, We had a short discussion in Adoption Group about having its members triaging issues in JBS to identify low-hanging fruit for new OpenJDK developers to tackle. Dalibor sensibly suggested that each project/group was far more familiar with which issues would be appropriate and that we should simply list those recommended issues in the Adoption Wiki. So I'm starting by asking core-libs as I believe it covers the part of OpenJDK that is most accessible to new OpenJDK developers (please correct me if I'm wrong!). FYI - Currently London runs a hackday 1/month and several other user groups host more ad-hoc events. We're looking to expand this programme globally, but would like to firstly have a list of concrete JBS issue for people to tackle. Q: Do some members of this group have some low hanging fruit issues they'd like to nominate? Cheers, Martijn
Re: RFR 8073056: Repeating annotations throws java.security.AccessControlException with a SecurityManager
Hi, On 27 Feb 2015, at 11:06, Peter Levart peter.lev...@gmail.com wrote: On 02/27/2015 10:27 AM, Joel Borggrén-Franck wrote: On 26 feb 2015, at 22:35, Peter Levart peter.lev...@gmail.com wrote: On 02/26/2015 10:27 PM, Peter Levart wrote: The m.setAccessible(true) for the methods is needed to access methods of non-public annotations, right? This call could be moved to AnnotationType constructor as there it will be performed only once per Method object. ...which will have the added benefit in that it will guarantee that only one MethodAccessor object per Method will ever be constructed instead of two… I don’t see this. setAccessible sets override in AccessibleObject, I don’t see a new MethodAccessor being generated here. My fault! I was mistakenly thinking of Field objects in the context of setAccessible(boolean). If you use a Field object in two modes it will create and retain two different FieldAccessors (because they are different implementations in case field is final). But I agree with you, and setting it as accessible in the AnnotationType constructor should arguably be more secure since then we know it isn’t shared since we just got our copy fresh from getDeclaredMethods(). That's a good point from maintainability perspective, yes, if someone some time decides to optimize the AnnotationType. I think it would be nice if AnnotationType documents that members() method returns Method objects that are pre-conditioned with setAccessible(true) and that users should not change this flag. I don’t want to do this in AnnotationType without a bigger overhaul that may be slightly incompatible and therefor should be 9 only. I do want to backport this fix to 8 however, so here is an alternative solution that should be safe and correct at the cost of extra allocation in the path for custom implementations of annotations (that should be rare). New webrev: http://cr.openjdk.java.net/~jfranck/8073056/webrev.02/ cheers /Joel
Re: RFR: 8071571: Move substring of same string to slow path
On 03/27/2015 10:59 PM, Martin Buchholz wrote:
[...]
OTOH, if you touch code that uses the denigrated
char foo[]
please change it to
char[] foo
This is yet another of those global changes to the entire jdk sources that
nobody ever gets around to.
on the same subject, the Java grammar allows this code to compile
class A {
int foo() [] {
return null;
}
}
cheers,
Rémi
On Fri, Mar 27, 2015 at 2:51 PM, Lev Priima lev.pri...@oracle.com wrote:
Updated and put some more final in other(not @Deprecated) methods:
http://cr.openjdk.java.net/~lpriima/8071571/2/
Lev
On 03/27/2015 11:50 PM, Martin Buchholz wrote:
On Fri, Mar 27, 2015 at 1:49 PM, Lev Priima lev.pri...@oracle.com wrote:
Martin,
You mean it should be like this:
char[] val = value;/* avoid getfield opcode */
int end = val.length;
?
Yes.
(although I personally like to write it like this:
final char[] value = this.value;
int end = value.length;
Re: RFR: 8071571: Move substring of same string to slow path
On 03/27/2015 11:01 PM, Martin Buchholz wrote: On Fri, Mar 27, 2015 at 2:57 PM, Roger Riggs roger.ri...@oracle.com wrote: Hi, @Martin: does the final have some functional reason to be present? I see it just bulks up the source, reducing readability. If it is useful to improve performance then fine. If you use the idiom final Type foo = this.foo; and leave out the final, there is a risk of a maintainer assigning to foo and introducing a bug. Maybe I should stop, since no one seems to understand it? I really dislike this style, If you put final on foo you should also put final on len and on all parameters because following your idea a maintainer may introduce a bug. At that point, you have clutter the whole code with final everywhere for a bug that happens rarely. This kind of bugs (modifying a field that was loaded in a local variable) should be catch by a lint tool (or/and IDEs) and not by asking every developers to add final (randomly). And final on a local variable has no impact on performance because it doesn't appear in the generated bytecode. Historical note: final was introduce with jdk 1.1 to make explicit that the semantics of capturing a local variable in an anonymous class was by copying the local variable value and not by capturing the variable itself like in Lisp (Ruby, JavaScript, etc.). Since Java 8, you don't need to declare local variables 'final' anymore because the compiler enforces that captured variables are effectively final. The other problem of final is that while final on a local variables is futile, it's a very important concept on fields, and a awful lot of Java devs doesn't make the difference between a field and a local variable (both are variables right ?). And every time a discussion about final on local variable sprung, I'm quite sure that we (and i) just add more confusion :( regards, Rémi
