Skip to content

Commit

Permalink
Handle getting the address of an RVA static correctly in a composite …
Browse files Browse the repository at this point in the history
…image (dotnet#55301)

* Handle getting the address of an RVA static correctly in a composite image
- Since composite images have the original RVA statics in the original files, use the value from there
- This required re-enabling the ability of getFieldAddress to return an indirection
- And adding some new processing for the fixup as needed
* Silence pointless assert
- There is an assert around incorrect handling of byref pointers, that isn't right. It fails for various issues with the Unsafe.* apis under jit stress, and this change makes it occur during one of our regular builds. As this is just an assert firing when it isn't appropriate, I'm disabling the warning as suggested by the JIT team.
  • Loading branch information
davidwrighton authored Jul 15, 2021
1 parent ab21929 commit e3d319b
Show file tree
Hide file tree
Showing 5 changed files with 61 additions and 8 deletions.
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;
}
#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 @@ -14034,6 +14034,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

0 comments on commit e3d319b

Please sign in to comment.