diff options
Diffstat (limited to 'Documentation/coding-guidelines')
4 files changed, 3576 insertions, 0 deletions
diff --git a/Documentation/coding-guidelines/EventLogging.md b/Documentation/coding-guidelines/EventLogging.md new file mode 100644 index 0000000000..a53d6e9844 --- /dev/null +++ b/Documentation/coding-guidelines/EventLogging.md @@ -0,0 +1,19 @@ +# CoreClr Event Logging Design + +##Introduction + +Event Logging is a mechanism by which CoreClr can provide a variety of information on it's state. This Logging works by inserting explicit logging calls by the developer within the VM . The Event Logging mechanism is largely based on [ETW- Event Tracing For Windows](https://msdn.microsoft.com/en-us/library/windows/desktop/bb968803(v=vs.85).aspx) + +# Adding Events to the Runtime + +- Edit the [Event manifest](../../src/vm/ClrEtwAll.man) to add a new event. For guidelines on adding new events, take a look at the existing events in the manifest and this guide for [ETW Manifests](https://msdn.microsoft.com/en-us/library/dd996930%28v=vs.85%29.aspx?f=255&MSPPError=-2147217396). +- The build system should automatically generate the required artifacts for the added events. +- Add entries in the [exclusion list](../../src/vm/ClrEtwAllMeta) if necessary +- The Event Logging Mechanism provides the following two functions, which can be used within the VM: + - **FireEtw**EventName, this is used to trigger the event + - **EventEnabled**EventName, this is used to see if any consumer has subscribed to this event + + +# Adding New Logging System + +Though the the Event logging system was designed for ETW, the build system provides a mechanism, basically an [adapter script- genXplatEventing.py](../../src/scripts/genXplatEventing.py) so that other Logging System can be added and used by CoreClr. An Example of such an extension for [LTTng logging system](https://lttng.org/) can be found in [genXplatLttng.py](../../src/scripts/genXplatLttng.py ) diff --git a/Documentation/coding-guidelines/clr-code-guide.md b/Documentation/coding-guidelines/clr-code-guide.md new file mode 100644 index 0000000000..84ba5f2321 --- /dev/null +++ b/Documentation/coding-guidelines/clr-code-guide.md @@ -0,0 +1,1269 @@ +What Every CLR Developer Must Know Before Writing Code +=== + +Written in 2006, by: + +- Rick Byers ([@RByers](https://github.com/RByers)) +- Jan Kotas ([@jkotas](https://github.com/jkotas)) +- Mike Stall ([@mikestall](https://github.com/mikestall)) +- Rudi Martin ([@Rudi-Martin](https://github.com/Rudi-Martin)) + +# Contents + +* [1 Why you must read this document](#1) + * [1.1 Rules of the Code](#1.1) + * [1.2 How do I <insert common task>?](#1.2) +* [2 Rules of the Code (Unmanaged)](#2) + * [2.1 Is your code GC-safe?](#2.1) + * [2.1.1 How GC holes are created](#2.1.1) + * [2.1.2 Your First GC hole](#2.1.2) + * [2.1.3 Use GCPROTECT_BEGIN to keep your references up to date](#2.1.3) + * [2.1.4 Don't do nonlocal returns from within GCPROTECT blocks](#2.1.4) + * [2.1.5 Do not GCPROTECT the same location twice](#2.1.5) + * [2.1.6 Protecting multiple OBJECTREF's](#2.1.6) + * [2.1.7 Use OBJECTHANDLES for non-scoped protection](#2.1.7) + * [2.1.8 Use the right GC Mode – Preemptive vs. Cooperative](#2.1.8) + * [2.1.9 Use OBJECTREF to refer to object references as it does automatic sanity checking](#2.1.9) + * [2.1.10 How to know if a function can trigger a GC](#2.1.10) + * [2.1.10.1 GC_NOTRIGGER/TRIGGERSGC on a scope](#2.1.10.1) + * [2.2 Are you using holders to track your resources?](#2.2) + * [2.2.1 What are holders and we are they important?](#2.2.1) + * [2.2.2 An example of holder usage:](#2.2.2) + * [2.2.3 Common Features of Holders](#2.2.3) + * [2.2.4 Where do I find a holder?](#2.2.4) + * [2.2.5 Can I bake my own holder?](#2.2.5) + * [2.2.6 What if my backout code throws an exception?](#2.2.6) + * [2.2.7 Pay attention to holder initialization semantics](#2.2.7) + * [2.2.8 Some generally useful prebaked holders](#2.2.8) + * [2.2.8.1 New'ed memory](#2.2.8.1) + * [2.2.8.2 New'ed array](#2.2.8.2) + * [2.2.8.3 COM Interface Holder](#2.2.8.3) + * [2.2.8.4 Critical Section Holder](#2.2.8.4) + * [2.3 Does your code follow our OOM rules?](#2.3) + * [2.3.1 What is OOM and why is it important?](#2.3.1) + * [2.3.2 Documenting where OOM's can happen](#2.3.2) + * [2.3.2.1 Functions that handle OOM's internally](#2.3.2.1) + * [2.3.2.2 OOM state control outside of contracts](#2.3.2.2) + * [2.3.2.3 Remember...](#2.3.2.3) + * [2.4 Are you using SString and/or the safe string manipulation functions?](#2.4) + * [2.4.1 SString](#2.4.1) + * [2.5 Are you using safemath.h for pointer and memory size allocations?](#2.5) + * [2.6 Are you using the right type of Critical Section?](#2.6) + * [2.6.1 Use only the official synchronization mechanisms](#2.6.1) + * [2.6.2 Using Crsts](#2.6.2) + * [2.6.3 Creating Crsts](#2.6.3) + * [2.6.4 Entering and Leaving Crsts](#2.6.4) + * [2.6.5 Other Crst Operations](#2.6.5) + * [2.6.6 Advice on picking a level for your Crst](#2.6.6) + * [2.6.7 Can waiting on a Crst generate an exception?](#2.6.7) + * [2.6.8 CRITSECT_UNSAFE Flags](#2.6.8) + * [2.6.9 Bypassing leveling (CRSTUNORDEREDnordered)](#2.6.9) + * [2.6.10 So what are the prerequisites and side-effects of entering a Crst?](#2.6.10) + * [2.6.11 Using Events and Waitable Handles](#2.6.11) + * [2.6.12 Do not get clever with "lockless" reader-writer data structures](#2.6.12) + * [2.6.13 Yes, your thread could be running non-preemptively!](#2.6.13) + * [2.6.14 Dos and Don'ts for Synchronization](#2.6.14) + * [2.7 Are you making hidden assumptions about the order of memory writes?](#2.7) + * [2.8 Is your code compatible with managed debugging?](#2.8) + * [2.9 Does your code work on 64-bit?](#2.9) + * [2.9.1 Primitive Types](#2.9.1) + * [2.10 Does your function declare a CONTRACT?](#2.10) + * [2.10.1 What can be said in a contract?](#2.10.1) + * [2.10.1.1 THROWS/NOTHROW](#2.10.1.1) + * [2.10.1.2 INJECT_FAULT(handler-stmt)/FORBID_FAULT](#2.10.1.2) + * [2.10.1.3 GC_TRIGGERS/GC_NOTRIGGER](#2.10.1.3) + * [2.10.1.4 MODE_PREEMPTIVE/ MODE_COOPERATIVE/ MODE_ANY](#2.10.1.4) + * [2.10.1.5 LOADS_TYPE(loadlevel)](#2.10.1.5) + * [2.10.1.6 CAN_TAKE_LOCK / CANNOT_TAKE_LOCK](#2.10.1.6) + * [2.10.1.7 EE_THREAD_REQUIRED / EE_THREAD_NOT_REQUIRED](#2.10.1.7) + * [2.10.1.8 SO_TOLERANT/SO_INTOLERANT](#2.10.1.8) + * [2.10.1.9 PRECONDITION(expr)](#2.10.1.9) + * [2.10.1.10 POSTCONDITION(expr)](#2.10.1.10) + * [2.10.2 Is order important?](#2.10.2) + * [2.10.3 Using the right form of contract](#2.10.3) + * [2.10.4 When is it safe to use a runtime contract?](#2.10.4) + * [2.10.5 Do not make unscoped changes to the ClrDebugState](#2.10.5) + * [2.10.6 For more details...](#2.10.6) + * [2.11 Is your code DAC compliant?](#2.11) + +# <a name="1"/>1 Why you must read this document + +Like most large codebases, the CLR codebase has many internal invariants and an extensive debug build infrastructure for detecting problems. Clearly, it is important that developers working on the CLR understand these rules and conventions. + +The information contained here is considered the minimum set of knowledge required of developers who work on any part of the CLR. This is the document we wished we had all throughout the CLR's history, especially when fixing a bug that could have been prevented had this information been more readily available. + +This document is divided into the following sections. + +## <a name="1.1"/>1.1 Rules of the Code + +This is the most important section. Think of the chapter headings as a checklist to use while designing and writing your code. This section is divided into sections for managed and unmanaged code as they face quite different issues. + +Rules can either be imposed by invariants or team policy. + +"Invariants" are actual semantic rules imposed by the architecture, e.g. the GC-safe use of managed object references in unmanaged code. There's nothing discretionary about these. Violate these and you've introduced a customer-visible bug. + +"Team Policy" rules are rules we've established as "good practices" – for example, the rule that every function must declare a contract. While a missing contract here or there isn't a shipstopper, violating these rules is still heavily frowned upon and you should expect a bug filed against you unless you can supply a very compelling reason why your code needs an exemption. + +Team policy rules are not necessarily less important than invariants. For example, the rule to use [safemath.h][safemath.h] rather that coding your own integer overflow check is a policy rule. But because it deals with security, we'd probably treat it as higher priority than a very obscure (non-security) related bug. + +[safemath.h]: https://github.com/dotnet/coreclr/blob/master/src/inc/safemath.h + +One type of rule you won't find here are purely syntactic "code formatting" rules such as brace placement. While there is value in uniform stylistic conventions, we don't want to "lay down the law" on these to the extent that we do for the more semantic-oriented issues covered here. The rules included in this document are here because breaking them would do one of the following: + +- Introduce an actual bug. +- Significantly increase the risk of a serious bug slipping through. +- Frustrate our automated bug-detection infrastructure. + +## <a name="1.2"/>1.2 How do I <insert common task>? + +The chapter headings in this section can be regarded as a FAQ. If you have a specific need, look here for "best practices" guidance on how to get something. Also, if you're thinking of adding yet another hash table implementation to the code base, check here first as there's a good chance there's already existing code that can be adapted or used as is. + +This section will also be divided into managed and unmanaged sections. + +# <a name="2"/>2 Rules of the Code (Unmanaged) + +## <a name="2.1"/>2.1 Is your code GC-safe? + +### <a name="2.1.1"/>2.1.1 How GC holes are created + +The term "GC hole" refers to a special class of bugs that bedevils the CLR. The GC hole is a pernicious bug because it is easy to introduce by accident, repros rarely and is very tedious to debug. A single GC hole can suck up weeks of dev and test time. + +One of the major features of the CLR is the Garbage Collection system. That means that allocated objects, as seen by a managed application, are never freed explicitly by the programmer. Instead, the CLR periodically runs a Garbage Collector (GC). The GC discards objects that are no longer in use. Also, the GC compacts the heap to avoid unused holes in memory. Therefore, a managed object does not have a fixed address. Objects move around according to the whims of the garbage collector. + +To do its job, the GC must be told about every reference to every GC object. The GC must know about every stack location, every register and every non-GC data structure that holds a pointer to a GC object. These external pointers are called "root references." + +Armed with this information, the GC can find all objects directly referenced from outside the GC heap. These objects may in turn, reference other objects – which in turn reference other objects and so on. By following these references, the GC finds all reachable ("live") objects. All other objects are, by definition, unreachable and therefore discarded. After that, the GC may move the surviving objects to reduce memory fragmentation. If it does this, it must, of course, update all existing references to the moved object. + +Any time a new object is allocated, a GC may occur. GC can also be explicitly requested by calling the GarbageCollect function directly. GC's do not happen asynchronously outside these events but since other running threads can trigger GC's, your thread must act as if GC's _are_ asynchronous unless you take specific steps to synchronize with the GC. More on that later. + +A GC hole occurs when code inside the CLR creates a reference to a GC object, neglects to tell the GC about that reference, performs some operation that directly or indirectly triggers a GC, then tries to use the original reference. At this point, the reference points to garbage memory and the CLR will either read out a wrong value or corrupt whatever that reference is pointing to. + +### <a name="2.1.2"/>2.1.2 Your First GC hole + +The code fragment below is the simplest way to introduce a GC hole into the system. + + //OBJECTREF is a typedef for Object*. + + { + MethodTable *pMT = g_pObjectClass->GetMethodTable(); + + OBJECTREF a = AllocateObject(pMT); + OBJECTREF b = AllocateObject(pMT); + + //WRONG!!! "a" may point to garbage if the second + //"AllocateObject" triggered a GC. + DoSomething (a, b); + } + +All it does is allocate two managed objects, and then does something with them both. + +This code compiles fine, and if you run simple pre-checkin tests, it will probably "work." But this code will crash eventually. + +Why? If the second call to AllocateObject() triggers a GC, that GC discards the object instance you just assigned to "a". This code, like all C++ code inside the CLR, is compiled by a non-managed compiler and the GC cannot know that "a" holds a root reference to an object you want kept live. + +This point is worth repeating. The GC has no intrinsic knowledge of root references stored in local variables or non-GC data structures maintained by the CLR itself. You must explicitly tell the GC about them. + +### <a name="2.1.3"/>2.1.3 Use GCPROTECT_BEGIN to keep your references up to date + +Here's how to fix our buggy code fragment. + + #include "frames.h" + { + MethodTable *pMT = g_pObjectClass->GetMethodTable(); + + //RIGHT + OBJECTREF a = AllocateObject(pMT); + + GCPROTECT_BEGIN(a); + OBJECTREF b = AllocateObject(pMT); + + DoSomething (a, b); + + GCPROTECT_END(); + } + +Notice the addition of the line GCPROTECT_BEGIN(a). GCPROTECT_BEGIN is a macro whose argument is any OBJECTREF-typed storage location (it has to be an expression that can you can legally apply the address-of (&) operator to.) GCPROTECT_BEGIN tells the GC two things: + +- The GC is not to discard any object referred to by the reference stored in local "a". +- If the GC moves the object referred to by "a", it must update "a" to point to the new location. + +Now, if the second AllocateObject() triggers a GC, the "a" object will still be around afterwards, and the local variable "a" will still point to it. "a" may not contain the same address as before, but it will point to the same object. Hence, DoSomething() receives the correct data. + +Note that we didn't similarly protect 'b" because the caller has no use for "b" after DoSomething returns. Furthermore, there's no point in keeping "b" updated because DoSomething receives a copy of the reference (don't confuse with "copy of the object"), not the reference itself. If DoSomething internally causes GC as well, it is DoSomething's responsibility to protect its own copies of "a" and "b". + +Having said that, no one should complain if you play it safe and GCPROTECT "b" as well. You never know when someone might add code later that makes the protection necessary. + +Every GCPROTECT_BEGIN must have a matching GCPROTECT_END, which terminates the protected status of "a". As an additional safeguard, GCPROTECT_END overwrites "a" with garbage so that any attempt to use "a" afterward will fault. GCPROTECT_BEGIN introduces a new C scoping level that GCPROTECT_END closes, so if you use one without the other, you'll probably experience severe build errors. + +### <a name="2.1.4"/>2.1.4 Don't do nonlocal returns from within GCPROTECT blocks + +Never do a "return", "goto" or other non-local return from between a GCPROTECT_BEGIN/END pair. This will leave the thread's frame chain corrupted. + +One exception: it is explicitly allowed to leave a GCPROTECT block by throwing a managed exception (usually via the COMPlusThrow() function). The exception subsystem knows about GCPROTECT and correctly fixes up the frame chain as it unwinds. + +Why is GCPROTECT not implemented via a C++ smart pointer? The GCPROTECT macro originates in .NET Framework v1. All error handling was done explicitly at that time, without any use C++ exception handling or stack allocated holders. + +### <a name="2.1.5"/>2.1.5 Do not GCPROTECT the same location twice + +The following is illegal and will cause some sort of crash: + + // WRONG: Can't GCPROTECT twice. + OBJECTREF a = AllocateObject(...); + GCPROTECT_BEGIN(a); + GCPROTECT_BEGIN(a); + +It'd be nice if the GC was robust enough to ignore the second, unnecessary GCPROTECT but I've been assured many times that this isn't possible. + +Don't confuse the reference with a copy of the reference. It's not illegal to protect the same reference twice. What is illegal is protecting the same _copy_ of the reference twice. Hence, the following is legal: + + OBJECTREF a = AllocateObject(...); + GCPROTECT_BEGIN(a); + DoSomething(a); + GCPROTECT_END(); + + void DoSomething(OBJECTREF a) + { + GCPROTECT_BEGIN(a); + GCPROTECT_END(); + } + +### <a name="2.1.6"/>2.1.6 Protecting multiple OBJECTREF's + +You can protect multiple OBJECTREF locations using one GCPROTECT. Group them all into a structure and pass the structure to GCPROTECT_BEGIN. GCPROTECT_BEGIN applies a sizeof to determine how many locations you want to protect. Do not mix any non-OBJECTREF fields into the struct! + +### <a name="2.1.7"/>2.1.7 Use OBJECTHANDLES for non-scoped protection + +GCPROTECT_BEGIN is very handy, as we've seen, but its protection is limited to a C++ nesting scope. Suppose you need to store a root reference inside a non-GC data structure that lives for an arbitrary amount of time? + +The solution is the OBJECTHANDLE. OBJECTHANDLE allocates a location from special blocks of memory that are known explicitly to the GC. Any root reference stored in this location will automatically keep the object live and be updated to reflect object moves. You can retrieve the correct reference by indirecting the location. + +Handles are implemented through several layers of abstraction – the "official" interface for public use is the one described here and is exposed through [objecthandle.h][objecthandle.h]. Don't confuse this with [handletable.h][handletable.h] which contains the internals. The CreateHandle() api allocates a new location. ObjectFromHandle() dereferences the handle and returns an up-to-date reference. DestroyHandle() frees the location. + +[objecthandle.h]: https://github.com/dotnet/coreclr/blob/master/src/gc/objecthandle.h +[handletable.h]: https://github.com/dotnet/coreclr/blob/master/src/gc/handletable.h + +The following code fragment shows how handles are used. In practice, of course, people use GCPROTECT rather than handles for situations this simple. + + { + MethodTable *pMT = g_pObjectClass->GetMethodTable(); + + //Another way is to use handles. Handles would be + // wasteful for a case this simple but are useful + // if you need to protect something for the long + // term. + OBJECTHANDLE ah; + OBJECTHANDLE bh; + + ah = CreateHandle(AllocateObject(pMT)); + bh = CreateHandle(AllocateObject(pMT)); + + DoSomething (ObjectFromHandle(ah), + ObjectFromhandle(bh)); + + DestroyHandle(bh); + DestroyHandle(ah); + } + +There are actually several flavors of handles. This section lists the most common ones. ([objecthandle.h][objecthandle.h] contains the complete list.) + +- **HNDTYPE_STRONG**: This is the default and acts like a normal reference. Created by calling CreateHandle(OBJECTREF). +- **HNDTYPE_WEAK_LONG**: Tracks an object as long as one strong reference to its exists but does not itself prevent the object from being GC'd. Created by calling CreateWeakHandle(OBJECTREF). +- **HNDTYPE_PINNED**: Pinned handles are strong handles which have the added property that they prevent an object from moving during a garbage collection cycle. This is useful when passing a pointer to object innards out of the runtime while GC may be enabled. + +NOTE: PINNING AN OBJECT IS EXPENSIVE AS IT PREVENTS THE GC FROM ACHIEVING OPTIMAL PACKING OF OBJECTS DURING EPHEMERAL COLLECTIONS. THIS TYPE OF HANDLE SHOULD BE USED SPARINGLY! + +### <a name="2.1.8"/>2.1.8 Use the right GC Mode – Preemptive vs. Cooperative + +Earlier, we implied that GC doesn't occur spontaneously. This is true... for a given thread. But the CLR is multithreaded. Even if your thread does all the right things, it has no control over other threads. + +Consider two possible ways to schedule GC: + +- **Preemptive**: Any thread that needs to do a GC can do one without regard for the state of other threads. The other threads run concurrently with the GC. +- **Cooperative**: A thread can only start a GC once all other threads agree to allow the GC. The thread attempting the GC is blocked until all other threads reach a state of agreement. + +Both have their strengths and drawbacks. Preemptive mode sounds attractive and efficient except for one thing: it completely breaks our previously discussed GC-protection mechanism. Consider the following code fragment: + + OBJECTREF a = AllocateObject(...) + GCPROTECT_BEGIN(a); + DoSomething(a); + +Now, while the compiler can generate any valid code for this, it's very likely it will look something like this: + + call AllocateObject + mov [A],eax ;;store result in "a" + ... code for GCPROTECT_BEGIN omitted... + push [A] ;push argument to DoSomething + call DoSomething + +This is supposed to be work correctly in every case, according to the semantics of GCPROTECT. However, suppose just after the "push" instruction, another thread gets the time-slice, starts a GC and moves the object A. The local variable A will be correctly updated – but the copy of A which we just pushed as an argument to DoSomething() will not. Hence, DoSomething() will receive a pointer to the old location and crash. Clearly, preemptive GC alone will not suffice for the CLR. + +How about the alternative: cooperative GC? With cooperative GC, the above problem doesn't occur and GCPROTECT works as intended. Unfortunately, the CLR has to interop with legacy unmanaged code as well. Suppose a managed app calls out to the Win32 MessageBox api which waits for the user to click a button before returning. Until the user does this, all managed threads in the same process are blocked from GC. Not good. + +Because neither policy alone suffices for the CLR, the CLR supports both: and you, as a developer, are responsible for switching the threads accordingly. Note that the GC-scheduling mode is a property of an individual thread; not a global system property. + +Put precisely: as long as a thread is in cooperative mode, it is guaranteed that a GC will only occur when your thread triggers an object allocation, calls out to interruptible managed code or explicitly requests a GC. All other threads are blocked from GC. As long as your thread is in preemptive mode, then you must assume that a GC can be started any time (by some other thread) and is running concurrently with your thread. + +A good rule of thumb is this: a CLR thread runs in cooperative mode any time it is running managed code or any time it needs to manipulate object references in any way. An Execution Engine (EE) thread that is running in preemptive mode is usually running unmanaged code; i.e. it has left the managed world. Process threads that have never entered CLR in any way are effectively running in preemptive mode. Much of the code inside CLR runs in cooperative mode. + +While you are running in preemptive mode, OBJECTREF's are strictly hands-off; their values are completely unreliable. In fact, the checked build asserts if you even touch an OBJECTREF in preemptive mode. In cooperative mode, you are blocking other threads from GC so you must avoid long or blocking operations. Also be aware of any critical sections or semaphores you wait on. They must not guard sections that themselves trigger GC. + +**Setting the GC mode:** The preferred way to set the GC mode are the GCX_COOP and GCX_PREEMP macros. These macros operate as holders. That is, you declare them at the start of the block of code you want to execute in a certain mode. Upon any local or non-local exit out of that scope, a destructor automatically restores the original mode. + + { // always open a new C++ scope to switch modes + GCX_COOP(); + Code you want run in cooperative mode + } // leaving scope automatically restores original mode + +It's perfectly legal to invoke GCX_COOP() when the thread is already in cooperative mode. GCX_COOP will be a NOP in that case. Likewise for GCX_PREEMP. + +GCX_COOP and GCX_PREEMP will never throw an exception and return no error status. + +There is a special case for purely unmanaged threads (threads that have no Thread structure created for them.) Such threads are considered "permanently preemptive." Hence, GCX_COOP will assert if called on such a thread while GCX_PREEMP will succeed as a NOP. + +There are a couple of variants for special situations: + +- **GCX_MAYBE_\*(BOOL)**: This version only performs the switch if the boolean parameter is TRUE. Note that the mode restore at the end of the scope still occurs whether or not you passed TRUE. (Of course, this is only important if the mode got switched some other way inside the scope. Usually, this shouldn't happen.) +- **GCX_\*_THREAD_EXISTS(Thread\*)**: If you're concerned about the repeated GetThread() and null Thread checks inside this holder, use this "performance" version which lets you cache the Thread pointer and pass it to all the GCX_\* calls. You cannot use this to change the mode of another thread. You also cannot pass NULL here. + +To switch modes multiple times in a function, you must introduce a new scope for each switch. You can also call GCX_POP(), which performs a mode restore prior to the end of the scope. (The mode restore will happen again at the end of the scope, however. Since mode restore is idempotent, this shouldn't matter.) Do not, however, do this: + + { + GCX_COOP(); + ... + GCX_PREEMP(): //WRONG! + } + +You will get a compile error due to a variable being redeclared in the same scope. + +While the holder-based macros are the preferred way to switch modes, sometimes one needs to leave a mode changed beyond the end of the scope. For those situations, you may use the "raw" unscoped functions: + + GetThread()->DisablePreemptiveGC(); // switch to cooperative mode + GetThread()->EnablePreemptiveGC(); // switch to preemptive mode + +There is no automatic mode-restore with these functions so the onus is on you to manage the lifetime of the mode. Also, mode changes cannot be nested. You will get an assert if you try to change to a mode you're already in. The "this" argument must be the currently executing thread. You cannot use this to change the mode of another thread. + +**Key Takeaway:** Use GCX_COOP/PREEMP rather than unscoped calls to DisablePreemptiveGC() whenever possible. + +**Testing/asserting the GC mode:** + +You can assert the need to be in a particular mode in the contract by using one of the following: + + CONTRACTL + { + MODE_COOPERATIVE + } + CONTRACTL_END + + CONTRACTL + { + MODE_PREEMPTIVE + } + CONTRACTL_END + +There are also standalone versions: + + { + GCX_ASSERT_COOP(); + } + + { + GCX_ASSERT_PREEMP(); + } + +You'll notice that the standalone versions are actually holders rather than simple statements. The intention was that these holders would assert again on scope exit to ensure that any backout holders are correctly restoring the mode. However, that exit check was disabled initially with the idea of enabling it eventually once all the backout code was clean. Unfortunately, the "eventually" has yet to arrive. As long as you use the GCX holders to manage mode changes, this shouldn't really be a problem. + +### <a name="2.1.9"/>2.1.9 Use OBJECTREF to refer to object references as it does automatic sanity checking + +The checked build inserts automatic sanity-checking every single time an OBJECTREF is manipulated. Under the retail build, OBJECTREF is defined as a pointer exactly as you'd expect. But under the checked build, OBJECTREF is defined as a "smart-pointer" class that sanity-checks the pointer on every operation. Also, the current thread is validated to be in cooperative GC mode. + +Thus, the following code fragment: + + OBJECTREF uninitialized; + DoSomething(uninitialized); + +will produce the following assert: + + "Detected use of a corrupted OBJECTREF. Possible GC hole." + +This is because the default constructor for OBJECTREF initializes to 0xcccccccc. When you pass "uninitialized" to DoSomething(), this invokes the OBJECT copy constructor which notices that the source of the copy is a bad pointer (0xcccccccc). This causes the assert. + +OBJECTREF's pointer mimicry isn't perfect. In certain cases, the checked build refuses to build legal-seeming constructs. We just have to work around this. A common case is casting an OBJECTREF to either a void* or a STRINGREF (we actually define a whole family of OBJECTREF-like pointers, for various interesting subclasses of objects.) The construct: + + //WRONG + OBJECTREF o = ...; + LPVOID pv = (LPVOID)o; + +compiles fine under retail but breaks under checked. The usual workaround is something like this: + + pv = (LPVOID)OBJECTREFToObject(o); + +### <a name="2.1.10"/>2.1.10 How to know if a function can trigger a GC + +The GC behavior of every function in the source base must be documented in its contract. Every function must have a contract that declares one of the following: + + // If you call me, assume a GC can happen + void Noisy() + { + CONTRACTL + { + GC_TRIGGERS; + } + CONTRACTL_END + } + +or + + // If you call me and the thread is in cooperative mode, I guarantee no GC + // will occur. + void Quiet() + { + CONTRACTL + { + GC_NOTRIGGER; + } + CONTRACTL_END + } + +A GC_NOTRIGGER function cannot: + +- Allocate managed memory +- Call managed code +- Enter a GC-safe point +- Toggle the GC mode <sup>[1]</sup> +- Block for long periods of time +- Synchronize with the GC +- Explicitly trigger a GC (duh) +- Call any other function marked GC_TRIGGERS +- Call any other code that does these things + +[1] With one exception: GCX_COOP (which effects a preemp->coop->preemp roundtrip) is permitted. The rationale is that GCX_COOP becomes a NOP if the thread was cooperative to begin with so it's safe to allow this (and necessary to avoid some awkward code in our product.) + +**Note that for GC to be truly prevented, the caller must also ensure that the thread is in cooperative mode.** Otherwise, all the precautions above are in vain since any other thread can start a GC at any time. Given that, you might be wondering why cooperative mode is not part of the definition of GC_NOTRIGGER. In fact, there is a third thread state called GC_FORBID which is exactly that: GC_TRIGGERS plus forced cooperative mode. As its name implies, GC_FORBID _guarantees_ that no GC will occur on any thread. + +Why do we use GC_NOTRIGGERS rather than GC_FORBID? Because forcing every function to choose between GC_TRIGGERS and GC_FORBID is too inflexible given that some callers don't actually care about GC. Consider a simple class member function that returns the value of a field. How should it be declared? If you choose GC_TRIGGERS, then the function cannot be legally called from a GC_NOTRIGGER function even though this is perfectly safe. If you choose GC_FORBID, then every caller must switch to cooperative mode to invoke the function just to prevent an assert. Thus, GC_NOTRIGGER was created as a middle ground and has become far more pervasive and useful than GC_FORBID. Callers who actually need GC stopped will have put themselves in cooperative mode anyway and in those cases, GC_NOTRIGGER actually becomes GC_FORBID. Callers who don't care can just call the function and not worry about modes. + +**Note:** There is no GC_FORBID keyword defined for contracts but you can simulate it by combining GC_NOTRIGGER and MODE_COOPERATIVE. + +**Important:** The notrigger thread state is implemented as a counter rather than boolean. This is unfortunate as this should not be necessary and exposes us to nasty ref-counting style bugs. What is important that contracts intentionally do not support unscoped trigger/notrigger transitions. That is, a GC_NOTRIGGER inside a contract will **increment** the thread's notrigger count on entry to the function but on exit, **it will not decrement the count , instead it will restore the count from a saved value.** Thus, any _net_ changes in the trigger state caused within the body of the function will be wiped out. This is good unless your function was designed to make a net change to the trigger state. If you have such a need, you'll just have to work around it somehow because we actively discourage such things in the first place. Ideally, we'd love to replace that counter with a Boolean at sometime. + +#### <a name="2.1.10.1"/>2.1.10.1 GC_NOTRIGGER/TRIGGERSGC on a scope + +Sometimes you want to mark a scope rather than a function. For that purpose, GC_TRIGGERS and TRIGGERSGC also exist as standalone holders. These holders are also visible to the static contract scanner. + + { + TRIGGERSGC(); + } + + { + GCX_NOTRIGGER(); + } + +One difference between the standalone TRIGGERSGC and the contract GC_TRIGGERS: the standalone version also performs a "phantom" GC that poisons all unreachable OBJECTREFs. The contract version does not do this mainly for checked build perf concerns. + +## <a name="2.2"/>2.2 Are you using holders to track your resources? + +### <a name="2.2.1"/>2.2.1 What are holders and we are they important? + +The CLR team has coined the name **holder** to refer to the infrastructure that encapsulates the common grunt work of writing robust **backout code**. **Backout code** is code that deallocate resources or restore CLR data structure consistency when we abort an operation due to an error or an asynchronous event. Oftentimes, the same backout code will execute in non-error paths for resources allocated for use of a single scope, but error-time backout is still needed even for longer lived resources. + +Way back in V1, error paths were _ad-hoc._ Typically, they flowed through "fail:" labels where the backout code was accumulated. + +Due to the no-compromise robustness requirements that the CLR Hosting model (with SQL Server as the initial customer) imposed on us in the .NET Framework v2 release, we have since become much more formal about backout. One reason is that we like to write backout that will execute if you leave the scope because of an exception. We also want to centralize policy regarding exceptions occurring inside backout. Finally, we want an infrastructure that will discourage developer errors from introducing backout bugs in the first place. + +Thus, we have centralized cleanup around C++ destructor technology. Instead of declaring a HANDLE, you declare a HandleHolder. The holder wraps a HANDLE and its destructor closes the handle no matter how control leaves the scope. We have already implemented standard holders for common resources (arrays, memory allocated with C++ new, Win32 handles and locks.) The Holder mechanism is extensible so you can add new types of holders as you need them. + +### <a name="2.2.2"/>2.2.2 An example of holder usage + +The following shows explicit backout vs. holders: + +**Wrong** + + HANDLE hFile = ClrCreateFile(szFileName, GENERIC_READ, ...); + if (hFile == INVALID_HANDLE_VALUE) { + COMPlusThrow(...); + } + + DWORD dwFileLen = SafeGetFileSize(hFile, 0); + if (dwFileLen == 0xffffffff) { + CloseHandle(hFile); + COMPlusThrow(...); + } + CloseHandle(hFile); + return S_OK; + +**Right** + + HandleHolder hFile(ClrCreateFile(szFileName, GENERIC_READ, ...)); + if (hFile == INVALID_HANDLE_VALUE) + COMPlusThrow(...); + + DWORD dwFileLen = SafeGetFileSize(hFile, 0); + if (dwFileLen == 0xffffffff) + COMPlusThrow(...); + + return S_OK; + +The difference is that hFile is now a HandleHolder rather than a HANDLE and that there are no more explicit CloseHandle calls. That call is now implicit in the holder's destructor and executes no matter how control leaves the scope. + +HandleHolder exposes operator overloads so it can be passed to APIs expecting HANDLEs without casting and be compared to INVALID_HANDLE_VALUE. The wrapper knows that INVALID_HANDLE_VALUE is special and won't attempt to close it. The holder also has some safety features. If you declare it without initializing it, it will be autoinitialized to INVALID_HANDLE_VALUE. If you assign a new value to the holder, the current value will be destructed before it is overwritten. + +Suppose you want to auto-close the handle if an error occurs but keep the handle otherwise? Call SuppressRelease() on the holder object. The underlying handle can still be pulled out of the holder but the destructor will no longer close it. + +**Wrong:** + + HANDLE hFile = ClrCreateFile(szFileName, GENERIC_READ, ...); + if (hFile == INVALID_HANDLE_VALUE) { + COMPlusThrow(...); + } + if (FAILED(SomeOperation())) { + CloseHandle(hFile); + COMPlusThrow(...); + } + return hFile; + +**Right:** + + HandleHolder hFile = ClrCreateFile(szFileName, GENERIC_READ, ...); + if (hFile == INVALID_HANDLE_VALUE) { + COMPlusThrow(...); + } + if (FAILED(SomeOperation())) { + COMPlusThrow(...); + } + // No failures allowed after this! + hFile.SuppressRelease(); + return hFile; + +### <a name="2.2.3"/>2.2.3 Common Features of Holders + +All holders, no matter how complex or simple, offer these basic services: + +- When the holder goes out of scope, via an exception or normal flow, it invokes a RELEASE function supplied by the holder's designer. The RELEASE function is responsible for the cleanup. +- A holder declared without an explicit initializer will be initialized to a default value. The precise value of the default is supplied by the holder's designer. +- Holders know about "null" values. The holder guarantees never to call RELEASE or ACQUIRE on a null value. The designer can specify any number of null values or no null value at all. +- Holders expose a public SuppressRelease() method which eliminates the auto-release in the destructor. Use this for conditional backout. +- Holders also support an ACQUIRE method when a resource can be meaningfully released and reacquired (e.g. locks.) + +In addition, some holders derive from the Wrapper class. Wrappers are like holders but also implement operator overloads for type casting, assignment, comparison, etc. so that the holder proxies the object smart-pointer style. The HandleHolder object is actually a wrapper. + +### <a name="2.2.4"/>2.2.4 Where do I find a holder? + +First, look for a prebaked holder that does what you want. Some common ones are described below. + +If no existing holder fits your need, make one. If it's your first holder, start by reading [src\inc\holder.h][holder.h]. Decide if you want a holder or a wrapper. If you don't do much with a resource except acquire and release it, use a holder. Otherwise, you want the wrapper since its overloaded operators make it much easier to replace the resource variable with the wrapper. + +[holder.h]: https://github.com/dotnet/coreclr/blob/master/src/inc/holder.h + +Instantiate the holder or wrapper template with the required parameters. You must supply the data type being managed, the RELEASE function, the default value for uninitialized constructions, the IS_NULL function and the ACQUIRE function. Unless you're implementing a critical section holder, you can probably supply a NOP for ACQUIRE . Most resources can't be meaningfully released and reacquired so it's easier to allocate the resource outside the holder and pass it in through its constructor. For convenience, [holder.h][holder.h] defines a DoNothing<Type> template that creates a NOP ACQUIRE function for any given resource type. There are also convenience templates for writing RELEASE functions. See [holder.h][holder.h] for their definitions and examples of their use. + +Publish the holder in the most global header file possible. [src\inc\holder.h][holder.h] is ideal for OS-type resources. Otherwise, put it in the header file that owns the type being managed. + +### <a name="2.2.5"/>2.2.5 Can I bake my own holder? + +When we first put holders into the code, we encouraged developers to inherit from the base holder class rather than writing their own. But the reality has been that many holders only need destruction and SuppressRelease() and it's proven easier for developers to write them from scratch rather than try to master the formidable C++ template magic that goes on in [holder.h][holder.h] It is better that you write your own holders than give up the design pattern altogether because you don't want to tackle [holder.h]. + +But however you decide to implement it, if you call your object a "holder", please make sure its external behavior conforms to the conventions listed above in "Common Features of Holders." + +### <a name="2.2.6"/>2.2.6 What if my backout code throws an exception? + +All holders wrap an implicit NOTHROW contract around your backout code. Thus, you must write your backout code only using primitives that are guaranteed not to throw. If you absolutely have no choice but to violate this (say, you're calling Release() on a COM object that you didn't write), you must catch the exception yourself. + +This may sound draconian but consider the real implications of throwing out of your backout code. Backout code, by definition, is code that must complete when throwing out of a block. If it didn't complete, there is no way to salvage the situation and still meet our reliability goals. Either something leaked or CLR state was left inconsistent. + +Often, you can avoid failures in backout code by designing a better data structure. For example, implementers of common data structures such as hash tables and collections should provide backout holders for undoing operations as inserts. When creating globally visible data structures such as EEClass objects, you should initialize the object in private and allocate everything needed before "publishing it." In some cases, this may require significant rethinking of your data structures and code. But the upshot is that you won't have to undo global data structure changes in backout code. + +### <a name="2.2.7"/>2.2.7 Pay attention to holder initialization semantics + +Holders consistently release on destruction – that's their whole purpose. Sadly, we are not so consistent when it comes the initialization semantics. Some holders, such as the Crst holder, do an implicit Acquire on initialization. Others, such as the ComHolder do not (initializing a ComHolder does _not_ do an AddRef.) The BaseHolder class constructor leaves it up to the holder designer to make the choice. This is an easy source of bugs so pay attention to this. + +### <a name="2.2.8"/>2.2.8 Some generally useful prebaked holders + +#### <a name="2.2.8.1"/>2.2.8.1 New'ed memory + +**Wrong:** + + Foo *pFoo = new Foo(); + delete pFoo; + +**Right:** + + NewHolder<Foo> pFoo = new Foo(); + +#### <a name="2.2.8.2"/>2.2.8.2 New'ed array + +**Wrong:** + + Foo *pFoo = new Foo[30]; + delete pFoo; + +**Right:** + + NewArrayHolder<Foo> pFoo = new Foo[30]; + +#### <a name="2.2.8.3"/>2.2.8.3 COM Interface Holder + +**Wrong:** + + IFoo *pFoo = NULL; + FunctionToGetRefOfFoo(&pFoo); + pFoo->Release(); + +**Right:** + + ComHolder<IFoo> pFoo; // declaring ComHolder does not insert AddRef! + FunctionToGetRefOfFoo(&pFoo); + +#### <a name="2.2.8.4"/>2.2.8.4 Critical Section Holder + +**Wrong:** + pCrst->Enter(); + pCrst->Leave(); + +**Right:** + + { + CrstHolder(pCrst); //implicit Enter + } //implicit Leave + +## <a name="2.3"/>2.3 Does your code follow our OOM rules? + +### <a name="2.3.1"/>2.3.1 What is OOM and why is it important? + +OOM stands for "Out of Memory." The CLR must be fully robust in the face of OOM errors. For us, OOM is not an obscure corner case. SQL Server runs its processes in low-memory conditions as normal practice. OOM exceptions are a regular occurrence when hosted under SQL Server and we are required to handle every single one correctly. + +This means that: + +- Any operation that fails due to an OOM must allow future retries. This means any changes to global data structures must be rolled back and OOM exceptions cannot be cached. +- OOM failures must be distinguishable from other error results. OOM's must never be transformed into some other error code. Doing so may cause some operations to cache the error and return the same error on each retry. +- Every function must declare whether or not it can generate an OOM error. We cannot write OOM-safe code if we have no way to know what calls can generate OOM's. This declaration is done by the INJECT_FAULT and FORBID_FAULT contract annotations. + +### <a name="2.3.2"/>2.3.2 Documenting where OOM's can happen + +Sometimes, a code sequence requires that no opportunities for OOM occur. Backout code is the most common example. This can become hard to maintain if the code calls out to other functions. Because of this, it is very important that every function document in its contract whether or not it can fail due to OOM. We do this using the (poorly named) INJECT_FAULT and FORBID_FAULT annotations. + +To document that a function _can_ fail due to OOM: + +**Runtime-based (preferred)** + + void AllocateThingie() + { + CONTRACTL + { + INJECT_FAULT(COMPlusThrowOM();); + } + CONTRACTL_END + } + +**Static** + + void AllocateThingie() + { + STATIC_CONTRACT_FAULT; + } + +To document that a function _cannot_ fail due to OOM: + +**Runtime-based (preferred)** + + BOOL IsARedObject() + { + CONTRACTL + { + FORBID_FAULT; + } + CONTRACTL_END + } + +**Static** + + BOOL IsARedObject() + { + STATIC_CONTRACT_FORBID_FAULT; + } + +INJECT_FAULT()'s argument is the code that executes when the function reports an OOM. Typically this is to throw an OOM exception or return E_OUTOFMEMORY. The original intent for this was for our OOM fault injection test harness to insert simulated OOM's at this point and execute this line. At the moment, this argument is ignored but we may still employ this fault injection idea in the future so please code it appropriately. + +The CLR asserts if you invoke an INJECT_FAULT function under the scope of a FORBID_FAULT. All our allocation functions, including the C++ new operator, are declared INJECT_FAULT. + +#### <a name="2.3.2.1"/>2.3.2.1 Functions that handle OOM's internally + +Sometimes, a function handles an internal OOM without needing to notify the caller. For example, perhaps the additional memory was used to implement an internal cache but your function can still do its job without it. Or perhaps the function is a logging function in which case, it can silently NOP – the caller doesn't care. In such cases, wrap the allocation in the FAULT_NOT_FATAL holder which temporarily lifts the FORBID_FAULT state. + + { + FAULT_NOT_FATAL(); + pv = new Foo(); + } + +FAULT_NOT_FATAL() is almost identical to a CONTRACT_VIOLATION() but the name indicates that it is by design, not a bug. It is analogous to TRY/CATCH for exceptions. + +#### <a name="2.3.2.2"/>2.3.2.2 OOM state control outside of contracts + +If you wish to set the OOM state for a scope rather than a function, use the FAULT_FORBID() holder. To test the current state, use the ARE_FAULTS_FORBIDDEN() predicate. + +#### <a name="2.3.2.3"/>2.3.2.3 Remember... + +- Do not use INJECT_FAULT to indicate the possibility of non-OOM errors such as entries not existing in a hash table or a COM object not supporting an interface. INJECT_FAULT indicates OOM errors and no other type. +- Be very suspicious if your INJECT_FAULT() argument is anything other than throwing an OOM exception or returning E_OUTOFMEMORY. OOM errors must distinguishable from other types of errors so if you're merely returning NULL without indicating the type of error, you'd better be a simple memory allocator or some other function that will never fail for any reason other than an OOM. +- THROWS and INJECT_FAULT correlate strongly but are independent. A NOTHROW/INJECT_FAULT combo might indicate a function that returns HRESULTs including E_OUTOFMEMORY. A THROWS/FORBID_FAULT however indicate a function that can throw an exception but not an OutOfMemoryException. While theoretically possible, such a contract is probably a bug. + +## <a name="2.4"/>2.4 Are you using SString and/or the safe string manipulation functions? + +The native C implementation of strings as raw char* buffers is a well-known breeding ground for buffer overflow bugs. While acknowledging that there's still a ton of legacy char*'s in the code, new code and new data structures should use the SString class rather than raw C strings whenever possible. + +### <a name="2.4.1"/>2.4.1 SString + +SString is the abstraction to use for unmanaged strings in CLR code. It is important that as much code is possible uses the SString abstraction rather than raw character arrays, because of the danger of buffer overrun related to direct manipulation of arrays. Code which does not use SString must be manually reviewed for the possibility of buffer overrun or corruption during every security review. + +This section will provide an overview for SString. For specific details on methods and use, see the file [src\inc\sstring.h][sstring.h]. SString has been in use in our codebase for quite a few years now so examples of its use should be easy to find. + +[sstring.h]: https://github.com/dotnet/coreclr/blob/master/src/inc/sstring.h + +An SString object represents a Unicode string. It has its own buffer which it internally manages. The string buffer is typically not referenced directly by user code; instead the string is manipulated indirectly by methods defined on SString. Ultimately there are several ways to get at the raw string buffer if such functionality is needed to interface to existing APIs. But these should be used only when absolutely necessary. + +When SStrings are used as local variables, they are typically used via the StackSString type, which uses a bit of stack space as a preallocated buffer for optimization purposes. When SStrings are use in structures, the SString type may be used directly (if it is likely that the string will be empty), or through the InlineSString template, which allows an arbitrary amount of preallocated space to be declared inline in the structure with the SString. Since InlineSStrings and StackSStrings are subtypes of SString, they have the same API, and can be passed wherever an SString is required. + +As parameters, SStrings should always be declared as reference parameters. Similarly, SStrings as return value should also use a "by reference" style. + +An SString's contents can be initialized by a "raw" string, or from another SString. A WCHAR based string is assumed to be Unicode, but for a CHAR based string, you must specify the encoding by explicitly invoking one of the tagged constructors with the appropriate encoding (currently Utf8, Ansi, or Console). + +In addition, you can specially call out string literals with the Literal tag – this allows the SString implementation to make some additional optimizations during copying and allocation operations. It's important to only use this take for actual read-only compiler literals. Never use them for other strings which might be freed or modified in the future, even if you believe the SString's lifetime will be shorter than the buffer's. + +SStrings' contents are typically operated on through use of one of the iterators provided. A SString::CIterator (obtained from a const SString), is used to look at but not change the string. A SString::Iterator (obtained from a non-const SString) should be used when the string will be modified. Note that it is a slightly heavier operation to create a non-const Iterator, so you should use CIterator when appropriate. + +Either kind of iterator acts like (through the magic of C++ operator overloading) a pointer into the string buffer. Performance is also similar, although it may be slightly reduced. An iterator also has similar lifetime constraints to a buffer pointer – if an SString changes sizes, the existing iterators on it cease to be valid. (Fortunately the iterator infrastructure provides some explicit checks to aid in enforcement of this constraint, in a checked build.) + +If you need to use the string in the context of an external API (either to get the string's contents to pass out, or to use the SString as a buffer to receive a return result.), you may use one of the conversion APIs. Read-only use of the buffer is provided via a simple API; however if you need to write to the string's buffer, you must use an Open/Close call pair around the operation. + +For easy creation of an SString for a string literal, use the SL macro. This can be used around either a normal (ASCII characters only) or wide string constant. + +## <a name="2.5"/>2.5 Are you using safemath.h for pointer and memory size allocations? + +Integer overflow bugs are an insidious source of buffer overrun vulnerabilities.Here is a simple example of how such a bug can occur: + + void *pInput = whatever; + UINT32 cbSizeOfData = GetSizeOfData(); + UINT32 cbAllocSize = SIZE_OF_HEADER + cbSizeOfData; + void *pBuffer = Allocate(cbAllocSize); + memcpy(pBuffer + SIZE_OF_HEADER, pInput, cbSizeOfData); + +If GetSizeOfData() obtains its result from untrusted data, it could return a huge value just shy of UINT32_MAX. Adding SIZE_OF_HEADER causes a silent overflow, resulting in a very small (and incorrect) value being passed to Allocate() which dutifully returns a short buffer. The memcpy, however, copies a huge number of bytes and overflows the buffer. + +The source of the bug is clear. The code should have checked if adding SIZE_OF_HEADER and cbSizeOfData overflowed before passing it to Allocate(). + +We have now standardized on an infrastructure for performing overflow-safe arithmetic on key operations such as calculating allocation sizes. This infrastructure lives in [clr\src\inc\safemath.h][safemath.h]. + +The _safe_ version of the above code follows: + + #include "safemath.h" + + void *pInput = whatever; + S_UINT32 cbSizeOfData = S_UINT32(GetSizeOfData()); + S_UINT32 cbAllocSize = S_UINT32(SIZE_OF_HEADER) + cbSizeOfData; + if (cbAllocSize.IsOverflow()) + { + return E_OVERFLOW; + } + void *pBuffer = Allocate(cbAllocSize.Value()); + memcpy(pBuffer + SIZE_OF_HEADER, pInput, cbSizeOfData); + +As you can see, the transformation consists of the following: + +- Replace the raw C++ integer type with the "S_" version. +- Do the arithmetic as usual. +- Call IsOverflow() on the _final_ result to see if an overflow occurred anytime during the calculations. It's not necessary to check intermediate results if multiple arithmetic operations are chained. [Safemath.h][safemath.h] will propagate the overflow state through the entire chain of operations. +- If IsOverflow() returned false, then call Value() on the final result to get the raw integer back. Otherwise, there's no value to be returned – invoke your error handling code. + +As you'd expect, Value() asserts if IsOverflow() is true. + +As you might _not_ expect, Value() also asserts if you never called IsOverflow() to check – whether or not the result actually overflowed. This guarantees you won't forget the IsOverflow() check. If you didn't check, Value() won't give you the result. + +Currently, the "S_" types are available only for unsigned ints and SIZE_T. Check in [safemath.h][safemath.h] for what's currently defined. Also, only addition and multiplication are supported although other operations could be added if needed. + +**Key Takeaway: Use safemath.h for computing allocation sizes and pointer offsets.** Don't rely on the fact that the caller may have already validated the data. You never know what new paths might be added to your vulnerable code. + +**Key Takeaway: If you're working on existing code that does dynamic memory allocation, check for this bug.** + +**Key Takeaway: Do not roll your own overflow checks. Always use safemath.h.** Writing correct overflow-safe arithmetic code is harder than you might think (take a look at the implementation in [safemath.h][safemath.h] if you don't believe me.) Every unauthorized version is another security hotspot that has to be watched carefully. If safemath.h doesn't support the functionality you need, please get the functionality added to safemath.h rather than creating a new infrastructure. + +**Key Takeaway: Don't let premature perf concerns stop you from using safemath.h.** Despite the apparently complexity, the optimized codegen for this helper is very efficient and in most cases, at least as efficient as any hand-rolled version you might be tempted to create. + +**Note:** If you've worked on other projects that use the SafeInt class, you might be wondering why we don't do that here. The reason is that we needed something that could be used easily from exception-intolerant code. + +## <a name="2.6"/>2.6 Are you using the right type of Critical Section? + +Synchronization in the CLR is challenging because we must support the strong requirements of the CLR Hosting API. This has two implications: + +- Hosting availability goals require that we eliminate all races and deadlocks. We need to maintain a healthy process under significant load for weeks and months at a time. Miniscule races will eventually be revealed. +- Hosting requires that we often execute on non-preemptively scheduled threads. If we block a non-preemptively scheduled thread, we idle a CPU and possibly deadlock the process. + +### <a name="2.6.1"/>2.6.1 Use only the official synchronization mechanisms + +First, the most important rule. If you learn nothing else here, learn this: + +> DO NOT BUILD YOUR OWN LOCK. + +A CLR host must be able to detect and break deadlocks. To do this, it must know at all times who owns locks and who is waiting to acquire a lock. If you bypass a host using your own mechanisms, or if even you use a host's events to simulate a lock, you will defeat a host's ability to trace and break deadlocks. You must also eschew the OS synchronization services such as CRITICAL_SECTION. + +We have the following approved synchronization mechanisms in the CLR: + +1. **Crst:** This is our replacement for the Win32 CRITICAL_SECTION. We should be using Crst's pretty much everywhere we need a lock in the CLR. +2. **Events:** A host can provide event handles that replace the Win32 events. +3. **InterlockedIncrement/Decrement/CompareExchange:** These operations may be used for lightweight ref-counting and initialization scenarios. + +Make sure you aren't using events to build the equivalent of a critical section. The problem with this is that we cannot identify the thread that "owns" the critical section and hence, the host cannot trace and break deadlocks. In general, if you're creating a situation that could result in a deadlock, even if only due to bad user code, you must ensure that a CLR host can detect and break the deadlock. + +### <a name="2.6.2"/>2.6.2 Using Crsts + +The Crst class ([crst.h][crst.h]) is a replacement for the standard Win32 CRITICAL_SECTION. It has all the properties and features of a CRITICAL_SECTION, plus a few extra nice features. We should be using Crst's pretty much everywhere we need a lock in the CLR. + +Crst's are also used to implement our locking hierarchy. Every Crst is placed into a numbered group, or _level_. A thread can only request a Crst whose level is lower than any Crst currently held by the thread. I.e., if a thread currently holds a level 3 Crst, it can try to enter a level 2 Crst, but not a level 4 Crst, nor a different level 3 Crst. This prevents the cyclic dependencies that lead to deadlocks. + +We used to assign levels manually, but this leads to problems when it comes time to add a new Crst type or modify an existing one. Since the assignment of levels essentially flattens the dependencies between Crst types into one linear sequence we have lost information on which Crst types really depend on each other (i.e. which types ever interact by being acquired simultaneously on one thread and in which order). This made it hard to determine where to rank a new lock in the sequence. + +Instead we now record the explicit dependencies as a set of rules in the src\inc\CrstTypes.def file and use a tool to automatically assign compatible levels to each Crst type. See CrstTypes.def for a description of the rule syntax and other instructions for updating Crst types. + +[crst.h]: https://github.com/dotnet/coreclr/blob/master/src/vm/crst.h + +### <a name="2.6.3"/>2.6.3 Creating Crsts + +To create a Crst: + + Crst *pcrst = new Crst(type [, flags]); + +Where "type" is a member of the CrstType enumeration (defined in the automatically generated src\inc\CrstTypes.h file). These types indicate the usage of the Crst, particularly with regard to which other Crsts may be obtained simultaneously, There is a direct mapping for the CrstType to a level (see CrstTypes.h) though the reverse is not true. + +Don't create static instances of Crsts<sup>[2]</sup>. Use CrstStatic class for this purpose, instead. + +Simply define a CrstStatic as a static variable, then initialize the CrstStatic when appropriate: + + g_GlobalCrst.Init(type"tag", level); + +A CrstStatic must be destroyed with the Destroy() method as follows: + + g_GlobalCrst.Destroy(); + +[2]: In fact, you should generally avoid use of static instances that require construction and destruction. This can have an impact on startup time, it can affect our shutdown robustness, and it will eventually limit our ability to recycle the CLR within a running process. + +### <a name="2.6.4"/>2.6.4 Entering and Leaving Crsts + +To enter or leave a crst, you must wrap the crst inside a CrstHolder. All operations on crsts are available only through the CrstHolder. To enter the crst, create a local CrstHolder and pass the crst as an argument. The crst is automatically released by the CrstHolder's destructor when control leaves the scope either normally or via an exception: + + { + CrstHolder ch(pcrst); // implicit enter + + ... do your thing... may also throw... + + } // implicit leave + +**You can only enter and leave Crsts in preemptive GC mode.** Attempting to enter a Crst in cooperative mode will forcibly switch your thread into preemptive mode. + +If you need a Crst that you can take in cooperative mode, you must pass a special flag to the Crst constructor to do so. See the information about CRITSECT_UNSAFE_\* flags below. You will also find information about why it's preferable not to take Crsts in cooperative mode. + +You can also manually acquire and release crsts by calling the appropriate methods on the holder: + + { + CrstHolder ch(pcrst); // implicit enter + + ... + ch.Release(); // temporarily leave + ... + ch.Acquire(); // temporarily enter + + } // implicit leave + +Note that holders do not let you nest Acquires or Releases. You will get an assert if you try. Introduce a new scope and a new holder if you need to do this. + +If you need to create a CrstHolder without actually entering the critical section, pass FALSE to the holder's "take" parameter like this: + + { + CrstHolder ch(pcrst, FALSE); // no implicit enter + + ... + } // no implicit leave + +If you want to exit the scope without leaving the Crst, call SuppressRelease() on the holder: + + { + CrstHolder ch(pcrst); // implicit enter + ch.SuppressRelease(); + } // no implicit leave + +### <a name="2.6.5"/>2.6.5 Other Crst Operations + +If you want to validate that you own no other locks at the same or lower level, assert the debug-only IsSafeToTake() method: + + _ASSERTE(pcrst->IsSafeToTake()); + +Entering a crst always calls IsSafeToTake() for you but calling it manually is useful for functions that acquire a lock only some of the time. + +### <a name="2.6.6"/>2.6.6 Advice on picking a level for your Crst + +The point of giving your critical section a level is to help us prevent deadlocks by detecting cycles early in the development process. We try to group critical sections that protect low-level data structures and don't use other services into the lower levels, and ones that protect higher-level data structures and broad code paths into higher levels. + +If your lock is only protecting a single data structure, and if the methods accessing that data structure don't call into other CLR services that could also take locks, then you should give your lock the lowest possible level. Using the lowest level ensures that someone can't come along later and modify the code to start taking other locks without violating the leveling. This will force us to consider the implications of taking other locks while holding your lock, and in the end will lead to better code. + +If your lock is protecting large sections of code that call into many other parts of the CLR, then you need to give your lock a level high enough to encompass all the locks that will be taken. Again, try to pick a level as low as possible. + +Add a new definition for your level rather than using an existing definition, even if there is an existing definition with the level you need. Giving each lock its own level in the enum will allow us to easily change the levels of specific locks at a later time. + +### <a name="2.6.7"/>2.6.7 Can waiting on a Crst generate an exception? + +It depends. + +If you initialize the crst as CRST_HOST_BREAKABLE, any attempt to acquire the lock can trigger an exception (intended to kill your thread to break the deadlock.) Otherwise, you are guaranteed not to get an exception or failure. Regardless of the flag setting, releasing a lock will never fail. + +You can only use a non host-breakable lock if you can guarantee that that lock will never participate in a deadlock. If you cannot guarantee this, you must use a host-breakable lock and handle the exception. Otherwise, a CLR host will not be able to break deadlocks cleanly. + +There are several ways we enforce this. + +1. A lock that is CRST_UNSAFE_SAMELEVEL must be HOST_BREAKABLE: SAMELEVEL allows multiple locks at the same level to be taken in any order. This sidesteps the very deadlock avoidance that leveling provides. +2. You cannot call managed code while holding a non-hostbreakable lock. We assume that you can't guarantee what the managed code will do. Thus, you can't guarantee that the managed code won't acquire user locks, which don't participate at all in the leveling scheme. User locks can be acquired in any order and before or after any internal CLR locks. Hence, you cannot guarantee that the lock won't participate in a deadlock cycle along with the user locks. + +You may be wondering why we invest so much effort into the discipline of deadlock avoidance, and then also require everyone to tolerate deadlock breaking by the host. Sometimes we are unhosted, so we must avoid deadlocks. Some deadlocks involve user code (like class constructors) and cannot be avoided. Some exceptions from lock attempts are due to resource constraints, rather than deadlocks. + +### <a name="2.6.8"/>2.6.8 CRITSECT_UNSAFE Flags + +By default, Crsts can only be acquired and released in preemptive GC mode and threads can only own one lock at any given level at a given time. Some locks need to bypass these restrictions. To do so, you must pass the appropriate flag when you create the critical section. (This is the optional third parameter to the Crst constructor.) + +**CRST_UNSAFE_COOPGC** + +If you pass this flag, it says that your Crst will always be taken in Cooperative GC mode. This is dangerous because you cannot allow a GC to occur while the lock is held<sup>[3]</sup>. Entering a coop mode lock puts your thread in ForbidGC mode until you leave the lock. For handy reference, some of the things you can't do in ForbidGC mode are: + +- Allocate managed memory +- Call managed code +- Enter a GC-safe point +- Toggle the GC mode +- Block for long periods of time +- Synchronize with the GC +- Call any other code that does these things + +**CRST_UNSAFE_ANYMODE** + +If you pass this flag, your Crst can be taken in either Cooperative or Preemptive mode. The thread's mode will not change as a result of taking the lock, however, it will be placed in a GCNoTrigger state. We have a set of assertions to try to ensure that you don't cause problems with the GC due to this freedom. These assertions are the famous "Deadlock situation" messages from our V1 code base. However, it's important to realize that these assertions do not provide full safety, because they rely on code coverage to catch your mistakes. + +Note that CRST_UNSAFE_COOPGC and CRST_UNSAFE_ANYMODE are mutually exclusive despite being defined as "or'able" bits. + +**CRST_UNSAFE_SAMELEVEL** + +All Crsts are ordered to avoid deadlock. The CRST_UNSAFE_SAMELEVEL flag weakens this protection by allowing multiple Crsts at the same level to be taken in any order. This is almost always a bug. + +I know of one legitimate use of this flag. It is the Crst that protects class construction (.cctors). The application can legally create cycles in class construction. The CLR has rules for breaking these cycles by allowing classes to see uninitialized data under well-defined circumstances. + +In order to use CRST_UNSAFE_SAMELEVEL, you should write a paragraph explaining why this is a legal use of the flag. Add this explanation as a comment to the constructor of your Crst. + +Under no circumstances may you use CRST_UNSAFE_SAMELEVEL for a non-host-breakable lock. + +[3] More precisely, you cannot allow a GC to block your thread at a GC-safe point. If it does, the GC could deadlock because the GC thread itself blocks waiting for a third cooperative mode thread to reach its GC-safe point... which it can't do because it's trying to acquire the very lock that your first thread owns. This wouldn't be an issue if acquiring a coop-mode lock was itself a GC-safe point. But too much code relies on this not being a GC-safe point to fix this easily + +### <a name="2.6.9"/>2.6.9 Bypassing leveling (CRSTUNORDEREDnordered) + +CrstUnordered (used in rules inside CrstTypes.def) is a special level that says that the lock does not participate in any of the leveling required for deadlock avoidance. This is the most heinous of the ways you can construct a Crst. Though there are still some uses of this in the CLR, it should be avoided by any means possible. + +### <a name="2.6.10"/>2.6.10 So what _are_ the prerequisites and side-effects of entering a Crst? + +The following matrix lists the effective contract and side-effects of entering a crst for all combinations of CRST_HOST_BREAKABLE and CRST_UNSAFE_\* flags. The SAMELEVEL flag has no effect on any of these parameters. + +| | Default | CRST_HOST_BREAKABLE | +| ------------------- | ----------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------- | +| Default | NOTHROW<br> FORBID_FAULT<br>GC_TRIGGERS<br>MODE_ANY<br>(switches thread to preemptive) | THROWS<br>INJECT_FAULT<br>GC_TRIGGERS<br>MODE_ANY<br>(switches thread to preemptive) | +| CRST_UNSAFE_COOPGC | NOTHROW<br>FORBID_FAULT<br>GC_NOTRIGGER<br>MODE_COOP<br>(puts thread in GCNoTrigger mode) | THROWS<br>INJECT_FAULT<br>GC_NOTRIGGER<br>MODE_COOP<br>(puts thread in GCNoTrigger mode) | +| CRST_UNSAFE_ANYMODE | NOTHROW<br>FORBID_FAULT<br>GC_NOTRIGGER<br>MODE_ANY<br>(puts thread in GCNoTrigger mode) | THROWS<br>INJECT_FAULT<br>GC_NOTRIGGER<br>MODE_ANY<br>(puts thread in GCNoTrigger mode) | + +### <a name="2.6.11"/>2.6.11 Using Events and Waitable Handles + +In typical managed app scenarios, services like WszCreateEvent are thin wrappers over OS services like ::CreateEvent. But in hosted scenarios, these calls may be redirected through an abstraction layer to the host. If that's the case, they may return handles that behave somewhat like OS events, but do not support coordination with unmanaged code. Nor can we provide WaitForMultipleHandles support on these handles. You are strictly limited to waiting on a single handle. + +If you need to coordinate with unmanaged code, or if you need to do WaitForMultipleHandles ANY/ALL, you will have to avoid WszCreateEvent. If you really know what you are doing, go directly to the OS to obtain these handles. Everyone else should seek advice from someone who thoroughly understands the implications to our host. Obviously the general rule is that everyone should go through our hosted abstraction. + +Sometimes you might find yourself building the equivalent of a critical section, but using an event directly. The problem here is that we cannot identify the thread that owns the lock, because the owner isn't identified until he "leaves'" the lock by calling SetEvent or Pulse. Consider whether a Crst might be more appropriate. + +### <a name="2.6.12"/>2.6.12 Do not get clever with "lockless" reader-writer data structures + +Earlier, we had several hashtable structures that attempted to be "clever" and allow lockless reading. Of course, these structures didn't take into account multiprocessors and the other memory models. Even on single-proc x86, stress uncovered exotic race conditions. This wasted a lot of developer time debugging stress crashes. + +We finally stopped being clever and added proper synchronization, with no serious perf degradation. + +So if you are tempted to get clever in this way again, **stop and do something else until the urge passes.** + +### <a name="2.6.13"/>2.6.13 Yes, your thread could be running non-preemptively! + +Under hosted scenarios, your thread could actually be scheduled non-preemptively (do not confuse this with "GC preemptive mode.".) Blocking a thread without yielding back to the host could have consequences ranging from CPU starvation (perf) to an actual deadlock. You are particularly vulnerable when calling OS apis that block. + +Unfortunately, there is no official list of "safe" OS apis. The safest approach is to stick to the officially approved synchronization mechanisms documented in this chapter and be extra careful when invoking OS api. + +### <a name="2.6.14"/>2.6.14 Dos and Don'ts for Synchronization + +- Don't build your own lock or use OS locks. Only use Crst or host events and waitable handles. A host must know who owns what to detect and break deadlocks. +- Don't use events to simulate locks or any other synchronization mechanism that could lead to deadlocks. Again, if a host doesn't know about a deadlock situation, it can't break it. +- Don't use a CRITICAL_SECTION anywhere inside the CLR. Use Crst. One exception. If there are bootstrap or shutdown issues that require synchronization beyond the period when the CLR is initialized, you may use CRITICAL_SECTION (e.g. g_LockStartup). +- Do pick the lowest possible level for your Crst. +- Don't create static instances of Crst. Use CrstStatic instead. +- Do assert IsSafeToTake() if your function only takes a crst some of the time. +- Do use the default Crst rather than the CRST_UNSAFE_\* alternatives. They're named that for a reason. +- Do choose correctly between host-breakable and non-breakable crsts. Crsts that don't protect calls to managed code and participate fully in the leveling scheme can be non-breakable. Otherwise, you must use breakable. +- Don't take locks in cooperative mode if you can avoid it. This can delay or stall the GC. You are in a ForbidGC region the entire time you hold the lock. +- Don't block a thread without yielding back to the host. Your "thread" may actually be a nonpreemptive thread. Always stick to the approved synchronization primitives. +- Do document your locking model. If your locking model involves protecting a resource with a critical section, maybe you don't have to mention that in a comment. But if you have an elaborate mechanism where half your synchronization comes from GC guarantees and being in cooperative mode, while the other half is based on taking a spin lock in preemptive mode – then you really need to write this down. Nobody (not even you) can debug or maintain your code unless you have left a detailed comment. + +## <a name="2.7"/>2.7 Are you making hidden assumptions about the order of memory writes? + +_Issues: X86 processors have a very predictable memory order that 64-bit chips or multiprocs don't observe. We've gotten burned in the past because of attempts to be clever at writing thread-safe data structures without crsts. The best advice here is "don't be so clever, the perf improvements usually don't justify the risk." (look for Vance's writeup on memory models for a start.) _ + +## <a name="2.8"/>2.8 Is your code compatible with managed debugging? + +The managed debugging services have some very unique properties in the CLR, and take a heavy dependency on the rest of the system. This makes it very easy to break managed debugging without even touching a line of debugger code. Here are some key trivia and tips to help you play well with the managed-debugging services. + +Be aware of things that make the debugger subsystem different than other subsystems: + +- The debugger runs mostly out-of-process. +- The debugger generally inspects things at a very intimate level. For example, the debugger can see private fields, the offsets of those fields, and what registers an object may be stored in. +- The debugger needs to be able to stop and synchronize the debuggee, in a similar way as the GC. That means all those GC-contracts, GC-triggers, GC-toggling, etc, may heavily affect the debugger's synchronization too. +- Whereas most subsystems can just patiently wait for a GC to complete, the debugger will need to do complicated work during a GC-suspension. + +Here are some immediate tips for working well with the managed-debugging services: + +- Check if you need to DAC-ize your code for debugging! DACizing means adding special annotations so that the debugger can re-use your code to read key CLR data structures from out-of-process. This is especially applicable for code that inspects runtime data structures (running callstacks; inspecting a type; running assembly or module lists; enumerating jitted methods; doing IP2MD lookups; etc). Code that will never be used by the debugger does not have to be DAC-ized. However, when in doubt, it's safest to just DAC-ize your code. +- Don't disassemble your own code. Breakpoints generally work by writing a "break opcode" (int3 on x86) into the instruction stream. Thus when you disassemble your code, you may get the breakpoint opcode instead of your own original opcode. Currently, we have to workaround this by having all runtime disassembly ask the debugger if there's a break opcode at the targeted address, and that's painful. +- Avoid self-modifying code. Avoid this for the same reasons that you shouldn't disassemble your own code. If you modify your own code, that would conflict with the debugger adding breakpoints there. +- Do not change behavior when under the debugger. An app should behave identically when run outside or under the debugger. This is absolutely necessary else we get complaints like "my program only crashes when run under the debugger". This is also necessary because somebody may attach a debugger to an app after the fact. Specific examples of this: + - Don't assume that just because an app is under the debugger that somebody is trying to debug it. + - Don't add additional run-time error checks when under the debugger. For example, avoid code like: if ((IsDebuggerPresent() && (argument == null)) { throw MyException(); } + - Avoid massive perf changes when under the debugger. For example, don't use an interpreted stub just because you're under the debugger. We then get bugs like [my app is 100x slower when under a debugger](http://blogs.msdn.com/b/jmstall/archive/2006/01/17/pinvoke-100x-slower.aspx). + - Avoid algorithmic changes. For example, do not make the JIT generate non-optimized code just because an app is under the debugger. Do not make the loader policy resolve to a debuggable-ngen image just because an app is under the debugger. +- Separate your code into a) side-effect-free (non-mutating) read-only accessors and b) functions that change state. The motivation is that the debugger needs to be able to read-state in a non-invasive way. For example, don't just have GetFoo() that will lazily create a Foo if it's not available. Instead, split it out like so: + - GetFoo() - fails if a Foo does not exist. Being non-mutating, this should also be GC_NOTRIGGER. Non-mutating will also make it much easier to DAC-ize. This is what the debugger will call. + - and GetOrCreateFoo() that is built around GetFoo(). The rest of the runtime can call this. + - The debugger can then just call GetFoo(), and deal with the failure accordingly. +- If you add a new stub (or way to call managed code), make sure that you can source-level step-in (F11) it under the debugger. The debugger is not psychic. A source-level step-in needs to be able to go from the source-line before a call to the source-line after the call, or managed code developers will be very confused. If you make that call transition be a giant 500 line stub, you must cooperate with the debugger for it to know how to step-through it. (This is what StubManagers are all about. See [src\vm\stubmgr.h](https://github.com/dotnet/coreclr/blob/master/src/vm/stubmgr.h)). Try doing a step-in through your new codepath under the debugger. +- **Beware of timeouts** : The debugger may completely suspend your process at arbitrary points. In most cases, the debugger will do the right thing (and suspend your timeout too), but not always. For example, if you have some other process waiting for info from the debuggee, it [may hit a timeout](http://blogs.msdn.com/b/jmstall/archive/2005/11/11/contextswitchdeadlock.aspx). +- **Use CLR synchronization primitives (like Crst)**. In addition to all the reasons listed in the synchronization section, the CLR-aware primitives can cooperate with the debugging services. For example: + - The debugger needs to know when threads are modifying sensitive data (which correlates to when the threads lock that data). + - Timeouts for CLR synchronization primitives may operate better in the face of being debugged. +- **Optimized != Non-debuggable:** While performance is important, you should make sure your perf changes do not break the debugger. This is especially important in stepping, which requires the debugger to know exactly where we are going to execute managed code. For example, when we started using IL stubs for reverse pinvoke calls in the .NET Framework 2, the debugger was no longer notified that a thread was coming back to managed code, which broke stepping. You can probably find a way to make your feature area debuggable without sacrificing performance. + +**Examples of dependencies** : Here's a random list of ways that the debugger depends on the rest of the runtime. + +- Debugger must be able to inspect CLR data-structures, so your code must be DAC-ized. Examples include: running a module list, walking the thread list, taking a callstack, recognizing stubs, and doing an IP2MD lookup. You can break the debugger by just breaking the DAC (changing DAC-ized code so that it is no longer dac-ized correctly). +- Type-system: Debugger must be able to traverse the type-system. +- Need notifications from VM: loader, exception, jit-complete, etc. +- Anything that affects codegen (Emit, Dynamic language, IBC)s: the debugger needs to know where the code is and how it's laid out. +- GC, threading – debugger must be GC-aware. For example, we must protect the user from trying to inspect the GC-heap in the middle of a GC. The debugger must also be able to do a synchronization that may compete with a GC-synchronization. +- Step-in through a stub: Any time you add a new stub or new way of calling managed code, you might break stepping. +- Versioning: You could write a debugger in managed code targeting CLR version X, but debugging a process that's loaded CLR version Y. Now that's some versioning nightmares. + +## <a name="2.9"/>2.9 Does your code work on 64-bit? + +### <a name="2.9.1"/>2.9.1 Primitive Types + +Because the CLR is ultimately compiled on several different platforms, we have to be careful about the primitive types which are used in our code. Some compilers can have slightly different declarations in standard header files, and different processor word sizes can require values to have different representations on different platforms. + +Because of this, we have gathered definition all of the "blessed" CLR types in a single header file, [clrtypes.h](https://github.com/dotnet/coreclr/blob/master/src/inc/clrtypes.h). In general, you should only use primitive types which are defined in this file. As an exception, you may use built-in primitive types like int and short when precision isn't particularly interesting. + +The types are grouped into several categories. + +- Fixed representation integral types. (INT8, UINT8, INT16, UINT16, INT32, UINT32, INT64, UINT64) These typedefs will always have the same representation on each platform. Each type is named with the number of bits in the representation. +- Pointer sized integral types. (SIZE_T, SSIZE_T) These types will change size on platforms, depending on the native pointer size. Use SIZE_T whenever you need to cast pointers to and from integral types. SSIZE_T is the signed version of SIZE_T; use it if you are computing a difference of two arbitrary pointers. +- Large count-sized integral types (COUNT_T, SCOUNT_T) These are used when you would normally use a SIZE_T or SSIZE_T on a 32 bit machine, but you know you won't ever need more than 32 bits, even on a 64 bit machine. Use this type where practical to avoid bloated data sizes. +- Semantic content types: (BOOL, BYTE). Use these types to indicate additional semantic context to an integral type. BYTE indicates "raw data", and BOOL indicates a value which can be either TRUE or FALSE. +- Character data types (CHAR, SCHAR, UCHAR, WCHAR, ASCII, ANSI, UTF8). These have fixed sizes and represent single characters in strings. CHAR may be either signed or unsigned. Note that CHAR/SCHAR/UCHAR specify no semantics about character set; use ASCII, ANSI, and UTF8 to indicate when a specific encoding is used.It is worth mentioning that manipulation of strings as raw character arrays is discouraged; instead code should use the SString class wherever possible. +- Pointer to executable code PCODE. Use these for any pointers to (managed) executable code. + +All standard integral types have *_MIN and *_MAX values declared as well. + +## <a name="2.10"/>2.10 Does your function declare a CONTRACT? + +Every function in the CLR must declare a contract. A contract enumerates important behavioral facts such as whether a function throws or whether it can trigger gc. It also a general container for expressing preconditions and postconditions specific to that function. + +Contracts help determine which functions can safely invoke others. These constraints are enforced in two ways: + +- Statically, using a special tool that analyzes callgraphs and flags violations. +- Runtime assertions. + +These two approaches are complementary. Static analysis is always preferable but the tool cannot reliably find all call paths and can not check custom preconditions. Runtime checks are only as good as our code coverage. + +Here is a typical contract: + + LPVOID Foo(char *name, Blob *pBlob) + { + CONTRACTL + { + THROWS; // This function may throw + INJECT_FAULT(COMPlusThrowOM()); // This function may fail due to OOM + GC_TRIGGERS; // This function may trigger a GC + MODE_COOPERATIVE; // Must be in GC-cooperative mode to call + CAN_TAKE_LOCK; // This function may take a Crst, spinlock, etc. + EE_THREAD_REQUIRED; // This function expects an EE Thread object in the TLS + PRECONDITION(CheckPointer(name)); // Invalid to pass NULL + PRECONDITION(CheckPointer(pBlob, NULL_OK)); // Ok to pass NULL + } + CONTRACTL_END; + + ... + } + +There are several flavors of contracts. This example shows the most common type (CONTRACTL, where "L" stands for "lite.") + +At runtime (on a checked build), the contract does the following: + +At the start of Foo(), it validates that it's safe to throw, safe to generate an out of memory error, safe to trigger gc, that the GC mode is cooperative, and that your preconditions are true. + +On a retail build, CONTRACT expands to nothing. + +### <a name="2.10.1"/>2.10.1 What can be said in a contract? + +As you can see, a contract is a laundry list of "items" that either assert some requirement on the current thread state or impose a requirement on downstream callees. The following is a whirlwind tour of the supported annotations. The nuances of each one are explained in more detail in their individual chapters. + +#### <a name="2.10.1.1"/>2.10.1.1 THROWS/NOTHROW + +Declares whether an exception can be thrown out of this function. Declaring **NOTHROW** puts the thread in a NOTHROW state for the duration of the function call. You will get an assert if you throw an exception or call a function declared THROWS. An EX_TRY/EX_CATCH construct however will lift the NOTHROW state for the duration of the TRY body. + +#### <a name="2.10.1.2"/>2.10.1.2 INJECT_FAULT(_handler-stmt_)/FORBID_FAULT + +This is a poorly named item. INJECT_FAULT declares that the function can **fail** due to an out of memory (OOM) condition. FORBID_FAULT means that the function promises never to fail due to OOM. FORBID_FAULT puts the thread in a FORBID_FAULT state for the duration of the function call. You will get an assert if you allocate memory (even with the C++ new operator) or call a function declared INJECT_FAULT. + +#### <a name="2.10.1.3"/>2.10.1.3 GC_TRIGGERS/GC_NOTRIGGER + +Declares whether the function is allowed to trigger a GC. GC_NOTRIGGER puts the thread in a NOTRIGGER state where any call to a GC_TRIGGERS function will assert. + +**Observation:** THROWS does not necessarily imply GC_TRIGGERS. COMPlusThrow does not trigger GC. + +#### <a name="2.10.1.4"/>2.10.1.4 MODE_PREEMPTIVE/ MODE_COOPERATIVE/ MODE_ANY + +This item asserts that the thread is in a particular mode or declares that the function is mode-agnostic. It does not change the state of the thread in any way. + +#### <a name="2.10.1.5"/>2.10.1.5 LOADS_TYPE(_loadlevel_) + +This item asserts that the function may invoke the loader and cause a type to loaded up to (and including) the indicated loadlevel. Valid load levels are taken from ClassLoadLevel enumerationin [classLoadLevel.h](https://github.com/dotnet/coreclr/blob/master/src/vm/classloadlevel.h). + +The CLR asserts if any attempt is made to load a type past the current limit set by LOADS_TYPE. A call to any function that has a LOADS_TYPE contract is treated as an attempt to load a type up to that limit. + +#### <a name="2.10.1.6"/>2.10.1.6 CAN_TAKE_LOCK / CANNOT_TAKE_LOCK + +These declare whether a function or callee takes any kind of EE or user lock: Crst, SpinLock, readerwriter, clr critical section, or even your own home-grown spin lock (e.g., ExecutionManager::IncrementReader). + +In TLS we keep track of the current intent (whether to lock), and actual reality (what locks are actually taken). Enforcement occurs as follows: + +[contract.h]: https://github.com/dotnet/coreclr/blob/master/src/inc/contract.h + +- SCAN + - A CANNOT_TAKE_LOCK function calling a CAN_TAKE_LOCK function is illegal (just like THROWS/NOTHROWS) +- Dynamic checking: + - A CANNOT_TAKE_LOCK function calling a CAN_TAKE_LOCK function is illegal + - *_LOCK_TAKEN / *_LOCK_RELEASED macros (contract.h): + - Sprinkled at all places we take/release actual or conceptual locks + - Asserts if taking a lock in a CANNOT_TAKE_LOCK scope + - Keeps count of locks currently taken by thread + - Remembers stack of lock pointers for diagnosis + - ASSERT_NO_EE_LOCKS_HELD(): Handy way for you to verify no locks are held right now on this thread (i.e., lock count == 0) + +#### <a name="2.10.1.7"/>2.10.1.7 EE_THREAD_REQUIRED / EE_THREAD_NOT_REQUIRED + +These declare whether a function or callee deals with the case "GetThread() == NULL". + +EE_THREAD_REQUIRED simply asserts that GetThread() != NULL. + +EE_THREAD_NOT_REQUIRED is a noop by default. You must "set COMPlus_EnforceEEThreadNotRequiredContracts=1" for this to be enforced. Setting the envvar forces a C version of GetThread() to be used, instead of the optimized assembly versions. This C GetThread() always asserts in an EE_THREAD_NOT_REQUIRED scope regardless of whether there actually is an EE Thread available or not. The reason is that if you claim you don't require an EE Thread, then you have no business asking for it (even if you get lucky and there happens to be an EE Thread available). + +Of course, there are exceptions to this. In particular, if there is a clear code path for GetThread() == NULL, then it's ok to call GetThread() in an EE_THREAD_NOT_REQUIRED scope. You declare your intention by using GetThreadNULLOk(): + + Thread* pThread = GetThreadNULLOk(); + if (pThread != NULL) + { + pThread->m_dwAVInRuntimeImplOkayCount++; + } + +Rule: You should only use GetThreadNULLOk if it is patently obvious from the call site that NULL is dealt with directly. Obviously, this would be bad: + + GetThreadNULLOk()->BeginCriticalRegion(); + +This is also frowned upon, as it's unclear whether a NULL Thread is handled: + + MyObj myObj(GetThreadNULLOk()); + +In more complex situations, a caller may be able to vouch for an EE Thread's existence, while its callee cannot. So you can set up a scope that temporarily stops doing the EE_THREAD_NOT_REQUIRED verification as follows: + + CONTRACTL + { + EE_THREAD_NOT_REQUIRED; + } CONTRACTL_END; + + Thread* pThread = GetThreadNULLOk(); + if (pThread == NULL) + return; + + // We know there's an EE Thread now, so it's safe to call GetThread() + // and expect a non-NULL return. + BEGIN_GETTHREAD_ALLOWED; + CallCodeThatRequiresThread(); + END_GETTHREAD_ALLOWED; + +BEGIN/END_GETTHREAD_ALLOWED simply instantiate a holder that temporarily disables the assert on each GetThread() call. A non-holder version is also available which can generate less code if you're wrapping a NOTHROW region: BEGIN/END_GETTHREAD_ALLOWED_IN_NO_THROW_REGION. In fact, GetThreadNULLOk() is implemented by just calling GetThread() from within a BEGIN/END_GETTHREAD_ALLOWED_IN_NO_THROW_REGION block. + +You should only use BEGIN/END_GETTHREAD_ALLOWED(_IN_NO_THROW_REGION) if: + +- It is provably impossible for GetThread() to ever return NULL from within that scope, or +- All code within that scope directly deals with GetThread()==NULL. + +If the latter is true, it's generally best to push BEGIN/END_GETTHREAD_ALLOWED down the callee chain so all callers benefit. + +#### <a name="2.10.1.8"/>2.10.1.8 SO_TOLERANT/SO_INTOLERANT + +These are related to stack probes. SO_TOLERANT means the function is written in such a way that it is safe to throw a StackOverflow exception between any two instructions. It doesn't update global state, doesn't modify data structures, and doesn't call out to the operating system. + +If you don't specify SO_TOLERANT, the function is treated as SO_INTOLERANT. + +The CLR asserts if you invoke an SO_INTOLERANT function outside the scope of a stack probe. The probe's purpose is to check in advance if sufficient stack is available and trigger the SO exception before venturing into SO_INTOLERANT code. + +#### <a name="2.10.1.9"/>2.10.1.9 PRECONDITION(_expr_) + +This is pretty self-explanatory. It is basically an **_ASSERTE.** Both _ASSERTE's and PRECONDITIONS are used widely in the codebase. The expression can evaluate to either a Boolean or a Check. + +#### <a name="2.10.1.10"/>2.10.1.10 POSTCONDITION(_expr_) + +This is an expression that's tested on a _normal_ function exit. It will not be tested if an exception is thrown out of the function. Postconditions can access the function's locals provided that the locals were declared at the top level scope of the function. C++ objects will not have been destructed yet. + +Because of the limitations of our macro infrastructure, this item imposes some syntactic ugliness into the function. More on this below. + +### <a name="2.10.2"/>2.10.2 Is order important? + +Preconditions and postconditions will execute in the order declared. The "intrinsic" items will execute before any preconditions regardless of where they appear. + +### <a name="2.10.3"/>2.10.3 Using the right form of contract. + +Contracts come in several forms: + +- CONTRACTL: This is the most common type. It does runtime checks as well as being visible to the static scanner. It is suitable for all runtime contracts except those that use postconditions. When in doubt, use this form. +- CONTRACT(returntype): This is an uglier version that's needed if you include a POSTCONDITION. You must supply the correct function return type for this form and it cannot be "void" (use CONTRACT_VOID instead.) You must also use the special RETURN macro rather than the normal return keyword. +- CONTRACT_VOID: Use this if you need a postcondition and the return type is void. CONTRACT(void) will not work. +- STATIC_CONTRACT_\*: This form generates no runtime code but still emits the hidden tags visible to the static contract scanner. Use this only if checked build perf would suffer greatly by putting a runtime contract there or if for some technical reason, the runtime-based contract is not possible.. +- LIMITED_METHOD_CONTRACT: A static contract equivalent to NOTHROW/GC_NOTRIGGER/FORBID_FAULT/MODE_ANY/CANNOT_TAKE_LOCK. Use this form only for trivial one-liner functions. Remember it does not do runtime checks so it should not be used for complex functions. +- WRAPPER_NO_CONTRACT: A static no-op contract for functions that trivially wrap another. This was invented back when we didn't have static contracts and we now wish it hadn't been invented. Please don't use this in new code. + +### <a name="2.10.4"/>2.10.4 When is it safe to use a runtime contract? + +Contracts do not require that current thread have a Thread structure. Even those annotations that explicitly check Thread bits (the GC and MODE annotations) will correctly handle the NULL ThreadState case. + +Contracts can and are used outside of the files that build CLR. However, the GC_TRIGGERS and MODE family of items is not available outside of CLR. + +You cannot use runtime contracts if: + +- Your code is callable from the implementation of FLS (Fiber Local Storage). This may result in an infinite recursion as the contract infrastructure itself uses FLS. +- Your code makes a net change to the ClrDebugState. Only the contract infrastructure should be doing this but see below for more details. + +### <a name="2.10.5"/>2.10.5 Do not make unscoped changes to the ClrDebugState. + +The ClrDebugState is the per-thread data structure that houses all of the flag bits set and tested by contracts (i.e. NOTHROW, NOTRIGGER.). You should never modify this data directly. Always go through contracts or the specific holders (such as GCX_NOTRIGGER.) + +This data is meant to be changed in a scoped manner only. In particular, the CONTRACT destructor always restores the _entire_ ClrDebugState from a copy saved on function entry. This means that any net changes made by the function body itself will be wiped out when the function exits via local _or_ non-local control. The same caveat is true for holders such as GCX_NOTRIGGER. + +### <a name="2.10.6"/>2.10.6 For more details... + +See the big block comment at the start of [src\inc\contract.h][contract.h]. + +## <a name="2.11"/>2.11 Is your code DAC compliant? + +At a high level, DAC is a technique to enable execution of CLR algorithms from out-of-process (eg. on a memory dump). Core CLR code is compiled in a special mode (with DACCESS_COMPILE defined) where all pointer dereferences are intercepted. + +Various tools (most notably the debugger and SOS) rely on portions of the CLR code being properly "DACized". Writing code in this way can be tricky and error-prone. Use the following references for more details: + +- The best documentation is in the code itself. See the large comments at the top of [src\inc\daccess.h](https://github.com/dotnet/coreclr/blob/master/src/inc/daccess.h). diff --git a/Documentation/coding-guidelines/clr-jit-coding-conventions.md b/Documentation/coding-guidelines/clr-jit-coding-conventions.md new file mode 100644 index 0000000000..0bab27616a --- /dev/null +++ b/Documentation/coding-guidelines/clr-jit-coding-conventions.md @@ -0,0 +1,2001 @@ +# CLR JIT Coding Conventions +May 2015 + +# Overview + +Consistent code conventions are important for several reasons: + +* *Most importantly:* To make it easy to read and understand the code. Remember: you may be the only one writing the code initially, but many people will need to read, understand, modify, and fix the code over its lifetime. These conventions attempt to make this task much easier. If the code consistently follows these standards, it improves developer understanding across the entire source base. +* To make it easy to debug the code, with both the Visual Studio and windbg debuggers. It should be easy to set breakpoints, view locals, and display and view data structures. +* To make it easier to search and browse the code, both with Visual Studio IntelliSense, and with simple tools like "grep" and Notepad. This implies, for example, that names must be unique, and not require C++ parser intelligence (for example, Visual Studio IntelliSense) to distinguish. +* To attempt to improve code quality through consistency, and requiring patterns that are less likely to result in bugs either initially, or after code modification. + +For these reasons, C++ code that is part of the Common Language Runtime (CLR) Just-In-Time compiler (JIT) will follow these conventions. + +Note that these conventions are different from the CLR C++ Coding Conventions, documented elsewhere and used for the VM code, though they do have strong similarities. This is due to historical differences in the JIT and VM code, the teams involved in forming these conventions, as well as to technical differences in the code itself. + +> Note: the JIT currently doesn't follow some of these conventions very widely. The non-conformant code should be updated, eventually. + +> Note: we now use jit-format to format our code. All changes it makes supersede the conventions in this doc. Please see the [jit-format documentation](https://github.com/dotnet/jitutils/blob/master/doc/getstarted.md#formatting-jit-source) for instructions on running jit-format. + +# How to use this document + +* All new code written in the JIT should adhere to these conventions. +* Existing code that does not follow these conventions should be converted to these conventions when it is modified. + * Typically, conversion to these conventions would be done on a function basis. + * You need to balance this suggestion against the very real value of submitting a minimal change for any individual fix or feature. Consider doing convention changes as a separate change, to avoid polluting the changes for a bug fix (or other change) with convention changes (which would make it harder to identify exactly which changes are strictly required for the bug fix). +* Code reviewers should look for adherence to the conventions. + +# Contents + +* [4 Principles](#4) +* [5 Spaces, not tabs](#5) +* [6 Source code line width](#6) +* [7 Commenting](#7) + * [7.1 General](#7.1) + * [7.1.1 Comment style](#7.1.1) + * [7.1.2 Spelling and grammar](#7.1.2) + * [7.1.3 Bug IDs](#7.1.3) + * [7.1.4 Email names](#7.1.4) + * [7.1.5 TODO](#7.1.5) + * [7.1.6 Performance](#7.1.6) + * [7.2 File header comment](#7.2) + * [7.3 Commenting code blocks](#7.3) + * [7.4 Commenting variables](#7.4) + * [7.5 Commenting `#ifdefs`](#7.5) +* [8 Naming Conventions](#8) + * [8.1 General](#8.1) + * [8.2 Hungarian or other prefix/postfix naming](#8.2) + * [8.3 Macro names](#8.3) + * [8.4 Local variables](#8.4) + * [8.5 Global variables](#8.5) + * [8.6 Function parameters](#8.6) + * [8.7 Non-static C++ member variables](#8.7) + * [8.8 Static member variables](#8.8) + * [8.9 Functions, including member functions](#8.9) + * [8.10 Classes](#8.10) + * [8.11 Enums](#8.11) +* [9 Function Structure](#9) + * [9.1 In a header file](#9.1) + * [9.1.1 Comments for function declarations](#9.1.1) + * [9.2 In an implementation file](#9.2) + * [9.2.1 Function size](#9.2.1) + * [9.3 Function definitions](#9.3) + * [9.4 Function header comment](#9.4) + * [9.4.1 Example](#9.4.1) + * [9.5 Specific function information](#9.5) + * [9.5.1 Constructor with member initialization list](#9.5.1) +* [10 Local Variable Declarations](#10) + * [10.1 Pointer declarations](#10.1) +* [11 Spacing](#11) + * [11.1 Logical and arithmetic expressions](#11.1) + * [11.2 Continuing statements on multiple lines](#11.2) + * [11.3 Function call](#11.3) + * [11.4 Arrays](#11.4) +* [12 Control Structures](#12) + * [12.1 Braces for `if`](#12.1) + * [12.2 Braces for looping structures](#12.2) + * [12.3 `switch` statements](#12.3) + * [12.4 Examples](#12.4) +* [13 C++ Classes](#13) +* [14 Preprocessor](#14) + * [14.1 Conditional compilation](#14.1) + * [14.1.1 `#if FEATURE`](#14.1.1) + * [14.1.2 Disabling code](#14.1.2) + * [14.1.3 Debug code](#14.1.3) + * [14.2 `#define` constants](#14.2) + * [14.3 Macro functions](#14.3) + * [14.3.1 Macro functions versus C++ inline functions](#14.3.1) + * [14.3.2 Line continuation](#14.3.2) + * [14.3.3 Multi-statement macro functions](#14.3.3) + * [14.3.4 Control flow](#14.3.4) + * [14.3.5 Scope](#14.3.5) + * [14.3.6 Examples](#14.3.6) +* [15 Language Usage Rules](#15) + * [15.1 C/C++ general](#15.1) + * [15.1.1 Casts](#15.1.1) + * [15.1.2 Globals](#15.1.2) + * [15.1.3 `bool` versus `BOOL`](#15.1.3) + * [15.1.4 `NULL` and `nullptr`](#15.1.4) + * [15.1.5 Use of zero](#15.1.5) + * [15.1.6 Nested assignment](#15.1.6) + * [15.1.7 `if` conditions](#15.1.7) + * [15.1.8 `const`](#15.1.8) + * [15.1.9 Ternary operators](#15.1.9) + * [15.1.10 Use of `goto`](#15.1.10) + * [15.2 Source file organization](#15.2) + * [15.3 Function declarations](#15.3) + * [15.3.1 Default arguments](#15.3.1) + * [15.3.2 Overloading](#15.3.2) + * [15.3.3 Enums versus primitive parameter types](#15.3.3) + * [15.3.4 Functions returning pointers](#15.3.4) + * [15.3.5 Reference arguments](#15.3.5) + * [15.3.6 Resource release](#15.3.6) + * [15.3.7 OUT parameters](#15.3.7) + * [15.4 STL usage](#15.4) + * [15.5 C++ class design](#15.5) + * [15.5.1 Public data members](#15.5.1) + * [15.5.2 Friend functions](#15.5.2) + * [15.5.3 Constructors](#15.5.3) + * [15.5.4 Destructors](#15.5.4) + * [15.5.5 Operator overloading](#15.5.5) + * [15.5.6 Copy constructor and assignment operator](#15.5.6) + * [15.5.7 Virtual functions](#15.5.7) + * [15.5.8 Inheritance](#15.5.8) + * [15.5.9 Global class objects](#15.5.9) + * [15.6 Exceptions](#15.6) + * [15.7 Code tuning for performance optimization](#15.7) + * [15.8 Obsoleting functions, classes and macros](#15.8) + +# <a name="4"/>4 Principles + +As stated above, the primary purpose of these conventions is to improve readability and understandability of the source code, by making it easier for any developer, now or in the future, to easily read, understand, and modify any portion of the source code. + +It is assumed that developers should be able to use the Visual Studio editor and debugger to the fullest extent possible. Thus, the conventions should allow us to leverage Visual Studio IntelliSense, editing and formatting, debugging, and so forth. The conventions will not preclude the use of other editors. For example, function declaration commenting style should be such that IntelliSense will automatically display that comment when typing that function name at a use site. Indenting style should be such that using Visual Studio automatic formatting rules creates correctly formatted code. + +# <a name="5"/>5 Spaces, not tabs + +Use spaces, not tabs. Files should not contain tab characters. + +Indenting is 4 characters per indent level. + +In Visual Studio, go to "Tools | Options ... | Text Editor | All Languages | Tabs", edit the tab size setting to be 4 spaces, and select the "Insert spaces" radio-button to enable conversion of tabs to spaces. + +# <a name="6"/>6 Source code line width + +A source code line should be limited to a reasonable length, so it fits in a reasonably-sized editor window without scrolling or wrapping. Consider 120 characters the baseline of reasonable, adjusted for per-site judgment. + +> Rationale: Modern widescreen monitors can easily display source files much wider than 120 characters, however we don't encourage (or allow) that for a number of reasons: +> +> 1. Very long lines tend to make the code more difficult to read +> 2. If the need for long lines is because there is a lot of scope-based indentation, that is an indication that refactoring is necessary to reduce the number of nested scopes. +> 3. Many people place other windows side-by-side with source code (such as additional source code windows, or Visual Studio tool windows like the Code Definition Window), or use side-by-side "diff" programs. Thus, making (most) code visible when viewed side-by-side, without scrolling, is advantageous. +> 4. Even if there are occasional uses for wide lines, it is expected that most lines will be much shorter, leaving a considerable amount of wasted space that could be used for other purposes (see #3). + +Many editors support display of a vertical line at a specified column position. Enable this in your editor to easily know when you write past the specified maximum column position. Visual Studio has this feature if you install the "Productivity Power Tools": https://visualstudiogallery.msdn.microsoft.com/3a96a4dc-ba9c-4589-92c5-640e07332afd. + +# <a name="7"/>7 Commenting + +## <a name="7.1"/>7.1 General + +A comment should never just restate what the code does. Instead it should answer a question about the code, such as why, when, or how. Comments that say we must do something need to also state why we must do this. + +Avoid using abbreviations or acronyms; it harms readability. + +### <a name="7.1.1"/>7.1.1 Comment style + +We prefer end-of-line style `//` comments to original C `/* */` comments. + +One important exception is when adding a small comment within an argument list to help document the argument, e.g., `PrintIt(/* AlignIt */ true);` (However, see section FIXTHIS for the suggested alternative to the form.) + +### <a name="7.1.2"/>7.1.2 Spelling and grammar + +Check for spelling and grammar errors in your comments: just because you can understand the comment when you write it doesn't mean somebody else will parse it in the same way. Carefully consider the poor reader, especially those for whom English is not their first language. + +### <a name="7.1.3"/>7.1.3 Bug IDs + +Don't put the bug or issue identifier (ID) of a fixed bug or completed feature in the source code comments. Such IDs become obsolete when (and not if) the bug tracking system changes, and are an indirect source of information. Also, understanding the bug report might be difficult in the absence of fresh context. Rather, the essence of the bug fix should be distilled into an appropriate comment. The precise condition that a case covers should be specified in the comment, as for all code. + +In particular, putting a bug ID in a comment is often a shortcut people use to avoid writing a complete, descriptive comment about the code. The code and comments should stand alone. + +Bug IDs of active bugs may be used in the source code to prevent other people from having to debug the problem again, only to figure out that a bug has already been opened for it. These bug IDs are expected to be removed as soon as the bug is fixed. + +One thing that would be useful is for a particular case in code to be associated with a test case that exercises the case. This only makes sense if the case in question is localized to one or a few locations in the code, not pervasive and spread throughout the code. However, we don't currently have a good mechanism for referencing test cases (or other external metadata). + +### <a name="7.1.4"/>7.1.4 Email names + +Email names or full names should not be used in the source code as people move on to other projects, leave the company, leave another company when working on the JIT in the open source world, or simply stop working on the JIT for some reason. For example, a comment that states, "Talk to JohnDoe to understand this code" isn't helpful after JohnDoe has left the company or is otherwise not available. + +### <a name="7.1.5"/>7.1.5 TODO + +"TODO" comments in the code should be used to identify areas in the code that: + +* May require tuning for code quality (runtime performance) or throughput. +* Need some cleanup for better maintainability or readability. +* Are either known or thought possibly to have a bug. + +Tracking bugs should be associated with the TODO items, but they are also in the source so that they are visible to the Open Source community, which may not have access to the same bug database. + +This is the format to be used: + +```c++ +// TODO[-Arch][-Platform][-CQ|-Throughput|-Cleanup|-Bug|-Bug?]: description of the issue +``` + +* One type modifier (CQ, Throughput, Cleanup, Bug or Bug?) must be specified. +* The -Arch and -Platform modifiers are optional, and should generally specify actual architectures in all-caps (e.g. AMD64, X86, ARM, ARM64), and then in Pascal casing for Platforms and architecture classes (e.g. –ARMArch, –LdStArch, –XArch, –Unix, –Windows). +* This list is not intended to be exhaustive. + +Examples: +```c++ + // TODO-LdStArch-Bug: Should regTmp be a dst on the node or an internal reg? + // Either way, it is not currently being handled by Lowering. + + // TODO-CQ: based on whether src type is aligned use movaps instead. + + // TODO-Cleanup: Add a comment about why this is unreached() for RyuJIT backend. + + // TODO-Arm64-Bug: handle large constants! Probably need something like the ARM + // case above: if (arm_Valid_Imm_For_Instr(ins, val)) ... +``` + +### <a name="7.1.6"/>7.1.6 Performance + +Be sure to comment the performance characteristics (memory and time) of an API, class, sensitive block of code, line of code that looks simple but actually does something complex, etc. + +## <a name="7.2"/>7.2 File header comment + +C and C++ source files (header files and implementation files) must include a file header comment at the beginning of the file that describes the file, gives the file owner, and gives some basic information about the purpose of the file, related documents, etc. The format of this header is as follows: + +```c++ +// +// Copyright (c) Microsoft. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. +// +// <summary of the purpose of the file, description of the component, overview of the architecture and API, etc.> +// +``` + +Major components usually occupy their own file. The top of the file is a good place to document the design of that component, including any information that would be helpful to a new reader of the code. A reference can be made to an actual design and implementation document (specification), but that document must be co-located with the source code, and not on some server that is unlikely to remain active for as long as the source will live. + +## <a name="7.3"/>7.3 Commenting code blocks + +Properly commented code blocks allow code to be scanned through and read like a book. There are a number of different commenting conventions that can be used for blocks of code, ranging from comments with significant whitespace to help visually distinguish major code segments to follow, down to single end-of-line comments to annotate individual statements or expressions. Choose the comment style that creates the most readable code. + +Major blocks can be commented using the following convention (in each example, the "<comment>" line represents any number of lines with actual comment text): + +```c++ +<blank line> +// +// <comment> +// +<blank line> +<code> +``` + +Or: + +```c++ +<blank line> +// <comment> +<blank line> +<code> +``` + +Minor blocks can be commented using the following convention: + +```c++ +// <comment> +<code> +``` + +Beware, however, of creating a visually dense block of comments-and-code without whitespace that is difficult to read. + +If the code line is short enough, and the comment is short enough, it can all be on the same line: + +```c++ +<code> // <comment> +``` + +Major comments should be used to comment a body of code, and minor comments should be used to clarify the intent of sub-bodies within a main body of code. + +Comments should be written with proper punctuation, including periods to end sentences. Be careful to check the spelling of your comments. + +The following example illustrates correct usage of major and minor comments in a body of code: + +```c++ +CorElementType MetaSig::NextArgNormalized() +{ + // Cache where the walk starts. + m_pLastType = m_pWalk; + + // + // Now get the next element if one exists. + // + + if (m_iCurArg == GetArgCount()) + { + // We are done walking the entire signature. + return ELEMENT_TYPE_END; + } + else + { + // Skip the current argument and go the next. + m_iCurArg++; + CorElementType mt = m_pWalk.PeekElemType(m_pModule, + m_typeContext); + m_pWalk.SkipExactlyOne(); + return mt; + } +} +``` + +## <a name="7.4"/>7.4 Commenting variables + +All global variables and C++ class data members must be commented at the point of declaration. + +It is recommended that local variable declarations are also commented. One should not have to scan all uses of the variable to determine the exact meaning of the variable, including possible values, when it may or may not be initialized, etc. If the name of the variable is sufficient to describe the intent of the variable, then variable comments are unnecessary. However, do keep in mind that someone reading the code for the first time will not know all the rules you might have had in mind. + +The following conventions should be used: + +```c++ +// <comment> +<variable declaration> +``` + +Or, for sufficiently concise comments: + +```c++ +<variable declaration> // <comment> +``` + +The following variable declarations provide an example of how to properly follow this convention when commenting variables: + +```c++ +class Thread +{ + // This is the maximum stack depth of allowed for any thread. + // This can be set by a config setting, but might be overridden + // if it's out of range. + static int s_MaxStackDepth; + + bool m_fStressHeap; // Are we doing heap-stressing? +}; +``` + +## <a name="7.5"/>7.5 Commenting `#ifdefs` + +Do specify the macro name in a comment at the end of the closing `#endif` of a long or nested `#if`/`#ifdef`. + +```c++ +#ifdef VIEWER +#ifdef MAC +... +#endif // MAC +#else // !VIEWER +... +#endif // !VIEWER +``` + +This is so, when you see a page of code with just the `#endif` somewhere in the page, but the `#ifdef` is somewhere off the top of the page, you don't need to page back to see if the `#ifdef`'ed code is relevant to you; you can just look at the comment on the `#endif`. It also helps when there is nesting of `#ifdef`s, as above. + +The comment on a `#else` should indicate the condition that will cause the following code to be compiled. The comment on a `#endif` should indicate the condition that caused the immediately preceding code block to be compiled. In both cases, don't simply repeat the condition of the original `#if` – that does help with matching, but doesn't help interpret the exact condition of interest. + +Right: +```c++ +#if defined(FOO) && defined(BAR) + ... +#endif // defined(FOO) && defined(BAR) +#ifdef FOO + ... +#else // !FOO + ... +#endif // !FOO +``` + +Wrong: +```c++ +#ifdef FOO + ... +#else // FOO + ... +#endif // FOO +``` + +Do comment why the conditional `#ifdef` is needed (just as you might comment an `if` statement). + +A comment for code within an `#ifdef` should also appear within the `#ifdef`. For example: + +Right: +```c++ +#ifdef _TARGET_ARM_ + // This case only happens on ARM... + if (...) + ... +#endif // _TARGET_ARM_ +``` + +Wrong: +```c++ +// This case only happens on ARM... +#ifdef _TARGET_ARM_ + if (...) + ... +#endif // _TARGET_ARM_ +``` + +# <a name="8"/>8 Naming Conventions + +Names should be sufficiently descriptive to immediately indicate the purpose of the function or variable. + +## <a name="8.1"/>8.1 General + +It is useful for names to be unique, to make it easier to search for them in the code. For example, it might make sense for every class to implement a debug-only `dump()` function. It's a simple name, and descriptive. However, if you do a simple textual search ("grep" or "findstr", or "Find in Files" in Visual Studio) for "dump", you will find far too many to be useful. Additionally, Visual Studio IntelliSense often gets confused when using "Go To Reference" or "Find All References" for such a common word that appears in many places (especially for our imprecise IntelliSense projects), rendering IntelliSense browsing also less useful. + +Functions and variables should be named at their level-of-intent. + +Good: +```c++ +int ClientConnectionsRemaining[MAX_CLIENT_CONNECTIONS + 1]; +``` + +Bad: +```c++ +int Connections[MAX]; +``` + +Do not use negative names, as thinking about the negation condition becomes difficult. + +Good: +```c++ +bool isVerificationEnabled, allowStressHeap; +``` + +Bad: +```c++ +bool isVerificationDisabled, dontStressHeap; +``` + +## <a name="8.2"/>8.2 Hungarian or other prefix/postfix naming + +We do not follow "Hungarian" naming, with a detailed set of name prefix requirements. We do suggest or require prefixes in a few cases, described below. + +* Global variables should be prefixed by "g_". (Note that the JIT has very few global variables.) +* Non-static C++ class member variables should be prefixed by "m_". +* Static C++ class member variables should be prefixed by "s_". + +Two common Hungarian conventions that we do not encourage are: + +* Prefixing boolean variables by "f" (for "flag"). Instead, consider using an appropriate verb prefix, such as "is" or "has". For example, `bool isFileEmpty`. +* Prefixing pointer variables by "p" (one "p" for each level of pointer). There may be situations where this helps clarity, but it is not required. + +It is often helpful to choose a short, descriptive prefix for all members of a class, e.g. "lv" for local variables, or "emit" for the emitter. This short prefix also helps make the name unique, and easier to find with "grep". Thus, you might have "m_lvFoo" for a non-static member variable of a class that is using a "lv" prefix. + +## <a name="8.3"/>8.3 Macro names + +All macros and macro constants should have uppercase names. Words within a name must be separated by underscores. The following statements illustrate some macro and macro constant names: + +```c++ +#define PAGE_SIZE 4096 +#define CONTAINING_RECORD(_address, _type, _field) \ + ((_type*)((LONG)(_address) - \ + (LONG)(&((_type*)0)->_field))) +``` + +The use of inline functions is strongly encouraged instead of macros, for type-safety, to avoid problems of double evaluation of arguments, and to ease debugging. + +The first example here, PAGE_SIZE, should probably be written instead as: + +```c++ +const int g_PageSize = 4096; +``` + +which eliminates the need for a `#define` at all. + +Macro parameter names should start with one leading underscore. No other names (local variables names, member names, class names, parameters names, etc.) should begin with one leading underscore. This prevents problems from occurring where a macro parameter name accidentally matches a variable name at the point of macro expansion. + +All macro parameter names should be surrounded by parentheses in the macro definition to guard against unintended token interactions. + +All this being said, there still are some cases where macros are useful, such as the JIT phase list or instruction table, where data is defined in a header file by a series of macro functions that must be defined before #including the header file. This allows the macro function to be defined in different ways and the header file to be included multiple times to create different definitions from the same data. Look, for example, at instrsxarch.h. (Unfortunately, this technique confuses Visual Studio Intellisense.) + +## <a name="8.4"/>8.4 Local variables + +Local function variables should be named using camelCasing: + +* Multiple words should be concatenated directly, without using underscores in between. +* The first letter of all words (except the first) should be upper case. The other letters should be lower case. +* The first letter of the first word should be upper case if there a (lower-case) prefix. Otherwise it should be lower case. +* Acronyms should be treated as words, and only the first letter may be upper case. + +The following variable declarations illustrate variable names that adhere to this convention: + +```c++ +CorElementType returnType; // simple camelCasing +TypeHandle thConstraintType; // constraint for the type argument +MethodDesc* pOverridingMD; // the override for the given method +``` + +## <a name="8.5"/>8.5 Global variables + +Global variables follow the same rules as local variable names, but should be prefixed by a "g_". The following global variable declarations illustrate variable names that adhere to this convention: + +```c++ +EEConfig* g_Config; // Configuration manager interface +bool g_isVerifierEnabled; // Is the verifier enabled? +``` + +## <a name="8.6"/>8.6 Function parameters + +Function parameters follow the same rules as local variables + +```c++ +int Point(int x, int y) : m_x(x), m_y(y) {} +``` + +## <a name="8.7"/>8.7 Non-static C++ member variables + +Non-static C++ member variables should follow the same rules as local variable names, but should be prefixed by "m_". + +```c++ +class MetaSig +{ + // The module containing the metadata of the signature blob + Module* m_Module; + + // The size of the signature blob in bytes. This is kSizeNotSpecified if + // the size is not specified. + UINT32 m_SigSize; + + // This contains the offsets of the stack arguments. + // It is valid only after the entire signature has been walked. + // It contains the offset of only the first few arguments. + short m_StackOffsets[MAX_CACHED_SIG_SIZE + 1]; +}; +``` + +## <a name="8.8"/>8.8 Static member variables + +Static C++ member variables should follow the same rules as non-static member variable names, but should be prefixed by "s_" instead of "m_". + +```c++ +class Thread +{ + static int s_MaxThreadRecord; // Set by config file + static HANDLE s_FakeThreadHandle; // Initialized after startup +}; +``` + +## <a name="8.9"/>8.9 Functions, including member functions + +Functions should be named in PascalCasing format: + +* Multiple words should be concatenated directly, without using underscores in-between. +* The first letter of all words (including the first) should be upper case. The other letters should be lower case. + +The following C++ method declarations illustrate method names that adhere to this convention: + +```c++ +IExecutionEngine* GetExecutionEngine(); +void SortArgs(); +void RecordStkLevel(unsigned stkLvl); +``` + +It is also acceptable, and in fact encouraged, to prefix such names with a "tag" related to the function name's component or group, such as: + +```c++ +unsigned lvaGetMaxSpillTempSize(); +bool lvaIsPreSpilled(unsigned lclNum, regMaskTP preSpillMask); +``` + +This makes it more likely that the names are globally unique. The tag can start either with a lower or upper case. + +## <a name="8.10"/>8.10 Classes + +C++ class names should be named in PascalCasing format. A "C" prefix should not be used (thus, use `FooBar`, not `CFooBar` as is used in some conventions). The following C++ class declaration demonstrates proper adherence to this convention: + +```c++ +class SigPointer : public SigParser +{ + ... +}; +``` + +Interfaces should use a prefix of "I" (capital letter "i"): + +```c++ +class ICorStaticInfo : public virtual ICorMethodInfo +{ + ... +}; +``` + +## <a name="8.11"/>8.11 Enums + +Enum type names are PascalCased, like function names. + +Enum values should be all-caps, and prefixed with a short prefix that is unique to the enum. This makes them easier to "grep" for. + +```c++ +enum RoundLevel +{ + ROUND_NEVER = 0, // Never round + ROUND_CMP_CONST = 1, // Round values compared against constants + ROUND_CMP = 2, // Round comparands and return values + ROUND_ALWAYS = 3, // Round always + + COUNT_ROUND_LEVEL, + DEFAULT_ROUND_LEVEL = ROUND_NEVER +}; +``` + +> The JIT is currently very inconsistent with respect to enum type and element naming standards. Perhaps we should adopt the VM standard of prefixing enum names with "k", and using PascalCasing for names, with a Hungarian-style per-enum prefix. For example, kRoundLevelNever or kRLNever / kRLCmpConst. + +# <a name="9"/>9 Function Structure + +The term "function" here refers to both C-style functions as well as C++ member functions, unless otherwise specified. + +There are two primary file types: + +* A header file. This is named using the .h suffix. It contains declarations, especially those declarations needed by other components. (Declarations only needed by a single file should generally be placed in the implementation file.) +* An implementation file. This contains function definitions, etc. It is named using the .cpp suffix. + +Some code uses a third type of file, an "implementation header file" (or .hpp file) in which inline functions are put, which is `#include`ed into the appropriate .h file. We don't use that. Instead, we trust that our retail (ship) build type will use the compiler's whole program optimization feature (such as link-time code generation) to do cross-module inlining. Thus, we organize the implementations logically without worrying about inlining. + +It is acceptable to put very small inline member function implementations directly in the header file, at the point of declaration. + +## <a name="9.1"/>9.1 In a header file + +This is the format for the declaration of a function in a header file. + +For argument lists that fit within the max line length: + +```c++ +[static] [virtual] [__declspec(), etc] +return-type FunctionName(<type-name>* <argument-name>, ...) [const]; +``` + +For argument lists that don't fit within the max line length, or where it is deemed to be more readable: + +```c++ +[static] [virtual] [__declspec(), etc] +return-type FunctionName(<type-name>* <argument-name>, + <type-name> <argument-name>, + ... + <type-name> <argument-name>) [const]; +``` + +For multi-line function declarations, both argument type names and argument names should be aligned. + +Most declarations with more than one argument will be more readable by using the second format. + +Functions with no arguments should just use an empty set of parenthesis, and should not use `void` for the argument list. + +Right: +```c++ +void Foo(); +``` + +Wrong: +```c++ +void Foo( ); // Don't put space between the parentheses +void Foo(void); // Don't use "void" +``` + +All arguments can be on the same line if the line fits within the maximum line length. + +Right: +```c++ +void Foo(int i); +T Min<T>(T a, T b); +``` + +### <a name="9.1.1"/>9.1.1 Comments for function declarations + +Function declarations in a header file should have a few lines of documentation using single-line comments, indicating the intent of the prototyped function. + +Detailed documentation should be saved for the Function Header Comment above the function definition in the implementation file. This makes it easy to scan the API of the class in the header file. + +However, note that Visual Studio IntelliSense will pick up the comment that immediately precedes the function declaration, for use when calling the function. Thus, the declaration comment should be detailed enough to aid writing a call-site using IntelliSense. + +```c++ +class MetaSig +{ +public: + + // Used to avoid touching metadata for mscorlib methods. + MetaSig(MethodDesc* pMD, + BinderMethodID methodId); + + // Returns type of current argument, then advances the + // argument index. + CorElementType NextArg(); + + // Returns type of current argument. Primitive valuetypes + // like System.Int32 are normalized to the form "int32". + CorElementType NextArgNormalized(UINT32* pSize); + + // Checks if the calling convention of pSig is varargs. + static + bool IsVarArg(Module* pModule, + PCCOR_SIGNATURE pSig); +}; +``` + +## <a name="9.2"/>9.2 In an implementation file + +Typically for each header file in the project there will be an implementation file that contains the function implementations and it is named using the .cpp suffix. + +The signature of a function definition in the implementation file should use the same format used in the header file. + +Generally the order of the functions in the implementation file should match to the order in the header file. + +### <a name="9.2.1"/>9.2.1 Function size + +It is recommended that functions bodies (from the opening brace to the closing brace) are no more than 200 lines of text (including any empty lines and lines with just a single brace in the function body). A large function is difficult to scan and understand. + +Use your best judgment here. + +## <a name="9.3"/>9.3 Function definitions + +If the header file uses simple comments for the function prototypes, then the function definition in the implementation file should include a full, descriptive function header. If the header file uses a full function header comments for the function prototypes, then the function definition in the implementation file can use a few descriptive lines of comments. That is, there should be a full descriptive comment for the function, but only in one place. The recommendation is to place the detailed comments at the definition site. One primary reason for this choice is that most code readers spend much more time looking at the implementation files than they do looking at the header files. + +Note that for virtual functions, the declaration site for the virtual function should provide a sufficient comment to specify the contract of that virtual function. The various implementation sites should provide implementation-specific details. + +Default argument values may be repeated as comments. Example: + +```c++ +void Foo(int i /* = 0 */) +{ + <function body> +} +``` + +Be careful to update all call sites when changing the default parameters of a function! + +Static member functions must repeat the "static" keyword as a comment. Example: + +```c++ +// static +BOOL IsVarArg(Module* pModule, + PCCOR_SIGNATURE pSig) +{ + <function body> +} +``` + +## <a name="9.4"/>9.4 Function header comment + +All functions, except trivial accessors and wrappers, should have a function header comment which describes the behavior and the implementation details of the function. The format of the function header in an implementation file is as shown below. + +Within the comment, argument names (and other program-related names) should be surrounded by double quotes, to emphasize that they are program objects, and not simple English words. This helps clarify those cases where a function argument might be parsed (by a human) in either way. + +Any of the sections that do not apply to a method may be skipped. For example, if a method has no arguments, the "Arguments" section can be left out. + +If you can formulate any assumptions as asserts in the code itself, you should do so. The "Assumptions" section is intended to encapsulate things that are harder (or impossible) to formulate as asserts, or to provide a place to write a more easily read English description of any assumptions that exist, even if they can be written with asserts. + +```c++ +//------------------------------------------------------------------------ +// <Function name>: <Short description of the function> +// +// Arguments: +// <argument1-name> - Description of argument 1 +// <argument2-name> - Description of argument 2 +// ... as many as the number of function arguments +// +// Return Value: +// Description of the values this function could return +// and under what conditions. When the return value is a +// described as a function of the arguments, those arguments +// should be mentioned specifically by name. +// +// Assumptions: +// Any entry and exit conditions, such as required preconditions of +// data structures, memory to be freed by caller, etc. +// +// Notes: +// More detailed notes about the function. +// What errors can the function return? +// What other methods are related or alternatives to be considered? + +<function definition> +``` + +### <a name="9.4.1"/>9.4.1 Example + +The following is a sample of a completed function definition: + +```c++ +//------------------------------------------------------------------------ +// IsVarArg: Checks if the calling convention of the signature "pSig" is varargs. +// +// Arguments: +// pModule – The module which contains the metadata of "pSig". +// pSig – The pointer to the signature blob. +// +// Return Value: +// true – The calling convention of the method is varargs. +// false – The method is not varargs, or there was some error. +// +// Assumptions: +// The caller must have ensured that the format of "pSig" is +// consistent, and that the size of the signature does not extend +// past the end of the metadata blob. +// +// Notes: +// Call-site signature blobs include ELEMENT_TYPE_SENTINEL. +// This method does not check for the presence of the sentinel. + +// static +BOOL MetaSig::IsVarArg(Module* pModule, + PCCOR_SIGNATURE pSig) +{ + <function body> +} +``` + +## <a name="9.5"/>9.5 Specific function information + +### <a name="9.5.1"/>9.5.1 Constructor with member initialization list + +This is the format to use for specifying the member initialization list + +```c++ +ClassName::ClassName(<type-name>* <argument-name>, + <type-name2000> <argument-name>, + ... + <type-name> <argument-name>) + : m_member1(val1) + , m_member2(val2) + , m_member3(val3) +{ + <function_body> +} +``` + +Note that the order of the initializers is defined by C++ to be member declaration order, and some compilers will report an error if the order is incorrect. + +# <a name="10"/>10 Local Variable Declarations + +Generally, variables should be declared at or close to the location of their first initialization, especially for large functions. For small functions, it is fine to declare all the locals at the start of the method. + +Each variable should be declared on a separate line. + +It is preferable to provide an initialization of a variable when it is declared. + +Variable names should be unique within a function. This is to make it easier to do a simple textual search for the declaration and all the uses of a name in a function, without worrying about scoping. + +Variables that are conditionally assigned or passed as OUT parameters to a function must be declared close to their first use. Values that are passed by reference as OUT parameters should be initialized prior to being passed. + +Variable names should generally be aligned, as are function parameter names, to improve readability. Variable initializers should also be aligned, if it improves readability. + +```c++ +char* name = getName(); +int iPosition = 0; +int errorCode = 0; +FindIllegalCharacter(name, &iPosition, &errorCode); +``` + +## <a name="10.1"/>10.1 Pointer declarations + +For pointer declaration, there should be no space between the type and the `*`, and one or more spaces between the `*` and the following symbol being declared. This emphasizes that the `*` is logically part of the type declaration (even though the C/C++ parser binds the `*` to the name). + +Right: +```c++ +int* pi; +int* pq; +``` + +Wrong: +```c++ +int * pi; +int * pi; +int *pi; +int *pi; // the alignment is on the name, not the * +int *pi, *pq; +``` + +Each local pointer variable must be declared on its own line. Combined with the fact that most local variables will be declared and initialized on the same line, this naturally prevents the confusing syntax of mixing pointer and non-pointer declarations like: + +```c++ +int* piVal1, i2, *piVal3, i4; +``` + +For return types, there should be no space between the type and the `*`, and one or more spaces between the `*` and the function name. + +Right: +```c++ +Module* GetModule(); +``` + +Wrong: +```c++ +Module *GetModule(); // no space between * and function name +Module * GetModule(); // one space between type and * +``` + +For pointer types used in casts, there should be no space between the type and the `*`. + +Right: +```c++ +BYTE* pByte = (BYTE*)pBuffer; +``` + +Wrong: +```c++ +BYTE * pByte = (BYTE *)pBuffer; // one space between type and * +``` + +Reference variables should use similar spacing. + +Right: +```c++ +void Foo(const BYTE& byte); +``` + +Double stars should appear together with no spaces. + +Right: +```c++ +int** ppi; +``` + +Wrong: +```c++ +int ** ppi; +int * * ppi; +``` + +# <a name="11"/>11 Spacing + +## <a name="11.1"/>11.1 Logical and arithmetic expressions + +The following example illustrates correct spacing of parentheses for expressions: + +```c++ +if (a < ((!b) + c)) +{ + ... +} +``` + +There is a space between the `if` and the open parenthesis. This is used to distinguish C statements from functions (where the name and open parenthesis have no intervening space). + +There is no space after the open parenthesis following the `if` or before the closing parenthesis. + +Binary operators are separated on both sides with spaces. + +There should be parentheses around all unary and binary expressions if they are contained within other expressions. We prefer to over-specify parentheses instead of requiring developers to memorize the complete C++ precedence rules. This is especially true for `&&` and `||`, whose precedence relationship is often forgotten. + +Complex expressions should be broken down to use local variables to express and identify the semantics of the individual components. + +While `sizeof` is a built-in operator, it does not require a space between the `sizeof` and the open parenthesis. We require the argument to `sizeof` to be surrounded by parentheses. + +Wrong: +```c++ +if(a < b) // no space after "if" +if (a&&b) // no space around operator "&&" +if (a || ( b && c )) // space after "(" and before ")" +if (a < b + c) // binary expression "b + c" is not parenthesized +x = sizeof f; // "sizeof" requires parentheses: sizeof(f) +``` + +## <a name="11.2"/>11.2 Continuing statements on multiple lines + +When wrapping statements, binary operators are left hanging after the left expression, so that the continuation is obvious. The right expression is indented to match the left expression. Additional spaces between the parentheses may be inserted as necessary in order to clarify how a complex conditional expression is expected to be evaluated. In fact, additional spaces are encouraged so that it is easy to read the condition. + +Right: +```c++ +if ((condition1) || + ((condition2A) && + (condition2B))) + +if ( (condition1) || + ( (condition2A) && + (condition2B) ) ) + +if ( ((condition1A) || (condition1B)) && + ((condition2A) || (condition2B)) ) +``` + +Wrong: +```c++ +if ((condition1) + || ((condition2A) + && (condition2B))) + +if ((condition1) +|| ((condition2A) + && (condition2B))) +``` + +## <a name="11.3"/>11.3 Function call + +When calling a function, use the following formatting: + +```c++ +Value = FunctionName(argument1, argument2, argument3); +``` + +There is no space between the function name and the open parenthesis. + +There is no space between the open parenthesis and the first argument. + +There is no space between the last argument and the closing parenthesis. + +There is a space between every comma and the next argument. + +There is no space between an argument and the comma following it. + +If all the arguments won't fit in the maximum line-width or you wish to add per-argument comments, enter each argument on its own line. A line must either contain all the arguments, or exactly one argument. All arguments should be aligned. It is preferred that the alignment is with the first argument that comes immediately after the opening parenthesis of the function call. If that makes the call too wide for the screen, the first argument can start on the next line, indented one tab stop. This avoids potential line-length conflicts, avoids having to realign all the arguments each time the method-call expression changes, and allows per-argument comments. + +Right: +```c++ +Value = FunctionName(argument1, + (argument2A + argument2B) / 2, + *argument3, // comment about arg3 + argument4); +``` + +Acceptable: +```c++ +Value = FunctionName( + argument1, + (argument2A + argument2B) / 2, + *argument3, // comment about arg3 + argument4); + +Value = TrulyVeryLongAndVerboseFunctionNameThatTakesUpALotOfHorizontalSpace( + argument1, + (argument2A + argument2B) / 2, + *argument3, // comment about arg3 + argument4); +``` + +The following are examples of incorrect usage of spaces. + +Wrong: +```c++ +Foo( i ); // space before first argument +Foo (i); // space after function name +Foo(i,j); // space is missing between arguments +``` + +For arguments that are themselves function calls you should consider assigning them to a new temporary local variable and passing the local variable. + +There are a couple of reasons for this: + +* The C++ language allows the compiler the freedom to evaluate the arguments of a function in any order, thus making the program non-deterministic when the compiler changes. +* When debugging the program, the step into procedure becomes tedious as you have to step-in/step-out of every nested argument call before stepping into the final call. + +Right: +```c++ +GenTreePtr asgStmt = gtNewStmt(asg, ilOffset); +*pAfterStmt = fgInsertStmtAfter(block, *pAfterStmt, asgStmt); +``` + +## <a name="11.4"/>11.4 Arrays + +Array indices should not have spaces around them. + +Right: +```c++ +int val = array[i] + array[j * k]; +``` + +Wrong: +```c++ +int val = array[ i ] + array[ j * k ]; +``` + +# <a name="12"/>12 Control Structures + +The structure for control-flow structures like `if`, `while`, and `do-while` blocks is as follows: + +```c++ +// Comment about the upcoming control structure +<statement> +{ + <statement 1> + ... + <statement n> +} +``` + +The opening curly for the statement block is aligned with the preceding statement block or control-flow structure, and is on the next line. Statements within a block are indented 4 spaces. Curly braces are always required, with one exception allowed for very simple `if` statements (see below). + +Each distinct statement must be on a separate line. While this improves readability, it also allows for breakpoints to easily be set on any statement, since the debuggers use per-line source-level breakpoints. + +It is generally a good idea to leave a blank line after a control structure, for readability. + +## <a name="12.1"/>12.1 Braces for `if` + +Braces are required for all `else` blocks of all `if` statements. However, "then" blocks (the true case of an `if` statement) may omit braces if: + +* the "then" block is a single-line assignment statement, function call, `return` statement, or `continue` statement, *and* +* there is no `else` block. + +Braces are required for all other `if` statements. + +Right: +```c++ +if (x == 5) + printf("5\n"); + +if (x == 5) +{ + printf("5\n"); +} + +if (x == 5) +{ + printf("5\n"); +} +else +{ + printf("not 5\n"); +} + +if (x != 5) +{ + if (x == 6) + printf("6\n"); +} + +if (x == 5) + return; + +if (x == 5) + continue; +``` + +Wrong: +```c++ +if (x == 5) + printf("5\n"); +else + printf("not 5\n"); + +if (x == 5) + printf("5\n"); +else +{ + printf("not 5\n"); + printf("Might be 6\n"); +} + +if (x != 5) + if (x !=6) + printf("Neither 5 or 6\n"); + +if (x != 5) + for (int i = 0; i < 10; i++) + { + printf("x*i = %d\n", (x * i)); + } +``` + +## <a name="12.3"/>12.3 Braces for looping structures + +Similar spacing should be used for `for`, `while` and `do-while` statements. These examples show correct placement of braces: + +```c++ +for (int i = 0; i < 100; i++) +{ + printf("i=%d\n", i); +} + +for (int i = SomeVeryLongFunctionName(); // Each part of the "for" is aligned + SomeVeryComplexExpression(); + i++) +{ + printf("i=%d\n", i); +} + +while (i < 100) +{ + printf("i=%d\n", i); +} + +do +{ + printf("i=%d\n", i); +} +while(i < 100); +``` + +Note that a loop body *must* have braces; an empty loop body with just a semicolon cannot be used, as it can easily be missed when reading. + +Right: +```c++ +Foo* p; +for (p = start; p != q; p = p->Next) +{ + // Empty loop body +} +``` + +Right: +```c++ +for (Foo* p = start; p != q; p = p->Next) +{ + // Empty loop body +} +``` + +Wrong: +```c++ +Foo* p; +for (p = start; p != q; p = p->Next); +``` + +## <a name="12.4"/>12.4 `switch` statements + +For `switch` statements, each `case` label must be aligned to the same column as the `switch` (and the opening brace). The code body for each `case` label should be indented one level. Note that this implies that each case label must exist on its own line; do not place multiple case labels on the same line. + +A default case is always required. Use the `unreached()` macro to indicate that it is unreachable, if necessary. + +Local variables should not be declared in the scope defined by the `switch` statement. + +A nested statement block can be used for the body of a `case` statement if you need to declare local variables, especially if you need local variable initializers. The braces should be indented one level from the `case` label, so as to not be at the same level as the `switch` braces. As with all statement blocks, the statements within the block should be indented one level from the opening and closing braces. + +Fall-through between two case statements should be indicated with the `__fallthrough;` annotation. It should be surrounded by blank lines to make it maximally visible. + +`case` labels (except the first) should generally be preceded by a blank line, to increase readability. + +```c++ +// +// Comment about the purpose of the switch +// +switch (Counter) +{ +case 1: + // Comment about the action required for case 1 + [code body] + + __fallthrough; + +case 2: + // Comment about the action required for case 1 or 2 + [code body] + break; + +case 3: + { + // Comment about the action required for case 3 + <local variable declaration> + [code body] + } + break; + +default: + unreached(); +} +``` + +## <a name="12.5"/>12.5 Examples + +The following skeletal statements illustrate the proper indentation and placement of braces for control structures. In all cases, indentations consist of four spaces each. + +```c++ +// static +int MyClass::FooBar(int iArgumentOne /* = 0 */, + ULONG* pArgumentTwo) +{ + // Braces for "if" statements are generally preferred. + if (iArgumentOne == 0) + { + return 0; + } + + // All "for" loops must have braces. + for (int counter = 0; counter < 10; counter++) + { + ThenBlock (); + } + + // Simple "if" statements with assignments or function calls + // in the "then" block may skip braces. + if (counter == 0) + ThenCode(); + + // All if-else statements require braces. + if (counter == 2) + { + counter += 100; + } + else + { + counter += 200; + } + + return 0; +} +``` + +# <a name="13"/>13 C++ Classes + +The format for a C++ class declaration is as follows. + +The labels for the `public`, `protected`, and `private` sections should be aligned with the opening brace. + +The labels for the `public`, `protected`, and `private` sections should occur in this order. + +This is the format to use: + +```c++ +//-------------------------------------------- +// +// <Class name>: <Description of the class> +// +// Assumptions: +// Is the class thread-safe? How does it handle memory allocation? +// etc. +// +// Notes: +// More detailed notes about the class. +// Alternative or related types to compare +// +class <class name> +{ +public: + <optional blank line> + <public methods> + <public data members. Ideally, there will be none> + +protected: + <optional blank line> + <protected methods> + <protected data members> + +private: + <optional blank line> + <private methods> + <private data members> +}; +``` + +Example: +```c++ +//-------------------------------------------- +// +// MetaSig: encapsulate a meta-data signature blob. This could be the +// signature of functions, fields or local variables. It provides +// facilities to inspect the properties of the signature like +// calling convention and number of arguments, as well as to iterate +// the arguments. +// +// Assumptions: +// The caller must have ensured that the format of the signature is +// consistent, and that the size of the signature does not extend +// past the end of the metadata blob. +// +// Notes: +// Note that the elements of the signature representing primitive +// valuetype can be accessed either in their raw form +// (e.g., System.Int32) or in their normalized form (e.g., int32). +// Note that parsing of generic signatures requires the caller to +// provide the SigTypeContext to use to interpret the +// type arguments. +// Also look at SigPointer if you need to parse a single +// element of a signature. +// +class MetaSig +{ +public: + + // + // Constructors + // + MetaSig(PCCOR_SIGNATURE szMetaSig, + DWORD cbMetaSig, + Module* pModule); + + // Used to avoid touching metadata for mscorlib methods. + MetaSig(MethodDesc* pMD, + BinderMethodID methodId); + + // + // Argument iterators + // + + // Returns type of current argument, then advances the + // argument index. + CorElementType NextArg(); + + // Returns type of current argument. Primitive valuetypes like + // System.Int32 are normalized to the form "int32". + CorElementType NextArgNormalized(UINT32* pSize); + + // + // Helper methods + // + + // Checks if the calling convention of pSig is varargs + // two given strings. + static + BOOL IsVarArg(Module* module, + PCCOR_SIGNATURE sig); + +private: + + // The module containing the metadata of the signature blob. + Module* m_module; + + // The size of the signature blob. This is SizeNotSpecified if + // the size is not specified. + UINT32 m_sigSizeBytes; + + // This contains the offsets of the stack arguments. + // It is valid only after the entire signature has been walked. + // It contains the offset of only the first few arguments. + short m_stackOffsets[MAX_CACHED_SIG_SIZE + 1]; +}; +``` + +# <a name="14"/>14 Preprocessor + +## <a name="14.1"/>14.1 Conditional compilation + +Prefer `#if` over `#ifdef` for conditional compilation. This allows setting the macro to 0 to disable it. `#ifdef` will not work in this case, and instead requires ensuring that the macro is not defined. + +One exception: we use `#ifdef DEBUG` for debug-only code (see Section FIXME). + +Right: +```c++ +#if MEASURE_MEM_ALLOC +``` + +Wrong: +```c++ +#ifdef MEASURE_MEM_ALLOC +``` + +Note, however, that you must be diligent in only using `#if`, because `#ifdef FOO` will be true whether FOO is defined to 0 or 1! + +`#if` and other preprocessor directives should not be indented at all, and should be placed at the very start of the line. + +If you have conditional `#if`/`#ifdef` in the source, explain what they do, just like you would comment an `if` statement. + +Minimize conditional compilation by defining good abstractions, partitioning files better, or defining appropriate constants or macros. + +### <a name="14.1.1"/>14.1.1 `#if FEATURE` + +If a new or existing feature is being added or modified then use a `#define FEATURE_XXX` to both highlight the code used to implement this and to allow the JIT to be compiled both with and without the feature. + +```c++ +#define FEATURE_VALNUM_CSE 0 // disable Value Number CSE optimization logic +#define FEATURE_LEXICAL_CSE 1 // enable Lexical CSE optimization logic + +#if FEATURE_VALNUM_CSE +void Compiler::optValnumCSEinit() +``` + +Note that periodically we do need to go through and remove FEATURE_* defines that are always enabled, and will never be disabled. + +### <a name="14.1.2"/>14.1.2 Disabling code + +It is generally discouraged to permanently disable code by commenting it out or by putting `#if 0` around it, in an attempt to keep it around for reference. This reduces the hygiene of the code base over time and such disabled code is rarely actually useful. Instead, such disabled code should be entirely deleted. If you do disable code without deleting it, then you must add a comment as to why the code is disabled, and why it is better to leave the code disabled than it is to delete it. + +One exception is that it is often useful to `#if 0` code that is useful for debugging an area, but is not otherwise useful. Even in this case, however, it is probably better to introduce a COMPlus_* variable to enable the special debugging mode. + +### <a name="14.1.3"/>14.1.3 Debug code + +Use `#ifdef DEBUG` for debug-only code. Do not use `#ifdef _DEBUG` (with a leading underscore). + +Use the `INDEBUG(x)` macro (and related macros) judiciously, for code that only runs in DEBUG, to avoid `#ifdef`s. + +Use the `JITDUMP(x)` macro for printing things to the JIT dump output. Note that these things will only get printed when the `verbose` variable is set, which is when `COMPlus_JitDump=*` or when `COMPlus_JitDump=XXX` and we are JITting function XXX; or when `COMPlus_NgenDump=*` or `COMPlus_NgenDump=XXX` and we are NGENing function XXX. Do not use `JITDUMP` for all output in a debug-only function that might be useful to call from the debugger. In that case, define a function that uses `printf` (which is a JIT-specific implementation of this function), which can be called from the debugger, and invoke that function like this: + +```c++ +DBEXEC(verbose, MyDumpFunction()); +``` + +A common pattern in the JIT is the following: + +```c++ +#ifdef DEBUG + if (verbose) + printf("*************** In genGenerateCode()\n"); +#endif // DEBUG +``` + +This could be written on fewer lines as: + +```c++ +JITDUMP("*************** In genGenerateCode()\n"); +``` + +However, the former is preferred because it is trivial to set an unconditional breakpoint on the "printf" that triggers when we are compiling the function that matches what COMPlus_JitDump is set to – a very common debugging technique. Note that conditional breakpoints could be used, but they are more cumbersome, and are very difficult to get right in windbg. + +If many back-to-back JITDUMP statements are going to be used it is preferred that they be written using printf(). + +Wrong: +```c++ +JITDUMP(" TryOffset: 0x%x\n", clause.TryOffset); +JITDUMP(" TryLength: 0x%x\n", clause.TryLength); +JITDUMP(" HandlerOffset: 0x%x\n", clause.HandlerOffset); +JITDUMP(" HandlerLength: 0x%x\n", clause.HandlerLength); +``` + +Right: +```c++ +#ifdef DEBUG +if (verbose) +{ + printf(" TryOffset: 0x%x\n", clause.TryOffset); + printf(" TryLength: 0x%x\n", clause.TryLength); + printf(" HandlerOffset: 0x%x\n", clause.HandlerOffset); + printf(" HandlerLength: 0x%x\n", clause.HandlerLength); +} +#endif // DEBUG +``` + +Always put debug-only code under `#ifdef DEBUG` (or the equivalent). Do not assume the compiler will get rid of your debug-only code in a non-debug build flavor. This also documents more clearly that you intend the code to be debug-only. + +## <a name="14.2"/>14.2 `#define` constants + +Use `const` or `enum` instead of `#define` for constants when possible. The value will still be constant-folded, but the `const` adds type safety. + +If you do use `#define` constants, the values of multiple constant defines should be aligned. + +```c++ +#define PREFIX_CACHE_DEFAULT_SIZE 16 +#define PREFIX_CACHE_MAX_SIZE (2 * 1024) +#define DEVICE_NAME L"MyDevice" +``` + +## <a name="14.3"/>14.3 Macro functions + +Expressions (except very simple constants) should be enclosed in parentheses to prevent incorrect multiple expansion of the macro arguments. + +Enclose all argument instances in parentheses. + +Macro arguments should be named with two leading underscores, to prevent their names from being confused with normal source code names, such as variable or function names. + +### <a name="14.3.1"/>14.3.1 Macro functions versus C++ inline functions + +All macro functions should be replaced with a C++ inline function or C++ inline template function if possible. This allows type checking of arguments, and avoids the problem of macro-expansion of macro arguments. + +### <a name="14.3.2"/>14.3.2 Line continuation + +All the `\` at the end of a multi-line macro definition should be aligned with each other. + +There must be no `\` on the last line of a multi-line macro definition. + +### <a name="14.3.3"/>14.3.3 Multi-statement macro functions + +Functional macro definitions with multiple statements or with `if` statements should use `do { <statements> } while(0)` to ensure that the statements will always be compiled together as a single statement block. This ensures that those who mistake the macro for a function don't accidentally split the statements into multiple scopes when the macro is used. Example: consider a macro used like this: + +```c++ +if (fCond) + SOME_MACRO(); +``` + +Wrong: +```c++ +#define SOME_MACRO() \ + Statement1; \ + Statement2; +``` + +Right: +```c++ +#define SOME_MACRO() \ + do \ + { \ + Statement1; \ + Statement2; \ + } while(0) +``` + +The braces ensure the statement block isn't split. The `do { ... } while(0)` ensures that uses of the macro always end with a semicolon. + +### <a name="14.3.4"/>14.3.4 Control flow + +Avoid using control flow inside of preprocessor functions. Since these read like function calls in the source it is best if they also act like function calls. The expectation should be that all arguments will get evaluated one time and we should avoid strange behavior such as only evaluating an argument if a prior argument evaluates to true or evaluating some argument multiple times. + +### <a name="14.3.5"/>14.3.5 Scope + +Macros that require a pair of macros due to the introduction of a scope are strongly discouraged in the JIT. These do exist in the VM and the convention there is to have a _BEGIN and _END suffix at the end of a common all caps macro name. + +```c++ +#define PAL_CPP_EHUNWIND_BEGIN { +#define PAL_CPP_EHUNWIND_END } +``` + +### <a name="14.3.6"/>14.3.6 Examples + +```c++ +#define MIN(_x, _y) (((__x) < (__y)) ? (__x) : (__y)) + +#define CONTAINING_RECORD(_address, _type, _field) \ + ((_type*)((LONG)(_address) - \ + (LONG)(&((_type*)0)->_field))) + +#define STRESS_ASSERT(_cond) \ + do \ + { \ + if (!(_cond) && g_pConfig->IsStressOn()) \ + DebugBreak(); \ + } \ + while(0) +``` + +# <a name="15"/>15 Language Usage Rules + +The following rules are not related to formatting; they provide guidance to improve semantic clarity. + +## <a name="15.1"/>15.1 C/C++ general + +### <a name="15.1.1"/>15.1.1 Casts + +Instead of C-style casts, use `static_cast<>`, `const_cast<>` and `reinterpret_cast<>` for pointers as they are more expressive and type-safe. + +### <a name="15.1.2"/>15.1.2 Globals + +Avoid global variables as they pollute the global namespace and require careful handling to ensure thread safety. Prefer static class variables. + +### <a name="15.1.3"/>15.1.3 `bool` versus `BOOL` + +`bool` is a built-in C++ language type. `bool` variables contain the value `true` or `false`. When stored (such as a member of a struct), it is one byte in size, and `true` is stored as one, `false` as zero. + +`BOOL` is a typedef, either of `bool` (from clrtypes.h) or `int` (from Windows header files), whose value is one of the `#define` macros `TRUE` (1) or `FALSE` (0). + +Use `bool`. Only use `BOOL` when calling an existing API that uses it. + +Right: +```c++ +bool isComplete = true; +``` + +Wrong: +```c++ +BOOL isComplete = TRUE; +``` + +### <a name="15.1.4"/>15.1.4 `NULL` and `nullptr` + +Use the C++11 `nullptr` keyword when assigning a "null" to a pointer variable, or comparing a pointer variable against "null". Do not use `NULL`. + +Right: +```c++ +int* p = nullptr; +if (p == nullptr) + ... +``` + +Wrong: +```c++ +int* p = NULL; +if (p == NULL) + ... +int* p = 0; +if (p == 0) + ... +``` + +### <a name="15.1.5"/>15.1.5 Use of zero + +Integers should be explicitly checked against 0. Pointers should be explicitly checked against `nullptr`. Types that have a legal zero value should use a named zero value, not an explicit zero. For example, `regMaskTP` is a register mask type. Use `RBM_NONE` instead of a constant zero for it. + +Right: +```c++ +int i; +int* p = Foo(); +if (p == nullptr) + ... +if (p != nullptr) + ... +if (i == 0) + ... +if (i != 0) + ... +``` + +Wrong: +```c++ +int i; +int* p = Foo(); +if (!p) + ... +if (p) + ... +if (p == 0) + ... +if (p != 0) + ... +if (!i) + ... +if (i) + ... +``` + +### <a name="15.1.6"/>15.1.6 Nested assignment + +Do not use assignments within `if` or other control-flow statements. + +Right: +```c++ +int x = strlen(szMethodName); +if (x > 5) +``` + +Wrong: +```c++ +if ((x = strlen(szMethodName)) > 5) +``` + +### <a name="15.1.7"/>15.1.7 `if` conditions + +Do not place constants first in comparison checks (unless that reads more naturally), as a trick to avoid accidental assignment in a condition, as assignment within a condition will be a compiler error in our builds. + +Right: +```c++ +if (x == 5) +``` + +Wrong: +```c++ +if (5 == x) +``` + +### <a name="15.1.8"/>15.1.8 `const` + +Use of the `const` qualifier is encouraged. + +It is specifically encouraged to mark class member function as `const`, especially small "accessors", for example: + +```c++ +var_types TypeGet() const { return gtType; } +``` + +### <a name="15.1.9"/>15.1.9 Ternary operators + +Ternary operators `?:` are best used to make quick and simple decisions inside function invocations. Don't use it as a replacement for the `if` statement. Note that putting individual statements on their own line makes it easy to set debugging breakpoints on them. Use of nested ternary operators is strongly discouraged. Using it for simple assignment of a single condition is fine. It's recommended that the "then" and "else" conditions of the ternary operator do not have side-effects. + +Right: +```c++ +if (a == b) +{ + Foo(); +} +else +{ + Bar(); +} +``` + +Wrong: +```c++ +(a == b) ? Foo() : Bar(); // top-level ?: disallowed +``` + +Acceptable: +```c++ +x = (a == b) ? 7 : 9; +Foo((a == b) ? "hi" : "bye"); +``` + +Wrong: +```c++ +x = (a == b) ? ((c == d) ? 1 : 2) : 3; // nested ?: disallowed +``` + +### <a name="15.1.10"/>15.1.10 Use of `goto` + +The `goto` statement should be avoided. + +If you *must* use `goto`, the `goto` label must be all-caps, with words separated by underscores. The label should be surrounded by empty lines and otherwise made very visible, with prominent comments and/or placing it in column zero. + +Example: +```c++ +case GT_LSH: ins = INS_SHIFT_LEFT_LOGICAL; goto SHIFT; +case GT_RSH: ins = INS_SHIFT_RIGHT_ARITHM; goto SHIFT; +case GT_RSZ: ins = INS_SHIFT_RIGHT_LOGICAL; goto SHIFT; + +SHIFT: +``` + +You should think very hard about other ways to code this to avoid using a `goto`. One of the biggest problems is that the `goto` label can be targeted from anyplace in the function, which makes understanding the code very difficult. + +## <a name="15.2"/>15.2 Source file organization + +The general guideline is that header files should not be bigger than 1000 lines and implementation files should not be bigger than 5000 lines of code (including comments, function headers, etc.). Files larger than this should be split up and organized in some better logical fashion. + +A class declaration should contain no implementation code. This is intended to make it easy to browse the API of the class. Note that our shipping "retail" build uses Visual C++ Link Time Code Generation, which can perform cross-module inlining. It is acceptable to define small accessor functions in the class declaration, for simplicity. + +Maintain clear visual separation and identification of "segments" of API, and in particular of the private area of declarations. Logical chunks of APIs should be separated with comments like this: + +```c++ +<blank line> +// +// Description of the following chunk of API +// +<blank line> +``` + +## <a name="15.3"/>15.3 Function declarations + +### <a name="15.3.1"/>15.3.1 Default arguments + +Avoid default arguments values unless the argument has very little semantic impact, especially when adding a new argument to an existing method. Avoiding default values forces all call sites to think about the argument value to use, and prevents call sites from silently opting into unexpected behavior. + +### <a name="15.3.2"/>15.3.2 Overloading + +Never overload functions on a primitive type (e.g. `Foo(int i)` and `Foo(long l)`). + +Avoid operator overloading unless the overload matches the "natural" semantics of the operator when applied to integral types. + +### <a name="15.3.3"/>15.3.3 Enums versus primitive parameter types + +Use enums rather than primitive types for function arguments as it promotes type-safety, and the function signature is more descriptive. + +Specifically, declare and use enum types with two values instead of boolean for function arguments – the enum conveys more information to the reader at the callsite. + +Bad: +```c++ +Foo(true); +Foo(false); +``` + +Good: +```c++ +enum DuplicateSpecification +{ + DS_ALLOW_DUPS, + DS_UNIQUE_ONLY +}; +void Foo(DuplicateSpecification useDups); +Foo(DS_ALLOW_DUPS); +Foo(DS_UNIQUE_ONLY); +``` + +This is especially true if the function has multiple boolean arguments. + +Bad: +```c++ +Bar(true, false); +``` + +Good: +```c++ +Bar(DS_ALLOW_DUPS, FORMAT_FIT_TO_SCREEN); +``` + +### <a name="15.3.4"/>15.3.4 Functions returning pointers + +Functions that return pointers must think carefully about whether a `nullptr` return value could be ambiguous between success with a `nullptr` return value and failure. + +### <a name="15.3.5"/>15.3.5 Reference arguments + +Never use non-const reference arguments as the call-site has no indication that the argument may change. Const reference arguments may be used as they do not have the above problem, and are also required for operators. + +### <a name="15.3.6"/>15.3.6 Resource release + +If you call a function to release a resource and pass it a pointer or handle, you must set the pointer to `nullptr` or handle to `INVALID_HANDLE`. This ensures that the pointer or handle will not be accidentally used in code that follows. + +```c++ +CloseHandle(hMyFile); +hMyFile = INVALID_HANDLE; +``` + +### <a name="15.3.7"/>15.3.7 OUT parameters + +Functions with OUT parameters must initialize them (e.g., to 0 or `nullptr`) on entry to the function. If the function fails, this protects the caller from accidental use of potentially uninitialized values. + +## <a name="15.4"/>15.4 STL usage + +> JIT STL usage rules need to be specified. + +## <a name="15.5"/>15.5 C++ class design + +### <a name="15.5.1"/>15.5.1 Public data members + +Do not declare public data members. Instead, public accessor functions should be exposed to access class members. + +### <a name="15.5.2"/>15.5.2 Friend functions + +Avoid friend functions - they expose internals of the class to the friend function, making subsequent changes to the class more fragile. However, it is notably worse to make everything public. + +### <a name="15.5.3"/>15.5.3 Constructors + +If you declare a constructor, make sure to initialize all the class data members. + +### <a name="15.5.4"/>15.5.4 Destructors + +The JIT uses a specialized memory allocator that does not release memory until compilation is complete. Thus, it is generally bad to declare or require destructors and the calling of `delete`, since memory will never be reclaimed, and JIT developers used to never dealing with deallocation are also likely to omit calls to `delete`. + +### <a name="15.5.5"/>15.5.5 Operator overloading + +Define operators such as `=`, `==`, and `!=` only if you really want and use this capability, and can make them super-efficient. + +Never define an operator to do anything other than the standard semantics for built-in types. + +Never hide expensive work behind an operator. If it's not super efficient then make it an explicit method call. + +### <a name="15.5.6"/>15.5.6 Copy constructor and assignment operator + +The compiler will automatically create a default copy constructor and assignment operator for a class. If that is undesirable, use the C++11 delete functions feature to prevent that, as so: + +```c++ +private: + // No copy constructor or operator= + MyClass(const MyClass& info) = delete; + MyClass& operator=(const MyClass&) = delete; +``` + +### <a name="15.5.7"/>15.5.7 Virtual functions + +An overridden virtual method is explicitly declared virtual for clarity. + +Virtual functions have overhead, so don't use them unless you need polymorphism. + +However, note that virtual functions are often a cleaner, clearer, and faster solution than alternatives. + +### <a name="15.5.8"/>15.5.8 Inheritance + +Don't use inheritance just because it will work. Use it sparingly and judiciously, when it makes sense to the situation. Deeply nested hierarchies can be confusing to understand. + +Be careful with inheritance vs. containment. When in doubt, use containment. + +Don't use multiple implementation inheritance. + +### <a name="15.5.9"/>15.5.9 Global class objects + +Never declare a global instance of a class that has a constructor. Such constructors run in a non-deterministic order which is bad for reliability, and they have to be executed during process startup which is bad for startup performance. It is better to use lazy initialization in such a case. + +## <a name="15.6"/>15.6 Exceptions + +Exceptions should be thrown only on true error paths, not in the general execution of a function. Exceptions are quite expensive on some platforms. What is an error path is a subjective choice depending on the scenario. + +Do not catch all exceptions blindly. Catching all exceptions may mask a genuine bug in your code. Also, on Win32, some exceptions such as out-of-stack, cannot be safely resumed without careful coding. + +Use care when referencing local variables within the "catch" and "finally" blocks, as their values may be an undefined state if an exception occurs. + +## <a name="15.7"/>15.7 Code tuning for performance optimization + +In general, code should be written to be readable first, and optimized for performance second. Don't optimize for the compiler! This will help to keep the code understandable, maintainable, and less prone to bugs. + +In the case of tight loops and code that has been analyzed to be a performance bottleneck, performance optimizations take a higher priority. Talk to the performance team if in doubt. + +## <a name="15.8"/>15.8 Obsoleting functions, classes and macros + +The Visual C++ compiler has support built in for marking various user defined constructs as deprecated. This functionality is accessed via one of two mechanisms: + +```c++ +#pragma deprecated(identifier1 [, identifier2 ...]) +``` + +This mechanism allows you to deprecate pretty much any identifier. In particular it can be used to mark a macro as obsolete: + +```c++ +#define FOO(x) x +#pragma deprecated(FOO) +``` + +Attempts to utilize FOO will result in compiler warning C4995 being raised: + +```c++ +obs.cpp(18) : warning C4995: 'FOO': name was marked as #pragma deprecated +``` + +Note that this warning will fire at the point in the code where the macro is expanded, which may include instances where another macro is used which happens to utilize the deprecated macro (the warning will not fire at the definition of the outer macro). In order to correctly obsolete such a macro it may be necessary to refactor your code to avoid its use by any outer macros which are not being obsoleted. + +Another obsoleting mechanism is: + +```c++ +__declspec(deprecated("descriptive text")) +``` + +This mechanism can be used with classes and methods. It cannot be applied to macros. It is more flexible than the #pragma mechanism since it provides a way to output additional information with the warning message and can be applied to a specific overload of a given method: + +```c++ +#ifdef _MSC_VER +__declspec(deprecated("This method is deprecated, use the version that takes a StackCrawlMark instead")) +#endif +static Module* GetCallersModule(int skip); +``` + +Attempting to call the method annotated above will result in a C4996 warning being raised: + +```c++ +d:\dd\puclr\ndp\clr\src\vm\marshalnative.cpp(431) : warning C4996: 'SystemDomain::GetCallersModule' was declared deprecated +``` + +Code that legitimately still needs to use deprecated functionality (or is being grandfathered in as new functions are deprecated) can use the normal C++ mechanism to suppress the deprecation warnings: + +```c++ +#ifdef _MSC_VER +#pragma warning(push) +#pragma warning(disable:4996) // Suppress warning on call to + // deprecated method +#endif + +Module* pModule = SystemDomain::GetCallersModule(1); + +#ifdef _MSC_VER +#pragma warning(pop) +#endif +``` + +Note that all these techniques are specific to the Microsoft C++ compiler and must therefore be conditionally compiled out for non-Windows builds, as shown in the examples above. diff --git a/Documentation/coding-guidelines/cross-platform-performance-and-eventing.md b/Documentation/coding-guidelines/cross-platform-performance-and-eventing.md new file mode 100644 index 0000000000..f332724478 --- /dev/null +++ b/Documentation/coding-guidelines/cross-platform-performance-and-eventing.md @@ -0,0 +1,287 @@ +# .NET Cross-Plat Performance and Eventing Design + +##Introduction + +As we bring up CoreCLR on the Linux and OS X platforms, it’s important that we determine how we’ll measure and analyze performance on these platforms. On Windows we use an event based model that depends on ETW, and we have a good amount of tooling that builds on this approach. Ideally, we can extend this model to Linux and OS X and re-use much of the Windows tooling. + +# Requirements + +Ideally, we'd like to have the following functionality on each OS that we bring-up: + +- Collection of machine-wide performance data including CPU sampling, threading information (e.g. context switches), and OS specific events / system call tracing. +- Collection of CLR-specific events that we have today exposed as ETW events. +- Collection of EventSource events by a default OS-specific collector, as we do today with ETW on Windows. +- User-mode call stacks for both performance and tracing data. +- Portability of traces across machines, so that analysis can occur off box. +- Data viewable on collection OS. +- Stretch: Data can be understood by TraceEvent, which opens up interesting analysis scenarios. + - Using PerfView and existing tooling. + - Ability to use CAP (Automated analysis) on non-Windows data. + +# Scoping to the Current Landscape + +Given that we’ve built up a rich set of functionality on Windows, much of which depends on ETW and is specific to the OS, we’re going to see some differences across the other operating systems. + +Our goal should be to do the best job that we can to enable data collection and analysis across the supported operating systems by betting on the right technologies, such that as the landscape across these operating systems evolve, .NET is well positioned to take advantage of the changes without needing to change the fundamental technology choices that we’ve made. While this choice will likely result in some types of investigations being more difficult due to absent features that we depend upon on Windows, it is likely to position us better for the future and align us with the OS communities. + +# Linux + +## Proposed Design + +Given that the performance and tracing tool space on Linux is quite fragmented, there is not one tool that meets all of our requirements. As such, we'll use two tools when necessary to gather both performance data and tracing data. + +**For performance data collection we'll use perf_events**, an in-tree performance tool that provides access to hardware counters, software counters and system call tracing. Perf_event will be the primary provider of system-wide performance data such as CPU sampling and context switches. + +**For tracing we'll use LTTng**. LTTng supports usermode tracing with no kernelspace requirements. It allows for strongly typed static events with PID and TID information. The system is very configurable and allows for enabling and disabling of individual events. + +## Tools Considered + +### Perf_Events + +#### Pros + +- Kernel level tracing of hardware counters (CPU samples, context switches, etc.), software counters and system calls. +- Machine-wide or process-wide. No process attach required. +- Collection of owned processes without elevated permissions. +- Provides user-mode stack traces via frame pointers and libunwind. +- Extensible support for [JIT symbol resolution](https://git.kernel.org/cgit/linux/kernel/git/namhyung/linux-perf.git/tree/tools/perf/Documentation/jit-interface.txt). +- In-tree: Basically available for almost every distro. +- Data is stored in perf tool file format (perf.data) – can be opened by a viewer such as “perf report”. + +#### Cons + +- No user-mode static tracing. Only dynamic user-mode tracing, using “breakpoints” with no event payloads. + +### LTTng + +#### Pros + +- User-mode static tracing with no kernel modules required. +- Strongly-typed static event support. +- No pre-registration of static event types required. Events can be enabled before they are known to LTTng. +- System call tracing supported with optional kernel module. User-mode does not require a kernel module. +- Machine-wide or process-wide. No process attach required. +- Collection of owned processes without elevated permissions. +- Events can be tagged with context such as PID, TID. +- Out-of-tree but binaries available for many common distros. +- Data stored in Common Trace Format – designed for interop. + +#### Cons + +- No built-in callstack collection. + +### SystemTap + +#### Pros + +- Supports User-mode static tracing including call stacks. +- Static tracing does not require pre-registration of the event or payload definition when the app starts, which makes EventSource support simple. +- Out-of-tree but binaries available for many common distros. + +#### Cons + +- Complex kernel module is generated and compiled on-the-fly based on the tracing script. +- Static tracing includes a fixed set of static tracing APIs with limited overloads (e.g. int, string). Can’t consider it strongly typed tracing. +- User-mode stack trace support requires debug information to support unwinding. No story for JIT compiled code. +- User-mode stack traces are only supported on x86 and x64. +- Data is stored as unstructured text. + +### DTrace4Linux + +#### Pros + +- Would allow for tracing code and collection script re-use across Linux and OS X. + +#### Cons + +- Source only – no binary redist. +- Small subset of actual DTrace functionality. +- One person’s work rather than many contributions from the community. + +### FTrace + +#### Pros + +- High performance function tracer. +- In-tree: Basically available for almost every distro. + +#### Cons + +- Tracing in kernel-mode only. +- No performance data capture. + +### Extended Berkeley Packet Filter (eBPF) + +#### Pros + +- Should support user-mode static tracing. +- Possible integration with perf_event. + +#### Cons + +- Not currently available – currently being integrated into the kernel. +- Final featureset not clear yet. + +## Infrastructure Bring-Up Action Items + +- Investigate: Determine if clock skew across the trace files will be an issue. +- Investigate: Are traces portable, or do they have to be opened on collection machine? +- Investigate: Do we need rundown or can we use /tmp/perf-$pid.map? How does process/module rundown work? +- Implement: Enable frame pointers on JIT compiled code and helpers to allow stacks to be walked. (PR # [468](https://github.com/dotnet/coreclr/pull/468)) +- Implement: Stack walking using existing stackwalker (libunwind and managed code). +- Implement: JIT/NGEN call frame resolution - /tmp/perf-$pid.map +- Implement: Trace collection tool. + - Responsible for handling all of the complexity. + - Start and stop tracing based on information requested. + - **OPEN ISSUE:** Handle rundown if required. + - Collect any information needed for off-box viewing (e.g. /tmp/perf-$pid.map). + - Compress into one archive that can be copied around easily. +- Implement: Viewing of data in PerfView + +# OS X + +## Proposed Design + +On OS X, the performance tooling space is much less fragmented than Linux. However, this also means that there are many fewer options. + +**For performance data collection and tracing, we’ll use Instruments.** Instruments is the Apple-built and supported performance tool for OS X. It has a wide range of collection abilities including CPU sampling, context switching, system call tracing, power consumption, memory leaks, etc. It also has support for custom static and dynamic tracing using DTrace as a back-end, which we can take advantage of to provide a logging mechanism for CLR events and EventSource. + +Unfortunately, there are some features that Instruments/DTrace do not provide, such as resolution of JIT compiled call frames. Given the existing tooling choices, and the profiler preferences of the OS X community of developers, it likely makes the most sense to use Instruments as our collection and analysis platform, even though it does not support the full set of features that we would like. It’s also true that the number of OS X specific performance issues is likely to be much smaller than the set of all performance issues, which means that in many cases, Windows or Linux can be used, which will provide a more complete story for investigating performance issues. + +## Tools Considered + +### Instruments + +#### Pros + +- Available for all recent versions of OS X. +- Provided free by Apple as part of XCode. +- Wide range of performance collection options, both using a GUI and on the command line. +- Can be configured to have relatively low overhead at collection time (unfortunately not the default). +- Supports static and dynamic tracing via DTrace probes. +- Supports machine wide and process specific collection. +- Supports kernel and user-mode call stack collection. + +#### Cons + +- No support for JIT compiled frame resolution. +- Closed source - no opportunities for contribution of "missing" features. +- Closed file format - likely difficult to open a trace in PerfView. + +### DTrace + +#### Pros + +- In-box as part of the OS. +- Supports static tracing using header files generated by dtrace. +- Supports dynamic tracing and limited argument capture. +- Supports kernel and user-mode call stack collection. + +#### Cons + +- No support for JIT compiled frame resolution - Third party call stack frame resolution feature (jstack) does not work on OS X. +- Minimal to no investment - DTrace only kept functional for Instruments scenarios. +- No opportunities for contribution of "missing" features. + +## Infrastructure Bring-Up Action Items + +- Implement: Enable frame pointers on JIT compiled code and helpers to allow stacks to be walked. (PR # [468](https://github.com/dotnet/coreclr/pull/468)) +- Implement: Trace collection tool + - NOTE: Use deferred mode to minimize overhead. + - Investigate: Using iprofiler to collect data instead of the instruments UI. + +# CLR Events + +On Windows, the CLR has a number of ETW events that are used for diagnostic and performance purposes. These events need to be enabled on Linux and OS X so that we can collect and use them for performance investigations. + +## Platform Agnostic Action Items + +- Implement: Abstract ETW calls to an inline-able platform abstraction layer. + - **OPEN ISSUE:** Can / should we re-use PAL? +- Implement: Stack walker event implementation for x-plat – this is likely the same code for both Linux and OS X. + +## Linux Action Items + +- Implement: Build mechanics to translate ETW manifest into LTTng tracepoint definitions. +- Implement: Generate calls to tracepoints in the PAL (see above). + +## OS X Action Items + +- Implement: Build mechanics to translate ETW manifest into DTrace probe definitions. +- Implement: Generate calls to probes in PAL (see above). + +# EventSource Proposal + +Ideally, EventSource operates on Linux and OS X just like it does on Windows. Namely, there is no special registration of any kind that must occur. When an EventSource is initialized, it does everything necessary to register itself with the appropriate logging system (ETW, LTTng, DTrace), such that its events are stored by the logging system when configured to do so. + +EventSource should emit events to the appropriate logging system on each operating system. Ideally, we can support the following functionality on all operating systems: + +- No pre-registration of events or payload definitions. +- Enable/disable individual events or sets of events. +- Strongly typed payload fields. + +**Supporting all of these requirements will mean a significant investment.** Today, LTTng and DTrace support all of these requirements, but do so for tracepoints that are defined statically at compile time. This is done by providing tooling that takes a tool specific manifest and generates C code that can then be compiled into the application. + +As an example of the kind of work we’ll need to do: LTTng generates helpers that are then called as C module constructors and destructors to register and unregister tracepoint provider definitions. If we want to provide the same level of functionality for EventSource events, we’ll need to understand the generated code and then write our own helpers and register/unregister calls. + +While doing this work puts us in an ideal place from a performance and logging verbosity point-of-view, we should make sure that the work done is getting us the proper amount of benefit (e.g. is pay-for-play). As such, **we should start with a much simpler design, and move forward with this more complex solution once we’ve proven that the benefit is clear**. + +## Step # 1: Static Event(s) with JSON Payload + +As a simple stop-gap solution to get EventSource support on Linux and OS X, we can implement a single EventSource event (or one event per verbosity) that is used to emit all EventSource events regardless of the EventSource that emits them. The payload will be a JSON string that represents the arguments of the event. + +## Step # 2: Static Event Generation with Strongly-Typed Payloads + +Once we have basic EventSource functionality working, we can continue the investigation into how we’d register/unregister and use strongly typed static tracepoints using LTTng and DTrace, and how we’d call them when an EventSource fires the corresponding event. + +## Compatibility Concerns + +In general, we should be transparent about this plan, and not require any compatibility between the two steps other than to ensure that our tools continue to work as we transition. + +## Step # 1 Bring-Up Action Items + +- Implement: A static EventSource tracepoint / probe as a CLR event. +- Implement: JSON serialization of the event payload. +- Implement: EventListener implementation for each platform that calls out to the tracepoint / probe. + +# Proposed Priorities + +Given the significant work required to bring all of this infrastructure up, this is likely to be a long-term investment. As such, it makes sense to aim at the most impactful items first, and continually evaluate where we are along the road. + +## Scenarios + +We’ll use the following scenarios when defining priorities: + +- P1: Performance analysis in support of bring-up of the .NET Core runtime and framework on Linux and OS X. +- P2: Performance analysis of ASP.NET running on .NET Core on Linux and OS X. + +To support these scenarios, we need the following capabilities: + +- P1: Collection and analysis of CPU, threading, syscalls, native memory. Support for JIT compiled call frame resolution. +- P2: Collection and analysis of managed memory, managed thread pool, async, causality, JIT events. + +We expect that the following assumptions will hold for the majority of developers and applications: + +- Development occurs on Windows or OS X. +- Application deployment occurs on Windows or Linux. + +## Work Items + +### Priority 1 + +- Enable basic performance data collection on Linux with perf_events: + - Implement a collection script that makes collection easy for anyone. + - Enable JIT compiled code resolution for call stacks in perf_event. + +### Priority 2 + +- Enable more advanced performance data collection for runtime components on Linux: + - CLR in-box event support – Emits diagnostic / performance events for GC, JIT, ThreadPool, etc. + - Linux EventSource support – Support for FrameworkEventSource, Tasks, Async Causality, and custom EventSource implementations. + - Data collection on Linux via LTTng. + +### Future: + +- Enable Linux traces to be analyzed using PerfView / TraceEvent on Windows. +- Evaluate options for viewing Linux traces on OS X. +- Enable more advanced performance data collection for runtime components on OS X via CLR in-box events and EventSource. |