Hi Eric,
Please revert the change to j.l.r.Modifer. The fix can be pushed with
just that modification; however, I strongly recommend also removing the
"here is everything that can go wrong" list from j.l.r.Executable. Core
reflection generally doesn't delve into such details in the main-line
API docs and calling the details out in the exception type should be
sufficient.
Cheers,
-Joe
On 10/2/2013 11:55 AM, Eric McCorkle wrote:
I've updated the test, switched to an in-memory class loader, and added
a test case. Please review.
On 10/01/13 16:27, Eric McCorkle wrote:
On 10/01/13 02:41, Joe Darcy wrote:
(Suggested changes have been applied)
I think the test is acceptable as-is, but an RFE could be filed for some
refactoring (having each bad class be represented as a diff from a base
byte[], avoiding sending the bytes through the file system).
Better yet: I've filed an RFE for a framework for generating bad class
files.
Webrev has been updated, please review:
http://cr.openjdk.java.net/~emc/8020981/
Cheers,
-Joe
On 9/24/2013 12:15 PM, Eric McCorkle wrote:
Webrev updated to address these issues.
On 09/24/13 07:51, Joel Borggren-Franck wrote:
364 try {
365 tmp = getParameters0();
366 } catch(IllegalArgumentException e) {
367 // Rethrow ClassFormatErrors
368 throw new MalformedParametersException("Invalid
constant pool index");
369 }
What is causing the IAE? Have you considered having
MalformedParametersExcepton taking a Throwable cause and having the IAE
as cause?
The IAE comes from hotspot itself. There is a standard constant pool
index verifying function that throws IAE if given a bad index.
On 2013-09-19, Eric McCorkle wrote:
The webrev has been updated with Joe's comments addressed.
On 09/19/13 00:11, David Holmes wrote:
On 19/09/2013 9:59 AM, Eric McCorkle wrote:
This still needs to be reviewed.
You seem to have ignored Joe's comments regarding the change to
Modifier
being incorrect.
David
On 09/16/13 14:50, Eric McCorkle wrote:
I pulled the class files into byte array constants, as a temporary
measure until a viable method for testing bad class files is
developed.
The webrev has been refreshed. The class files will be taken out
before
I push.
http://cr.openjdk.java.net/~emc/8020981/
On 09/13/13 20:48, Joe Darcy wrote:
To avoid storing binaries in Hg, you could try something like:
* uuencode / ascii armor the class file
* convert to byte array in the test
* load classes from byte array
-Joe
On 09/13/2013 11:54 AM, Eric McCorkle wrote:
I did it by hand with emacs.
I would really rather tackle the bad class files for testing issue
once
and for all, the Right Way (tm). But with ZBB looming, now is
not the
time to do it.
Hence, I have created this task
https://bugs.openjdk.java.net/browse/JDK-8024674
I also just created this one:
https://bugs.openjdk.java.net/browse/JDK-8024812
On 09/13/13 13:54, Peter Levart wrote:
Hi Eric,
How did you create those class files? By hand using a HEX
editor? Did
you create a program that patched the original class file? If the
later
is the case, you could pack that patching logic inside a custom
ClassLoader...
To hacky? Dependent on future changes of javac? At least the "bad
name"
patching could be performed trivially and reliably, I think:
search and
replace with same-length string.
Regards, Peter
On 09/13/2013 07:35 PM, Eric McCorkle wrote:
Ugh. Byte arrays of class file data is really a horrible
solution.
I have already filed a task for test development post ZBB to
develop a
solution for generating bad class files. I'd prefer to file a
follow-up
to this to add the bad class file tests when that's done.
On 09/13/13 10:55, Joel Borggrén-Franck wrote:
I think the right thing to do is to include the original
compiling
source in a comment, together with a comment on how you modify
them, and then the result as a byte array.
IIRC I have seen test of that kind before somewhere in our
repo.
cheers
/Joel
On Sep 13, 2013, at 4:49 PM, Eric McCorkle
<eric.mccor...@oracle.com> wrote:
There is no simple means of generating bad class files for
testing.
This is a huge deficiency in our testing abilities.
If these class files shouldn't go in, then I'm left with no
choice
but
to check in no test for this patch.
However, anyone can run the test I've provided with the class
files and
see that it works.
On 09/13/13 09:55, Joel Borggrén-Franck wrote:
Hi Eric,
IIRC we don't check in classfiles into the repo.
I'm not sure how we handle testing of broken class-files
in jdk,
but ASM might be an option, or storing the class file as an
embedded byte array in the test.
cheers
/Joel
On Sep 13, 2013, at 3:40 PM, Eric McCorkle
<eric.mccor...@oracle.com> wrote:
A new webrev is posted (and crucible updated), which
actually
validates
parameter names correctly. Apologies for the last one.
On 09/12/13 16:02, Eric McCorkle wrote:
Hello,
Please review this patch, which implements correct
behavior for
the
Parameter Reflection API in the case of malformed class
files.
The bug report is here:
https://bugs.openjdk.java.net/browse/JDK-8020981
The webrev is here:
http://cr.openjdk.java.net/~emc/8020981/
This review is also on crucible. The ID is CR-JDK8TL-182.
Thanks,
Eric
<eric_mccorkle.vcf>
