Re: RFR [16/java.xml] 8248348: Regression caused by the update to BCEL 6.0
True, It's unusual (refer to some more details in the previous email). It's an unusual patch over an unusual design. IdentifyHashMap might have been a good choice for the mutable types in this issue. I'm not sure about other types as the BCEL process does involve removing items from the HashSet with a cloned copy. Using HashSet and maybe the larger architectural design surrounding it might not have been the best. But that's up to the BCEL engineers to decide. For the JDK, BCEL has been as stable as it is. I believe only small (none-API/archtectural) patches will be considered for the JDK to function properly. Regards, Joe On 6/27/2020 4:02 AM, Bernd Eckenfels wrote: It does not sound aligned if you allow to change the op code which is compared in equal() but remember the hashcode. But I guess it would be best to use a IdendityHashMap if you care for the objects. In that case you can implement hashcode uncached and aligned with equals. (And not use it) Gruss Bernd -- http://bernd.eckenfels.net *Von:* Joe Wang *Gesendet:* Friday, June 26, 2020 8:29:41 AM *An:* Bernd Eckenfels ; Core-Libs-Dev *Betreff:* Re: RFR [16/java.xml] 8248348: Regression caused by the update to BCEL 6.0 On 6/25/2020 5:14 PM, Bernd Eckenfels wrote: > Hello, > > What is the advantage of having such a narrow hashcode value space compared to the built in idendity hashcode? Would stocking to the object idendity not only reduce the footprint, but also make hash lookups faster? Without the unclear relationship to the op code? It's a general contract to override hashCode() when equals() is overridden, although due to the hacky nature the later was implemented and also the narrow scope it was applied in the (BCEL) code it didn't provide more than the Object identity. Still, it's right to keep the hashCode() method along with equals(), would have allowed all other subtypes to behave correctly, and beneficial to others who may read the code (who might wonder why it's missing). It's a bit improvement over no hashCode(). -Joe > > Gruss > Bernd > > > -- > http://bernd.eckenfels.net > > Von: core-libs-dev im Auftrag von Joe Wang > Gesendet: Friday, June 26, 2020 1:53:08 AM > An: Core-Libs-Dev > Betreff: RFR [16/java.xml] 8248348: Regression caused by the update to BCEL 6.0 > > Hi, > > Please review a fix to a BCEL regression. At issue was the addition of > hashCode() method to Instruction.java in BCEL 6.0. This hashCode() > method was implemented to return the instruction's opcode that > unfortunately is mutable. The problem hasn't showed up until the code > path led to calls to remove from a HashSet a target that has been > changed since it was added to the HashSet. The proposed patch is to make > the hashCode final/immutable. > > This patch implies that a target object is considered the same one even > if its field values may have been changed. It therefore may not be > appropriate in other situations (or may cause problems). However, since > it had always had no hashCode() override before BCEL 6.0, thereby > relying on Object's identity hash code, its use case has been limited > and time-tested. It can benefit from this patch in that it provides the > same function as Object's hash code, and then serves as a reminder (to > any who might read the code) how it was used (objects considered to be > the same over the course as far as the hashCode is concerned). > > JBS: https://bugs.openjdk.java.net/browse/JDK-8248348 > webrevs: http://cr.openjdk.java.net/~joehw/jdk16/8248348/webrev/ > > Thanks, > Joe >
Re: RFR [16/java.xml] 8248348: Regression caused by the update to BCEL 6.0
On 6/26/2020 12:46 PM, Stuart Marks wrote: On 6/25/20 4:53 PM, Joe Wang wrote: Please review a fix to a BCEL regression. At issue was the addition of hashCode() method to Instruction.java in BCEL 6.0. This hashCode() method was implemented to return the instruction's opcode that unfortunately is mutable. The problem hasn't showed up until the code path led to calls to remove from a HashSet a target that has been changed since it was added to the HashSet. The proposed patch is to make the hashCode final/immutable. This patch implies that a target object is considered the same one even if its field values may have been changed. It therefore may not be appropriate in other situations (or may cause problems). However, since it had always had no hashCode() override before BCEL 6.0, thereby relying on Object's identity hash code, its use case has been limited and time-tested. It can benefit from this patch in that it provides the same function as Object's hash code, and then serves as a reminder (to any who might read the code) how it was used (objects considered to be the same over the course as far as the hashCode is concerned). JBS: https://bugs.openjdk.java.net/browse/JDK-8248348 webrevs: http://cr.openjdk.java.net/~joehw/jdk16/8248348/webrev/ Hi Joe, This fix seems vaguely wrong, but in the context of what I understand it's doing, it seems that BCEL or the surrounding JAXP code is even more wrong. :-) The patch is surely unusual, because of this unusual case. As far as which one is more wrong, I believe the burden is mostly on BCEL although JAXP might share a bit, at the least it exposed the issue. From what I understand, this bugfix was prompted by a case where an Instruction was added to a HashSet. The Instruction was then mutated while in the HashSet, and this resulted in it not being removed from the HashSet in the future, when it was necessary to do so, thus leaving a stale entry. How could this have possibly worked?!? It works in most majority of the cases, although I think the BCEL developers admitted they had limited tests. This is because while Instruction allows mutation, all but one type of instructions actually do, and the mutated one don't get removed from the HashSet in a normal build-up process. But XML applications can go berserk sometimes, and in this case resulted in a method exceeding the max method size. When that happens, a rebuild process involved many copies/clones, resetting targeters, the later included removing targeters (among them the mutated ones) from the HashSet. Given that such a big stylesheet is unusual, it makes it an edge case. At least, we haven't received any error reports from the 20% who adopted Java 11. I'm speculating a bit, but I could imagine code like this: var inst = new Instruction(ORIG_OPCODE); var set = new HashSet(); set.add(inst); ... inst.setOpcode(NEW_OPCODE); // (1) mutates inst while in HashSet! ... set.remove(inst); // (2) In the current version, the hashCode() is simply the opcode, and equals() compares opcodes (and possibly other things). This is correct from the standpoint of Instruction itself, as equal items have the same hashcode. However, the above code fails because at line (2), the remove() method looks in the "wrong" hash bucket and can't find the instruction, so it does nothing. Yes, it will be correct for the majority cases where opcodes don't change. Also note that HashSet checks hashcode, and then the key's identity before key.equals, which is why this patch works. When the build process calls on an instruction to dispose itself, it removes itself from the targeter HashSet. But while it is itself, if the hashcode has changed, it won't even find itself! (sort of lost itself) (that's too many "itself") The patch changes the class to compute the hashcode at construction time, and makes the hashcode constant, so it "fixes" the above code, since the remove() method will look in the same bucket and find the desired instance. But now Instruction breaks the hashCode/equals contract, since it's possible for two instances to be equal but to have different hashcodes. (Consider an Instruction that was created with NEW_OPCODE. It can be equal to inst, but its hashcode will differ.) It would break the hashCode/equals contract if it was implemented normally. But this is unusual, the equals implementation was nothing but normal: for branch instructions, it always returns false! Thus, this unusual hashCode implementation will not break the contract. It seems to me that this patch is compensating for broken code elsewhere in BCEL or JAXP by adding more brokenness. It could be that is ok in this narrow usage context. That is, as far as we know, this "shaded" copy of BCEL is used *only* by JAXP and not used in general, and it may be that the particular ways that JAXP uses BCEL aren't broken (and are indeed fixed) by this change.
Re: RFR: 8248059: [macos] EmptyFolderPackageTest.java failed "hdiutil: create failed - No child processes"
looks good /Andy On 6/26/2020 9:22 PM, alexander.matv...@oracle.com wrote: Please review the jpackage fix for bug [1] at [2]. Added fallback for creating DMG if original approach fails. In original approach DMG will be created by providing app image to hdiutil. It was noticed that helper sub-processes run by hdiutil to copy app image sometimes crashed or failed during automated testing. Fallback approach will create empty DMG and copy files manually. It was observed that in this case hdiutil does not run sub-process that caused tests to fail, so hopefully this workaround will fix issue. Both cases produces same working DMG. Tests which failed due to this issue was removed from ProblemList. [1] https://bugs.openjdk.java.net/browse/JDK-8248059 [2] http://cr.openjdk.java.net/~almatvee/8248059/webrev.00/ Thanks, Alexander
Re: RFR: JDK-8248254: jpackage fails if app module is in external runtime
Looks good. /Andy On 6/26/2020 7:30 PM, Alexey Semenyuk wrote: Please review fix [2] for jpackage bug [1]. Fix jpackage code to be able to locate app module if it is linked in external runtime. The suggested fix only verifies if app module exists in external runtime. It doesn't extract version and main class from modules in exetrnal runtime. To address this shortcoming [3] CR was filed. Corresponding jtreg created and added to problem list. - Alexey [1] https://bugs.openjdk.java.net/browse/JDK-8248254 [2] http://cr.openjdk.java.net/~asemenyuk/8248254/webrev.00 [3] https://bugs.openjdk.java.net/browse/JDK-8248418
Re: Explicitly constructed NPE is showing (wrong) message
The message from this sender included one or more files
which could not be scanned for virus detection; do not
open these files unless you are certain of the sender's intent.
--
Hi,
I‘ll have a look on Monday. Thanks for reporting any analyzing it so far!
Best regards,
Götz
> Am 27.06.2020 um 14:44 schrieb Remi Forax :
>
> - Mail original -
>> De: "Christoph Dreis"
>> À: "hotspot-runtime-dev"
>> Envoyé: Samedi 27 Juin 2020 11:54:19
>> Objet: Explicitly constructed NPE is showing (wrong) message
>
>> Hi,
>
> Hi Christoph,
>
>>
>> I hope this is the correct mailing list.
>>
>> The latest changes on JDK-8233014 to enable
>> ShowCodeDetailsInExceptionMessages
>> by default have uncovered a likely bug (I think).
>>
>> To my understanding explicitly created NullPointerExceptions should not print
>> any message.
>> With the following example:
>>
>> public class Main {
>>public static void main(String[] args) {
>>NullPointerException ex = new NullPointerException();
>>Throwable throwable = ex.fillInStackTrace();
>>System.out.println(throwable);
>>}
>> }
>>
>> I see the following output though:
>>
>> java.lang.NullPointerException: Cannot invoke
>> "java.lang.NullPointerException.fillInStackTrace()" (on slot 0) because "ex"
>> is
>> null
>>
>> Which obviously is not really true.
>> I was debugging the thing (hence the additional "on slot 0") output, but
>> couldn't find the error so far.
>>
>> I'm suspecting the error to be somewhere around bytecodeUtils.cpp 1155:
>>
>> if (name != vmSymbols::object_initializer_name()) {
>> int type_index = cp->signature_ref_index_at(name_and_type_index);
>> Symbol* signature = cp->symbol_at(type_index);
>> // The 'this' parameter was null. Return the slot of it.
>> return ArgumentSizeComputer(signature).size();
>> } else {
>> return NPE_EXPLICIT_CONSTRUCTED;
>> }
>>
>> I initially hoped to find a fix for it directly, but I would probably need a
>> bit
>> more time for it, so I decided to report it.
>> Is this a bug even or am I chasing ghosts here? In case it is, I would be
>> happy
>> about a mentioning somewhere in an eventual commit.
>
> I see the issue.
> The idea of the algorithm is to use the backtrace stored in the exception to
> try to find where the NPE occurs, here by calling fillStackTrace()
> emplicitly, you are changing the backtrace so getExtendedNPEMessage now use
> the new backtrace reporting the error in main() :(
>
> One way to fix the issue is to record if fillInStackTrace is called more than
> once (by having two sentinels by example) and to not call
> getExtendedNPEMessage if it's the case. Obviously the new added sentinel has
> to work with the serialization.
>
>>
>> Cheers,
>> Christoph
>
> regards,
> Rémi
Re: Explicitly constructed NPE is showing (wrong) message
- Mail original -
> De: "Christoph Dreis"
> À: "hotspot-runtime-dev"
> Envoyé: Samedi 27 Juin 2020 11:54:19
> Objet: Explicitly constructed NPE is showing (wrong) message
> Hi,
Hi Christoph,
>
> I hope this is the correct mailing list.
>
> The latest changes on JDK-8233014 to enable ShowCodeDetailsInExceptionMessages
> by default have uncovered a likely bug (I think).
>
> To my understanding explicitly created NullPointerExceptions should not print
> any message.
> With the following example:
>
> public class Main {
> public static void main(String[] args) {
> NullPointerException ex = new NullPointerException();
> Throwable throwable = ex.fillInStackTrace();
> System.out.println(throwable);
> }
> }
>
> I see the following output though:
>
> java.lang.NullPointerException: Cannot invoke
> "java.lang.NullPointerException.fillInStackTrace()" (on slot 0) because "ex"
> is
> null
>
> Which obviously is not really true.
> I was debugging the thing (hence the additional "on slot 0") output, but
> couldn't find the error so far.
>
> I'm suspecting the error to be somewhere around bytecodeUtils.cpp 1155:
>
>if (name != vmSymbols::object_initializer_name()) {
> int type_index = cp->signature_ref_index_at(name_and_type_index);
> Symbol* signature = cp->symbol_at(type_index);
> // The 'this' parameter was null. Return the slot of it.
> return ArgumentSizeComputer(signature).size();
>} else {
> return NPE_EXPLICIT_CONSTRUCTED;
>}
>
> I initially hoped to find a fix for it directly, but I would probably need a
> bit
> more time for it, so I decided to report it.
> Is this a bug even or am I chasing ghosts here? In case it is, I would be
> happy
> about a mentioning somewhere in an eventual commit.
I see the issue.
The idea of the algorithm is to use the backtrace stored in the exception to
try to find where the NPE occurs, here by calling fillStackTrace() emplicitly,
you are changing the backtrace so getExtendedNPEMessage now use the new
backtrace reporting the error in main() :(
One way to fix the issue is to record if fillInStackTrace is called more than
once (by having two sentinels by example) and to not call getExtendedNPEMessage
if it's the case. Obviously the new added sentinel has to work with the
serialization.
>
> Cheers,
> Christoph
regards,
Rémi
Re: RFR [16/java.xml] 8248348: Regression caused by the update to BCEL 6.0
It does not sound aligned if you allow to change the op code which is compared in equal() but remember the hashcode. But I guess it would be best to use a IdendityHashMap if you care for the objects. In that case you can implement hashcode uncached and aligned with equals. (And not use it) Gruss Bernd -- http://bernd.eckenfels.net Von: Joe Wang Gesendet: Friday, June 26, 2020 8:29:41 AM An: Bernd Eckenfels ; Core-Libs-Dev Betreff: Re: RFR [16/java.xml] 8248348: Regression caused by the update to BCEL 6.0 On 6/25/2020 5:14 PM, Bernd Eckenfels wrote: > Hello, > > What is the advantage of having such a narrow hashcode value space compared > to the built in idendity hashcode? Would stocking to the object idendity not > only reduce the footprint, but also make hash lookups faster? Without the > unclear relationship to the op code? It's a general contract to override hashCode() when equals() is overridden, although due to the hacky nature the later was implemented and also the narrow scope it was applied in the (BCEL) code it didn't provide more than the Object identity. Still, it's right to keep the hashCode() method along with equals(), would have allowed all other subtypes to behave correctly, and beneficial to others who may read the code (who might wonder why it's missing). It's a bit improvement over no hashCode(). -Joe > > Gruss > Bernd > > > -- > http://bernd.eckenfels.net > > Von: core-libs-dev im Auftrag von Joe > Wang > Gesendet: Friday, June 26, 2020 1:53:08 AM > An: Core-Libs-Dev > Betreff: RFR [16/java.xml] 8248348: Regression caused by the update to BCEL > 6.0 > > Hi, > > Please review a fix to a BCEL regression. At issue was the addition of > hashCode() method to Instruction.java in BCEL 6.0. This hashCode() > method was implemented to return the instruction's opcode that > unfortunately is mutable. The problem hasn't showed up until the code > path led to calls to remove from a HashSet a target that has been > changed since it was added to the HashSet. The proposed patch is to make > the hashCode final/immutable. > > This patch implies that a target object is considered the same one even > if its field values may have been changed. It therefore may not be > appropriate in other situations (or may cause problems). However, since > it had always had no hashCode() override before BCEL 6.0, thereby > relying on Object's identity hash code, its use case has been limited > and time-tested. It can benefit from this patch in that it provides the > same function as Object's hash code, and then serves as a reminder (to > any who might read the code) how it was used (objects considered to be > the same over the course as far as the hashCode is concerned). > > JBS: https://bugs.openjdk.java.net/browse/JDK-8248348 > webrevs: http://cr.openjdk.java.net/~joehw/jdk16/8248348/webrev/ > > Thanks, > Joe >
