8272058: 25 Null pointer dereference defect groups in 4 files #51
Conversation
|
|
|
The Parfait complaint at the root of this issue is: MacroAssembler::target_addr_for_insn |
|
@dcubed-ojdk The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
|
/label add hotspot-runtime |
|
@dcubed-ojdk |
|
/label remove hotspot |
|
@dcubed-ojdk |
Webrevs
|
|
Hi Dan, In terms of the refactoring and addressing the warning this seems reasonable. But in terms of semantics this claims that all the remaining callsites for Thanks, |
|
@dholmes-ora - Thanks for taking a look. I did take the time to look at all the Having @theRealAph chime in here would be wonderful! |
|
@gerard-ziemski - When you have a chance, could you take a look at this one? |
There was a problem hiding this comment.
I don't understand the underlying code, but from a big picture point of view, splitting the call into "null not allowed" and "null allowed" makes sense. If the testing came back fine, then that's the best we can do, without asking someone who wrote this code to chime in.
Just one question: why do we need the "null allowed" version here?
void NativeMovRegMem::verify() {
#ifdef ASSERT
address dest = MacroAssembler::target_addr_for_insn_allow_null_ret(instruction_address());
#endif
}
|
@gerard-ziemski - Thanks for the review. According to "git blame", the original author is: 9c458de hotspot/src/cpu/aarch64/vm/macroAssembler_aarch64.cpp (Andrew Haley 2015-01-20 11:34:17 -0800 211) address MacroAssembler::target_addr_for_insn(address insn_addr, unsigned insn) { which I believe is why @dholmes-ora pinged @theRealAph on this PR... As for this function: I did not see any harm in converting to |
As long as parfait doesn't complain here - I didn't see where we checked for NULL explicitly in this case. I assume that parfait skips the ASSERT parts? |
Parfait does not complain anymore. The key about this call is that we don't de-reference |
|
Perhaps Boris Ulasevich boris.ulasevich@bell-sw.com could review? |
|
Mailing list message from David Holmes on hotspot-runtime-dev: Adding Boris Ulasevich <boris.ulasevich at bell-sw.com> to cc list :) On 24/12/2021 12:42 pm, David Holmes wrote: |
| } else { | ||
| ShouldNotReachHere(); | ||
| } | ||
| return address(((uint64_t)insn_addr + (offset << 2))); | ||
| } | ||
|
|
||
| address MacroAssembler::target_addr_for_insn_allow_null_ret(address insn_addr, unsigned insn) { | ||
| if (Instruction_aarch64::extract(insn, 31, 22) == 0b1011100101 && |
There was a problem hiding this comment.
Consider replacing raw constants check with NativeInstruction::is_ldrw_to_zr function call which performs the same check.
There was a problem hiding this comment.
Good catch. There are actually two uses of the raw constants in
macroAssembler_aarch64.cpp and I've cleaned up both.
There was a problem hiding this comment.
This was a nice refactoring.
| @@ -283,15 +283,20 @@ address MacroAssembler::target_addr_for_insn(address insn_addr, unsigned insn) { | |||
| return address(uint64_t(Instruction_aarch64::extract(insns[0], 20, 5)) | |||
| + (uint64_t(Instruction_aarch64::extract(insns[1], 20, 5)) << 16) | |||
| + (uint64_t(Instruction_aarch64::extract(insns[2], 20, 5)) << 32)); | |||
| } else if (Instruction_aarch64::extract(insn, 31, 22) == 0b1011100101 && | |||
There was a problem hiding this comment.
It must be a check for safePoint poll: ldrw zr, polling_page. Technically, the target address for this instruction is zero and the function is correct. I am not sure Parfait is right in his complaints. After all, does this code ever works? That is, if you remove it, will the SholdNotReachHere below fire?
There was a problem hiding this comment.
I don't think the safepoint poll is coming thru this code path. I ran this fix
through Mach5 Tier[1-6] and did not see any failures caused by hitting
a ShouldNotReachHere() call. It's hard to imagine that Tier[1-6] testing
wouldn't exercise the safepoint polling code paths.
I've also verified that the Parfait tool no longer issues the complaints.
I will poke around and see if I can determine what the safepoint polling
is doing.
There was a problem hiding this comment.
@bulasevich - I've made another pass through the code and checked the
calls to MacroAssembler::target_addr_for_insn() and I don't see any of
those calls in areas related to safepoint polling. From the other POV, I
took a look at the aarch64 safepoint polling code and I don't see a way
into MacroAssembler::target_addr_for_insn() .
It is entirely possible that I'm missing something here and would appreciate
it if you could sanity check my conclusions here.
There was a problem hiding this comment.
I believe this code is dead, in both MacroAssembler::pd_patch_instruction() and MacroAssembler::target_addr_for_insn(), since the poll read is PIC, we are not patching and should not need to acquire any target to update.
The associated fix_relocation_after_move() will not invoke any of the above on the poll read since the maybe_cpool_ref() condition is used:
void poll_Relocation::fix_relocation_after_move(const CodeBuffer* src, CodeBuffer* dest) {
if (NativeInstruction::maybe_cpool_ref(addr())) {
address old_addr = old_addr_for(addr(), src, dest);
MacroAssembler::pd_patch_instruction(addr(), MacroAssembler::target_addr_for_insn(old_addr));
}
}
Should be sufficient to simply remove these cases, possibly you could move an assert into the "unreachable" branch (for "precision").
} else {
assert(!NativeInstruction::is_ldrw_to_zr(<addr>), "Unexpected poll read");
ShouldNotReachHere();
}
(The code above in fix_relocation_after_move() might also be dead...)
Chip in @theRealAph ?
There was a problem hiding this comment.
@phedlin - Thanks for chiming in on this review thread.
There was a problem hiding this comment.
I wondered if target_addr_for_insn ever gets the ldrw_to_zr instruction.
Anyway, I am Ok to move it to target_addr_for_insn_allow_null_ret (target_addr_for_insn_or_null?).
Thank you.
There was a problem hiding this comment.
Since we moved to using per-thread safepoints, I don't think we'll ever see a reloc against a safepoint poll. I've just looked through the AArch64 back end, and as far as I can see the old safepoint code is gone.
There was a problem hiding this comment.
@bulasevich - I've made another pass through the code and checked the calls to
MacroAssembler::target_addr_for_insn()and I don't see any of those calls in areas related to safepoint polling. From the other POV, I took a look at the aarch64 safepoint polling code and I don't see a way intoMacroAssembler::target_addr_for_insn().It is entirely possible that I'm missing something here and would appreciate it if you could sanity check my conclusions here.
I think you're right. As far as I can see, this code is dead since
8253180: ZGC: Implementation of JEP 376: ZGC: Concurrent Thread-Stack Processing
which took out the code that generated safepoint relocs, but not the code here that relocated them.
There was a problem hiding this comment.
I've filed the following new RFE to track the dead compiler code:
JDK-8280283 dead compiler code found during the JDK-8272058 code review
https://bugs.openjdk.java.net/browse/JDK-8280283
|
Mach5 Tier[1-3] testing on v02 is clean (no failures). Update: Mach5 Tier[4-6] test on v02 is clean (1 known, unrelated client test failure). |
|
@bulasevich and/or @theRealAph - pinging for reviews... |
1 similar comment
|
@bulasevich and/or @theRealAph - pinging for reviews... |
|
@bulasevich - Please re-review this PR. The JDK18 RDP2 cut off is next week (01.20) and @theRealAph - Since I'm changing code that you integrated, I would prefer to hear from |
|
Mach5 Tier[1-3] on the latest bits had no failures. |
|
@dcubed-ojdk This change now passes all automated pre-integration checks. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been no new commits pushed to the
|
|
@coleenp - Thanks for the review and the approval. |
|
/cc hotspot-compiler |
|
@dean-long |
|
I'm glad you didn't have to change all the Got and Plt call sites that parfait was complaining about. It seems to be all dead code left over from AOT. |
| @@ -606,10 +606,15 @@ class MacroAssembler: public Assembler { | |||
| static bool uses_implicit_null_check(void* address); | |||
|
|
|||
| static address target_addr_for_insn(address insn_addr, unsigned insn); | |||
| static address target_addr_for_insn_allow_null_ret(address insn_addr, unsigned insn); | |||
| static address target_addr_for_insn(address insn_addr) { | |||
There was a problem hiding this comment.
The or_null suffix that we use in shared code reads a little better to me.
|
The change looks good for me (I'm not a reviewer). Thank you. |
|
@dean-long - Thanks for the review and for the rename suggestion. I'm making @bulasevich - Thanks for the re-review! @theRealAph - Thanks for chiming in on the review thread. |
|
This latest version has been tested with a Mach5 Tier1 job set. /integrate |
|
Going to push as commit f5de6fa. |
openjdk
bot
removed
ready
|
@dcubed-ojdk Pushed as commit f5de6fa. |
A small refactoring to resolve a Parfait complaint about the return value from
MacroAssembler::target_addr_for_insn(address insn_addr, unsigned insn)on AARCH64. The logic that supports returning
nullptras the target addr fora particular instruction is moved from
MacroAssembler::target_addr_for_insn(address insn_addr, unsigned insn)toMacroAssembler::target_addr_for_insn_allow_null_ret(address insn_addr, unsigned insn).A couple of
target_addr_for_insn()call sites that can tolerate anullptrareconverted to use
target_addr_for_insn_allow_null_ret().This fix has been tested with Mach5 Tier[1-3].
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk18 pull/51/head:pull/51$ git checkout pull/51Update a local copy of the PR:
$ git checkout pull/51$ git pull https://git.openjdk.java.net/jdk18 pull/51/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 51View PR using the GUI difftool:
$ git pr show -t 51Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk18/pull/51.diff