Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Forbid creation of non-faulting null-check nodes. #77078

Merged
merged 12 commits into from
Jan 28, 2023
Merged

Forbid creation of non-faulting null-check nodes. #77078

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
d5e8d6a
Forbid creation of non-faulting null-check nodes.
JulieLeeMSFT Oct 15, 2022
0a17d91
Skip null check if op1 cannot be null.
JulieLeeMSFT Oct 20, 2022
37d8f34
x86 bug fix for Skip gtChangeOperToNullCheck if it cannot be null.
JulieLeeMSFT Oct 22, 2022
6434f20
Fix Arm64 AVE: Do not lower indir when NOP.
JulieLeeMSFT Oct 27, 2022
4c1fddd
formatting fix.
JulieLeeMSFT Oct 27, 2022
98b6b90
Arm64 regression bug fix that removes const bool for a variable.
JulieLeeMSFT Oct 27, 2022
d9bb584
Preserve const and shorten expression.
JulieLeeMSFT Dec 29, 2022
6bb0d26
Fix format.
JulieLeeMSFT Dec 29, 2022
58a9b98
Merge branch 'main' into 44227
JulieLeeMSFT Dec 29, 2022
7193ca3
Addressed code review feedback
JulieLeeMSFT Jan 21, 2023
c984a91
Forbid creation of non-faulting nullcheck node: Addressed feedback
JulieLeeMSFT Jan 26, 2023
e20a756
Forbid creation of non-faulting null-check nodes: Remove duplicate co…
JulieLeeMSFT Jan 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/coreclr/jit/compiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9953,6 +9953,7 @@ void Compiler::gtChangeOperToNullCheck(GenTree* tree, BasicBlock* block)
assert(tree->OperIs(GT_FIELD, GT_IND, GT_OBJ, GT_BLK));
tree->ChangeOper(GT_NULLCHECK);
tree->ChangeType(gtTypeForNullCheck(tree));
assert(fgAddrCouldBeNull(tree->AsUnOp()->gtGetOp1()));
JulieLeeMSFT marked this conversation as resolved.
Show resolved Hide resolved
JulieLeeMSFT marked this conversation as resolved.
Show resolved Hide resolved
JulieLeeMSFT marked this conversation as resolved.
Show resolved Hide resolved
block->bbFlags |= BBF_HAS_NULLCHECK;
optMethodFlags |= OMF_HAS_NULLCHECK;
}
Expand Down
13 changes: 11 additions & 2 deletions src/coreclr/jit/gentree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16050,8 +16050,17 @@ void Compiler::gtExtractSideEffList(GenTree* expr,
Append(node);
if (node->OperIsBlk() && !node->OperIsStoreBlk())
{
JITDUMP("Replace an unused OBJ/BLK node [%06d] with a NULLCHECK\n", dspTreeID(node));
m_compiler->gtChangeOperToNullCheck(node, m_compiler->compCurBB);
if (m_compiler->fgAddrCouldBeNull(node->AsUnOp()->gtGetOp1()))
JulieLeeMSFT marked this conversation as resolved.
Show resolved Hide resolved
{
JITDUMP("Replace an unused OBJ/BLK node [%06d] with a NULLCHECK\n", dspTreeID(node));
// It would change a non-faulting ind to a non-faulting nullcheck.
JulieLeeMSFT marked this conversation as resolved.
Show resolved Hide resolved
m_compiler->gtChangeOperToNullCheck(node, m_compiler->compCurBB);
}
else
{
JITDUMP("Replace an unused OBJ/BLK node [%06d] with a NOTHING node\n", dspTreeID(node));
node = m_compiler->gtNewNothingNode();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like this does anything, since node is a local, and is unused after the subsequent return.

Does the Append(node); need to be moved below, just before the return?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Append(node) sets class variable GenTree* m_result, and node is used either (1) to be directly assigned to m_result or (2) to make comma tree that is subsequently assigned to m_result.
So, I think the code is correct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that Append(node) here happens before you set node = m_compiler->gtNewNothingNode();. The NothingNode gets created then never used. And for the non-faulting case, the original BLK node is still on the side-effect list.

I think you want code more like this:

                if (m_compiler->gtNodeHasSideEffects(node, m_flags))
                {
                    if (node->OperIsBlk() && !node->OperIsStoreBlk())
                    {
                        // Check for a guaranteed non-faulting IND, and create a NOP node instead of a NULLCHECK in that case.
                        if (m_compiler->fgAddrCouldBeNull(node->AsBlk()->Addr()))
                        {
                            Append(node);
                            JITDUMP("Replace an unused OBJ/BLK node [%06d] with a NULLCHECK\n", dspTreeID(node));
                            m_compiler->gtChangeOperToNullCheck(node, m_compiler->compCurBB);
                        }
                        else
                        {
                            JITDUMP("Dropping non-faulting OBJ/BLK node [%06d]\n", dspTreeID(node));
                        }
                    }
                    else
                    {
                        Append(node);
                    }
                    return Compiler::WALK_SKIP_SUBTREES;
                }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}
}
return Compiler::WALK_SKIP_SUBTREES;
}
Expand Down
9 changes: 8 additions & 1 deletion src/coreclr/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8251,7 +8251,14 @@ void Compiler::impImportBlockCode(BasicBlock* block)
// via an underlying address, just null check the address.
if (op1->OperIs(GT_FIELD, GT_IND, GT_OBJ))
{
gtChangeOperToNullCheck(op1, block);
if (fgAddrCouldBeNull(op1->AsUnOp()->gtGetOp1()))
JulieLeeMSFT marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The op1 (addr) of a static GT_FIELD can be nullptr, but fgAddrCouldBeNull doesn't check for that. Could we AV here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you said, if the op1 address of GT_FIELD can be nullptr, we want true returned from fgAddrCouldBeNull(op1->gtGetOp1()). And, it falls through to the default case and returns true. Isn't it enough?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, because fgAddrColdBeNull() has switch (addr->OperGet()) and in the case I'm worried about, addr will be nullptr, so that would AV. Now, maybe there's some reason the case I'm worried about doesn't happen.

Or, maybe you can rewrite it to protect against that, e.g.:

                                const GenTree* addr = op1->gtGetOp1();
                                if ((addr != nullptr) && fgAddrCouldBeNull(addr))
                                {
                                    gtChangeOperToNullCheck(op1, block);
                                }
                                else
                                {
                                    op1 = gtNewNothingNode();
                                }

(presumably static fields don't need null checks)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

{
gtChangeOperToNullCheck(op1, block);
}
else
{
op1 = gtNewNothingNode();
}
}
else
{
Expand Down
21 changes: 17 additions & 4 deletions src/coreclr/jit/lower.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7279,13 +7279,16 @@ void Lowering::LowerIndir(GenTreeIndir* ind)
#if defined(TARGET_ARM64)
// Verify containment safety before creating an LEA that must be contained.
//
const bool isContainable = IsSafeToContainMem(ind, ind->Addr());
const bool isContainable = (ind->Addr() != nullptr) && IsSafeToContainMem(ind, ind->Addr());
JulieLeeMSFT marked this conversation as resolved.
Show resolved Hide resolved
#else
const bool isContainable = true;
#endif

TryCreateAddrMode(ind->Addr(), isContainable, ind);
ContainCheckIndir(ind);
if (!ind->OperIs(GT_NOP))
JulieLeeMSFT marked this conversation as resolved.
Show resolved Hide resolved
{
TryCreateAddrMode(ind->Addr(), isContainable, ind);
ContainCheckIndir(ind);
}

#ifdef TARGET_XARCH
if (ind->OperIs(GT_NULLCHECK) || ind->IsUnusedValue())
Expand Down Expand Up @@ -7333,14 +7336,24 @@ void Lowering::TransformUnusedIndirection(GenTreeIndir* ind, Compiler* comp, Bas
//
assert(ind->OperIs(GT_NULLCHECK, GT_IND, GT_BLK, GT_OBJ));

GenTree* const addr = ind->Addr();
if (!comp->fgAddrCouldBeNull(addr))
{
addr->SetUnusedValue();
ind->gtBashToNOP();
JITDUMP("bash an unused indir [%06u] to NOP.\n", comp->dspTreeID(ind));
return;
}

ind->ChangeType(comp->gtTypeForNullCheck(ind));

#if defined(TARGET_ARM64) || defined(TARGET_LOONGARCH64)
bool useNullCheck = true;
#elif TARGET_ARM
bool useNullCheck = false;
#else // TARGET_XARCH
bool useNullCheck = !ind->Addr()->isContained();
// bool useNullCheck = !ind->Addr()->isContained();
JulieLeeMSFT marked this conversation as resolved.
Show resolved Hide resolved
bool useNullCheck = !addr->isContained();
ind->ClearDontExtend();
#endif // !TARGET_XARCH

Expand Down