Skip to content
This repository has been archived by the owner on Mar 15, 2022. It is now read-only.

Support for synchronized methods. #458

Merged
merged 1 commit into from
Apr 17, 2015
Merged

Conversation

erozenfeld
Copy link
Member

Add calls to MONITOR_ENTER on entry to the method and MONITOR_EXIT on returns.

We can't catch exceptions yet; when we can we will need to add a try/fault for the entire method
so that we can call MONITOR_EXIT on unhandled exceptions.

We have a synchronized method in HelloWorld (System.IO.TextWriter+SyncTextWriter.WriteLine).
I verified that the generated IR looks correct. I also tested with a simple two-thread race that the
behavior of synchronized instance and static methods is as expected.

@@ -303,9 +303,10 @@ void GenIR::readerPrePass(uint8_t *Buffer, uint32_t NumBytes) {
ABIMethodSignature(MethodSignature, *this, *JitContext->TheABIInfo);
Function = ABIMethodSig.createFunction(*this, *JitContext->CurrentModule);

EntryBlock = BasicBlock::Create(*JitContext->LLVMContext, "entry", Function);
llvm::LLVMContext *LLVMContext = JitContext->LLVMContext;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this could be LLVMContext &LLVMContext = *JitContext->LLVMContext

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed.

@pgavlin
Copy link
Contributor

pgavlin commented Apr 17, 2015

LGTM modulo two nits.

@@ -367,9 +368,26 @@ void GenIR::readerPrePass(uint8_t *Buffer, uint32_t NumBytes) {
// Check for special cases where the Jit needs to do extra work.
const uint32_t MethodFlags = getCurrentMethodAttribs();

// TODO: support for synchronized methods
// Check for syncronized method. If a method is syncronized the JIT is
Copy link
Member

Choose a reason for hiding this comment

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

typo (2x): synchronized

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@AndyAyersMS
Copy link
Member

Ditto, LGTM with just a few other nits.

} else {
HelperId = IsEnter ? CORINFO_HELP_MON_ENTER : CORINFO_HELP_MON_EXIT;
}
callHelperImpl(HelperId, Type::getVoidTy(*JitContext->LLVMContext),
Copy link
Contributor

Choose a reason for hiding this comment

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

This will conflict with my #457. How about you go in first, and I'll fix up this callsite before submitting #457. When I do, should MayThrow be true, or are these calls guaranteed not to throw (and the monitor object guaranteed not to be null)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good. I'll merge once CI build is done.
These calls are guaranteed not to throw and the monitor object is guaranteed not to be null.

Copy link
Contributor

Choose a reason for hiding this comment

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

These calls are guaranteed not to throw and the monitor object is guaranteed not to be null.

Is this true for an arbitrary mscorlib?

Copy link
Contributor

Choose a reason for hiding this comment

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

What does arbitrary mean in this context? Is this possibly a non-CoreCLR/modern surface mscorlib? (we do not support desktop)

Copy link
Member Author

Choose a reason for hiding this comment

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

These are calls to jit helpers, they don't call into mscorlib.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I just looked. Disregard my comment.

Add calls to MONITOR_ENTER on entry to the method and MONITOR_EXIT on returns.

We can't catch exceptions yet; when we can we will need to add a try/fault for the entire method
so that we can call MONITOR_EXIT on unhandled exceptions.

We have a synchronized method in HelloWorld (System.IO.TextWriter+SyncTextWriter.WriteLine).
I verified that the generated IR looks correct. I also tested with a simple two-thread race that the
behavior of synchronized instance and static methods is as expected.
erozenfeld added a commit that referenced this pull request Apr 17, 2015
Support for synchronized methods.
@erozenfeld erozenfeld merged commit 1f1bdd6 into dotnet:master Apr 17, 2015
@erozenfeld erozenfeld deleted the SynchMethods branch April 17, 2015 23:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants