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.

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

[NativeAOT-LLVM]: Question: Data()/gtOp2 for GT_OBJ ? #1673

Closed
yowl opened this issue Oct 23, 2021 · 2 comments
Closed

[NativeAOT-LLVM]: Question: Data()/gtOp2 for GT_OBJ ? #1673

yowl opened this issue Oct 23, 2021 · 2 comments

Comments

@yowl
Copy link
Contributor

yowl commented Oct 23, 2021

When converting GT_LCL_VAR to GT_OBJ for GC tracked objects, currently the gtOp2/Data() field for the GT_OBJ is not set.

https://github.com/yowl/runtimelab/blob/f8f9280b7e655d48fde4c437d8ad82d8ff5d3507/src/coreclr/jit/llvm.cpp#L1456-L1468

This causes a problem when dumping the tree at

https://github.com/yowl/runtimelab/blob/f8f9280b7e655d48fde4c437d8ad82d8ff5d3507/src/coreclr/jit/gentree.h#L7213

with stack

 	clrjit_browser_wasm32_x64.dll!GenTree::gtSkipReloadOrCopy() Line 7560	C++
>	clrjit_browser_wasm32_x64.dll!GenTree::OperIsInitBlkOp() Line 7213	C++
 	clrjit_browser_wasm32_x64.dll!GenTree::OperIsCopyBlkOp() Line 7220	C++
 	clrjit_browser_wasm32_x64.dll!Compiler::gtDispTree(GenTree * tree, IndentStack * indentStack, const char * msg, bool topOnly, bool isLIR) Line 12242	C++
 	clrjit_browser_wasm32_x64.dll!Compiler::gtDispLIRNode(GenTree * node, const char * prefixMsg) Line 13154	C++
 	clrjit_browser_wasm32_x64.dll!Compiler::gtDispRange(const LIR::ReadOnlyRange & range) Line 12984	C++
 	clrjit_browser_wasm32_x64.dll!Compiler::fgDumpBlock(BasicBlock * block) Line 2242	C++
 	clrjit_browser_wasm32_x64.dll!Compiler::fgDumpTrees(BasicBlock * firstBlock, BasicBlock * lastBlock) Line 2259	C++
 	clrjit_browser_wasm32_x64.dll!Compiler::fgDispBasicBlocks(BasicBlock * firstBlock, BasicBlock * lastBlock, bool dumpTrees) Line 2198	C++
 	clrjit_browser_wasm32_x64.dll!Compiler::fgDispBasicBlocks(bool dumpTrees) Line 2205	C++
 	clrjit_browser_wasm32_x64.dll!Phase::PostPhase(PhaseStatus status) Line 211	C++

What should GT_OBJ be storing in the second operand, gtOp2 ?

@SingleAccretion
Copy link

SingleAccretion commented Oct 23, 2021

Technically it should be set to nullptr. But SetOper already does this for debug builds, so it is a bit mysterious to me how this path is hit when dumping. What is the value of gtOp2 that you're seeing?

Edit: actually, nevermind, I see the issue, the check in SetOper is for an exact kind, but GT_OBJ is a GTF_EXOP as well.

I think the proper fix is to make OperIsBlkOp not rely on this Data() != nullptr fragile invariant and instead make it check explicitly for the interesting opers (i. e. GT_STORE_DYN_BLK, GT_STORE_BLK, GT_STORE_OBJ).

@yowl yowl closed this as completed Oct 25, 2021
@yowl
Copy link
Contributor Author

yowl commented Oct 25, 2021

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants