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

Handle getting the address of an RVA static correctly in a composite image #55301

Merged
merged 3 commits into from
Jul 15, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 6 additions & 4 deletions src/coreclr/jit/emitxarch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12226,10 +12226,12 @@ BYTE* emitter::emitOutputRR(BYTE* dst, instrDesc* id)
regMaskTP regMask;
regMask = genRegMask(reg1) | genRegMask(reg2);

// r1/r2 could have been a GCREF as GCREF + int=BYREF
// or BYREF+/-int=BYREF
assert(((regMask & emitThisGCrefRegs) && (ins == INS_add)) ||
((regMask & emitThisByrefRegs) && (ins == INS_add || ins == INS_sub)));
// Assert disabled as requested in PR 55301. A use of the Unsafe api
// which produces correct code, but isn't handled correctly here.
// r1/r2 could have been a GCREF as GCREF + int=BYREF
// or BYREF+/-int=BYREF
// assert(((regMask & emitThisGCrefRegs) && (ins == INS_add)) ||
// ((regMask & emitThisByrefRegs) && (ins == INS_add || ins == INS_sub)));
#endif
// Mark r1 as holding a byref
emitGCregLiveUpd(GCT_BYREF, reg1, dst);
Expand Down
27 changes: 24 additions & 3 deletions src/coreclr/jit/importer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7704,14 +7704,24 @@ GenTree* Compiler::impImportStaticFieldAccess(CORINFO_RESOLVED_TOKEN* pResolvedT
void** pFldAddr = nullptr;
void* fldAddr = info.compCompHnd->getFieldAddress(pResolvedToken->hField, (void**)&pFldAddr);

// We should always be able to access this static's address directly
//
// We should always be able to access this static's address directly unless this is a field RVA
// used within a composite image during R2R composite image building.
//
#ifdef FEATURE_READYTORUN_COMPILER
assert((pFldAddr == nullptr) ||
(pFieldInfo->fieldAccessor == CORINFO_FIELD_STATIC_RVA_ADDRESS && opts.IsReadyToRun()));
#else
assert(pFldAddr == nullptr);
#endif

FieldSeqNode* fldSeq = GetFieldSeqStore()->CreateSingleton(pResolvedToken->hField);

/* Create the data member node */
op1 = gtNewIconHandleNode(pFldAddr == nullptr ? (size_t)fldAddr : (size_t)pFldAddr, GTF_ICON_STATIC_HDL,
op1 = gtNewIconHandleNode(pFldAddr == nullptr ? (size_t)fldAddr : (size_t)pFldAddr,
#ifdef FEATURE_READYTORUN_COMPILER
pFldAddr != nullptr ? GTF_ICON_CONST_PTR :
#endif
GTF_ICON_STATIC_HDL,
fldSeq);
#ifdef DEBUG
op1->AsIntCon()->gtTargetHandle = op1->AsIntCon()->gtIconVal;
Expand All @@ -7721,6 +7731,17 @@ GenTree* Compiler::impImportStaticFieldAccess(CORINFO_RESOLVED_TOKEN* pResolvedT
{
op1->gtFlags |= GTF_ICON_INITCLASS;
}

#ifdef FEATURE_READYTORUN_COMPILER
if (pFldAddr != nullptr)
{
// Indirection used to get to initial actual field RVA when building a composite image
// where the actual field does not move from the original file
assert(!varTypeIsGC(lclTyp));
op1 = gtNewOperNode(GT_IND, TYP_I_IMPL, op1);
op1->gtFlags |= GTF_IND_INVARIANT | GTF_IND_NONFAULTING;
Copy link
Member

@EgorBo EgorBo Jul 7, 2021

Choose a reason for hiding this comment

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

there is also GTF_IND_NONNULL that can be used for indirects which return something non-null(0) always

}
#endif
}
else // We need the value of a static field
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,14 @@ private void CreateNodeCaches()
new ReadyToRunInstructionSetSupportSignature(key));
});

_precodeFieldAddressCache = new NodeCache<FieldDesc, ISymbolNode>(key =>
{
return new PrecodeHelperImport(
_codegenNodeFactory,
new FieldFixupSignature(ReadyToRunFixupKind.FieldAddress, key, _codegenNodeFactory)
);
});

_fieldAddressCache = new NodeCache<FieldDesc, ISymbolNode>(key =>
{
return new DelayLoadHelperImport(
Expand Down Expand Up @@ -398,6 +406,13 @@ public ISymbolNode FieldAddress(FieldDesc fieldDesc)
return _fieldAddressCache.GetOrAdd(fieldDesc);
}

private NodeCache<FieldDesc, ISymbolNode> _precodeFieldAddressCache;

public ISymbolNode PrecodeFieldAddress(FieldDesc fieldDesc)
{
return _precodeFieldAddressCache.GetOrAdd(fieldDesc);
}

private NodeCache<FieldDesc, ISymbolNode> _fieldOffsetCache;

public ISymbolNode FieldOffset(FieldDesc fieldDesc)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -641,7 +641,7 @@ protected override void ComputeDependencyNodeDependencies(List<DependencyNodeCor
}
}

public ISymbolNode GetFieldRvaData(FieldDesc field) => NodeFactory.CopiedFieldRva(field);
public ISymbolNode GetFieldRvaData(FieldDesc field) => NodeFactory.CompilationModuleGroup.IsCompositeBuildMode ? SymbolNodeFactory.PrecodeFieldAddress(field) : NodeFactory.CopiedFieldRva(field);

public override void Dispose()
{
Expand Down
15 changes: 15 additions & 0 deletions src/coreclr/vm/jitinterface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13997,6 +13997,21 @@ BOOL LoadDynamicInfoEntry(Module *currentModule,
break;
#endif // PROFILING_SUPPORTED

case ENCODE_FIELD_ADDRESS:
{
FieldDesc *pField = ZapSig::DecodeField(currentModule, pInfoModule, pBlob);

pField->GetEnclosingMethodTable()->CheckRestore();

// We can only take address of RVA field here as we don't handle scenarios where the static variable may move
// Also, this cannot be used with a ZapImage as it relies on a separate signatures block as an RVA static
// address may be unaligned which would interfere with the tagged pointer approach.
_ASSERTE(currentModule->IsReadyToRun());
_ASSERTE(pField->IsRVA());
result = (size_t)pField->GetStaticAddressHandle(NULL);
}
break;

case ENCODE_STATIC_FIELD_ADDRESS:
{
FieldDesc *pField = ZapSig::DecodeField(currentModule, pInfoModule, pBlob);
Expand Down