[MERGE #1819 @jianchun] swb: annotate BaseDictionary entry items

Merge pull request #1819 from jianchun:wbdict

BaseDictionary Key or Value can be of GC pointer type, potentially needs
write barrier on each entry item.

Use macro and template type traits to replace underlying entry Key/Value
type with WriteBarrierPtr when Key or Value is pointer type and used with
recycler allocators.

BaseDictionary methods still use original Key/Value types. Only the
underlying EntryType may contain swapped type. Most methods are not
affected. However, when WriteBarrierPtr is used, only immutable
TryGetReference should be used. (We cannot get a mutable reference and
possibly change the pointer underneath WriteBarrierPtr.)

Allow WriteBarrierPtr to take immutable address to support TryGetReference
(used a lot). Fixed a bunch of incorrect `const` usage.

Update: Apply template traits to field types to simplify write barrier field
annotations. Now most annotations are Field or FieldNoBarrier (for pointers
known not needing write barrier).

Author: Jianchun Xu

lib/Common/DataStructures/BaseDictionary.h

@@ -80,7 +80,9 @@ namespace JsUtil
         typedef TValue ValueType;
         typedef typename AllocatorInfo<TAllocator, TValue>::AllocatorType AllocatorType;
         typedef SizePolicy CurrentSizePolicy;
-        typedef Entry<TKey, TValue> EntryType;
+        typedef Entry<
+                    Field(TKey, TAllocator),
+                    Field(TValue, TAllocator)> EntryType;
 
         template<class TDictionary> class EntryIterator;
         template<class TDictionary> class BucketEntryIterator;
@@ -90,17 +92,17 @@ namespace JsUtil
         friend class Js::RemoteDictionary<BaseDictionary>;
         template <typename ValueOrKey> struct ComparerType { typedef Comparer<ValueOrKey> Type; }; // Used by diagnostics to access Comparer type
 
-        Pointer(int, TAllocator) buckets;
-        Pointer(EntryType, TAllocator) entries;
-        PointerNoBarrier(AllocatorType) alloc;
+        Field(int*, TAllocator) buckets;
+        Field(EntryType*, TAllocator) entries;
+        FieldNoBarrier(AllocatorType*) alloc;
         Field(int) size;
         Field(uint) bucketCount;
         Field(int) count;
         Field(int) freeList;
         Field(int) freeCount;
 
 #if PROFILE_DICTIONARY
-        PointerNoBarrier(DictionaryStats) stats;
+        FieldNoBarrier(DictionaryStats*) stats;
 #endif
         enum InsertOperations
         {
@@ -370,7 +372,8 @@ namespace JsUtil
         template <typename TLookup>
         bool TryGetReference(const TLookup& key, TValue** value) const
         {
-            return TryGetReference(key, const_cast<const TValue **>(value));
+            int i;
+            return TryGetReference(key, value, &i);
         }
 
         template <typename TLookup>
@@ -389,7 +392,14 @@ namespace JsUtil
         template <typename TLookup>
         bool TryGetReference(const TLookup& key, TValue** value, int* index) const
         {
-            return TryGetReference(key, const_cast<const TValue **>(value), index);
+            int i = FindEntryWithKey(key);
+            if (i >= 0)
+            {
+                *value = &entries[i].Value();
+                *index = i;
+                return true;
+            }
+            return false;
         }
 
         const TValue& GetValueAt(const int index) const
@@ -1385,7 +1395,7 @@ namespace JsUtil
     class BaseHashSet : protected BaseDictionary<TKey, TElement, TAllocator, SizePolicy, Comparer, Entry, Lock>
     {
         typedef BaseDictionary<TKey, TElement, TAllocator, SizePolicy, Comparer, Entry, Lock> Base;
-        typedef Entry<TKey, TElement> EntryType;
+        typedef typename Base::EntryType EntryType;
         typedef typename Base::AllocatorType AllocatorType;
         friend struct JsDiag::RemoteDictionary<BaseHashSet<TElement, TAllocator, SizePolicy, TKey, Comparer, Entry, Lock>>;
 
@@ -1559,7 +1569,7 @@ namespace JsUtil
     class SynchronizedDictionary: protected BaseDictionary<TKey, TValue, TAllocator, SizePolicy, Comparer, Entry>
     {
     private:
-        PointerNoBarrier(SyncObject) syncObj;
+        FieldNoBarrier(SyncObject*) syncObj;
 
         typedef BaseDictionary<TKey, TValue, TAllocator, SizePolicy, Comparer, Entry> Base;
     public:

lib/Common/DataStructures/Cache.h

@@ -184,7 +184,7 @@ namespace JsUtil
             return cacheStore->TryGetValue(key, value);
         }
 
-        bool TryGetReference(const TKey& key, TValue** value, int* index)
+        bool TryGetReference(const TKey& key, const TValue** value, int* index)
         {
             return cacheStore->TryGetReference(key, value, index);
         }

lib/Common/Memory/RecyclerPointers.h

@@ -63,19 +63,17 @@ struct AllocatorWriteBarrierPolicy<RecyclerNonLeafAllocator, T> { typedef _write
 template <>
 struct AllocatorWriteBarrierPolicy<RecyclerNonLeafAllocator, int> { typedef _no_write_barrier_policy Policy; };
 
-// Choose WriteBarrierPtr or NoWriteBarrierPtr based on Policy
+// Choose write barrier Field type: T unchanged, or WriteBarrierPtr based on Policy.
 //
 template <class T, class Policy>
-struct _WriteBarrierPtrPolicy { typedef NoWriteBarrierPtr<T> Ptr; };
+struct _WriteBarrierFieldType { typedef T Type; };
 template <class T>
-struct _WriteBarrierPtrPolicy<T, _write_barrier_policy> { typedef WriteBarrierPtr<T> Ptr; };
+struct _WriteBarrierFieldType<T*, _write_barrier_policy> { typedef WriteBarrierPtr<T> Type; };
 
-// Choose WriteBarrierPtr or NoWriteBarrierPtr based on Allocator and T* type
-//
 template <class T,
           class Allocator = Recycler,
-          class Policy = typename AllocatorWriteBarrierPolicy<Allocator, T*>::Policy>
-struct WriteBarrierPtrTraits { typedef typename _WriteBarrierPtrPolicy<T, Policy>::Ptr Ptr; };
+          class Policy = typename AllocatorWriteBarrierPolicy<Allocator, T>::Policy>
+struct WriteBarrierFieldTypeTraits { typedef typename _WriteBarrierFieldType<T, Policy>::Type Type; };
 
 // ArrayWriteBarrier behavior
 //
@@ -163,9 +161,9 @@ class NoWriteBarrierPtr
 
     // Getters
     T * operator->() const { return this->value; }
-    operator T*() const { return this->value; }
+    operator T* const & () const { return this->value; }
 
-    T const** operator&() const { return &value; }
+    T* const * operator&() const { return &value; }
     T** operator&() { return &value; }
 
     // Setters
@@ -216,21 +214,24 @@ class WriteBarrierPtr
 
     // Getters
     T * operator->() const { return ptr; }
-    operator T*() const { return ptr; }
+    operator T* const & () const { return ptr; }
 
-    T const** AddressOf() const { return &ptr; }
+    T* const * AddressOf() const { return &ptr; }
     T** AddressOf() { return &ptr; }
 
-    T const** operator&() const
-    {
-        static_assert(false, "Might need to set barrier for this operation, and use AddressOf instead.");
-        return &ptr;
-    }
-    T** operator&()
+    // Taking immutable address is ok
+    //
+    T* const * operator&() const
     {
-        static_assert(false, "Might need to set barrier for this operation, and use AddressOf instead.");
         return &ptr;
     }
+    // Taking mutable address is not allowed
+    //
+    // T** operator&()
+    // {
+    //     static_assert(false, "Might need to set barrier for this operation, and use AddressOf instead.");
+    //     return &ptr;
+    // }
 
     // Setters
     WriteBarrierPtr& operator=(T * ptr)

lib/Common/Memory/WriteBarrierMacros.h

@@ -28,33 +28,30 @@
 
 #define SAVE_WRITE_BARRIER_MACROS() \
     PushMacro("Field") \
-    PushMacro("PointerNoBarrier") \
-    PushMacro("Pointer")
+    PushMacro("FieldNoBarrier")
 
 #define RESTORE_WRITE_BARRIER_MACROS() \
     PopMacro("Field") \
-    PopMacro("PointerNoBarrier") \
-    PopMacro("Pointer")
+    PopMacro("FieldNoBarrier")
 
 #endif
 
 #ifdef FORCE_USE_WRITE_BARRIER
 SAVE_WRITE_BARRIER_MACROS()
 #undef Field
-#undef PointerNoBarrier
-#undef Pointer
+#undef FieldNoBarrier
 #endif
 
 // TODO: Turn off these annotations on Win32
 #if defined(__clang__) || defined(FORCE_USE_WRITE_BARRIER)
 // Various macros for defining field attributes
-#define Field(type) NoWriteBarrierField<type>
-#define PointerNoBarrier(type) NoWriteBarrierPtr<type>
-#define Pointer(type, ...) typename WriteBarrierPtrTraits<type, ##__VA_ARGS__>::Ptr
+#define Field(type, ...) \
+    typename WriteBarrierFieldTypeTraits<type, ##__VA_ARGS__>::Type
+#define FieldNoBarrier(type) \
+    typename WriteBarrierFieldTypeTraits<type, _no_write_barrier_policy, _no_write_barrier_policy>::Type
 #else
-#define Field(type) type
-#define PointerNoBarrier(type) type*
-#define Pointer(type, ...) type*
+#define Field(type, ...) type
+#define FieldNoBarrier(type) type
 #endif
 
 #undef FORCE_USE_WRITE_BARRIER

lib/Runtime/Base/AuxPtrs.h

@@ -28,7 +28,7 @@ namespace Js
         static const uint8 MaxCount;
         Field(uint8) count;                 // always saving maxCount
         Field(FieldsEnum) type[_MaxCount];  // save instantiated pointer enum
-        Pointer(void) ptr[_MaxCount];       // save instantiated pointer address
+        Field(void*) ptr[_MaxCount];        // save instantiated pointer address
         AuxPtrsFix();
         AuxPtrsFix(AuxPtrsFix<FieldsEnum, 16>* ptr16); // called when promoting from AuxPtrs16 to AuxPtrs32
         void* Get(FieldsEnum e);
@@ -53,7 +53,7 @@ namespace Js
         Field(uint8) count;                                      // save instantiated pointers count
         Field(uint8) capacity;                                   // save number of pointers can be hold in current instance of AuxPtrs
         Field(uint8) offsets[static_cast<int>(FieldsEnum::Max)]; // save position of each instantiated pointers, if not instantiate, it's invalid
-        Pointer(void) ptrs[1];                                   // instantiated pointer addresses
+        Field(void*) ptrs[1];                                    // instantiated pointer addresses
         AuxPtrs(uint8 capacity, AuxPtrs32* ptr32);               // called when promoting from AuxPtrs32 to AuxPtrs
         AuxPtrs(uint8 capacity, AuxPtrs* ptr);                   // called when expanding (i.e. promoting from AuxPtrs to bigger AuxPtrs)
         void* Get(FieldsEnum e);

lib/Runtime/Base/CompactCounters.h

@@ -37,7 +37,7 @@ namespace Js
         mutable Field(bool) bgThreadCallStarted;
         Field(bool) isCleaningUp;
 #endif
-        Pointer(Fields) fields;
+        Field(Fields*) fields;
 
         CompactCounters() { }
         CompactCounters(T* host)

lib/Runtime/Base/FunctionBody.cpp

@@ -1731,7 +1731,7 @@ namespace Js
                 !this->GetSourceContextInfo()->IsDynamic() &&
                 this->m_scriptContext->DoUndeferGlobalFunctions())
             {
-                this->GetUtf8SourceInfo()->UndeferGlobalFunctions([this](JsUtil::SimpleDictionaryEntry<Js::LocalFunctionId, Js::ParseableFunctionInfo*> func)
+                this->GetUtf8SourceInfo()->UndeferGlobalFunctions([this](const Utf8SourceInfo::DeferredFunctionsDictionary::EntryType& func)
                 {
                     Js::ParseableFunctionInfo *nextFunc = func.Value();
                     JavascriptExceptionObject* pExceptionObject = nullptr;

lib/Runtime/Base/FunctionBody.h

@@ -1351,7 +1351,7 @@ namespace Js
 
         typedef AuxPtrs<FunctionProxy, AuxPointerType> AuxPtrsT;
         friend AuxPtrsT;
-        Pointer(AuxPtrsT) auxPtrs;
+        Field(AuxPtrsT*) auxPtrs;
         void* GetAuxPtr(AuxPointerType e) const;
         void* GetAuxPtrWithLock(AuxPointerType e) const;
         void SetAuxPtr(AuxPointerType e, void* ptr);
@@ -1451,17 +1451,17 @@ namespace Js
 
     protected:
         // Static method(s)
-        static void SetDisplayName(const char16* srcName, Pointer(const char16)* destName, uint displayNameLength, ScriptContext * scriptContext, SetDisplayNameFlags flags = SetDisplayNameFlagsNone);
+        static void SetDisplayName(const char16* srcName, Field(const char16*)* destName, uint displayNameLength, ScriptContext * scriptContext, SetDisplayNameFlags flags = SetDisplayNameFlagsNone);
         static bool SetDisplayName(const char16* srcName, const char16** destName, uint displayNameLength, ScriptContext * scriptContext, SetDisplayNameFlags flags = SetDisplayNameFlagsNone);
         static bool IsConstantFunctionName(const char16* srcName);
 
     protected:
-        PointerNoBarrier(ScriptContext)  m_scriptContext;   // Memory context for this function body
-        Pointer(Utf8SourceInfo) m_utf8SourceInfo;
+        FieldNoBarrier(ScriptContext*) m_scriptContext;   // Memory context for this function body
+        Field(Utf8SourceInfo*) m_utf8SourceInfo;
         // WriteBarrier-TODO: Consider changing this to NoWriteBarrierPtr, and skip tagging- also, tagging is likely unnecessary since that pointer in question is likely not resolvable
-        PointerNoBarrier(FunctionProxyPtr) m_referenceInParentFunction; // Reference to nested function reference to this function in the parent function body (tagged to not be actual reference)
-        Pointer(ScriptFunctionType) deferredPrototypeType;
-        Pointer(ProxyEntryPointInfo) m_defaultEntryPointInfo; // The default entry point info for the function proxy
+        FieldNoBarrier(FunctionProxyPtr*) m_referenceInParentFunction; // Reference to nested function reference to this function in the parent function body (tagged to not be actual reference)
+        Field(ScriptFunctionType*) deferredPrototypeType;
+        Field(ProxyEntryPointInfo*) m_defaultEntryPointInfo; // The default entry point info for the function proxy
 
         Field(uint) m_functionNumber;  // Per thread global function number
 
@@ -1771,7 +1771,7 @@ namespace Js
         Field(bool) m_utf8SourceHasBeenSet;          // start of UTF8-encoded source
         Field(uint) m_sourceIndex;             // index into the scriptContext's list of saved sources
 #if DYNAMIC_INTERPRETER_THUNK
-        PointerNoBarrier(void) m_dynamicInterpreterThunk;  // Unique 'thunk' for every interpreted function - used for ETW symbol decoding.
+        FieldNoBarrier(void*) m_dynamicInterpreterThunk;  // Unique 'thunk' for every interpreted function - used for ETW symbol decoding.
 #endif
         Field(uint) m_cbStartOffset;         // pUtf8Source is this many bytes from the start of the scriptContext's source buffer.
 
@@ -1783,10 +1783,10 @@ namespace Js
 
         ULONG m_lineNumber;
         ULONG m_columnNumber;
-        Pointer(const char16) m_displayName;  // Optional name
+        Field(const char16*) m_displayName;  // Optional name
         uint m_displayNameLength;
-        Pointer(PropertyRecordList) m_boundPropertyRecords;
-        Pointer(NestedArray) nestedArray;
+        Field(PropertyRecordList*) m_boundPropertyRecords;
+        Field(NestedArray*) nestedArray;
 
     public:
 #if DBG
@@ -1988,16 +1988,16 @@ namespace Js
                 friend class ByteCodeBufferBuilder;
 
             public:
-                PointerNoBarrier(SmallSpanSequence) pSpanSequence;
+                FieldNoBarrier(SmallSpanSequence*) pSpanSequence;
 
                 RegSlot         frameDisplayRegister;   // this register slot cannot be 0 so we use that sentinel value to indicate invalid
                 RegSlot         objectRegister;         // this register slot cannot be 0 so we use that sentinel value to indicate invalid
-                Pointer(ScopeObjectChain) pScopeObjectChain;
-                Pointer(ByteBlock) m_probeBackingBlock; // NULL if no Probes, otherwise a copy of the unmodified the byte-codeblock //Delay
+                Field(ScopeObjectChain*) pScopeObjectChain;
+                Field(ByteBlock*) m_probeBackingBlock;  // NULL if no Probes, otherwise a copy of the unmodified the byte-codeblock //Delay
                 int32 m_probeCount;             // The number of installed probes (such as breakpoints).
 
                 // List of bytecode offset for the Branch bytecode.
-                Pointer(AuxStatementData) m_auxStatementData;
+                Field(AuxStatementData*) m_auxStatementData;
 
                 SourceInfo():
                     frameDisplayRegister(0),
@@ -2012,18 +2012,18 @@ namespace Js
             };
 
     private:
-        Pointer(ByteBlock) byteCodeBlock;               // Function byte-code for script functions
-        Pointer(FunctionEntryPointList) entryPoints;
-        Pointer(Var) m_constTable;
-        Pointer(void*) inlineCaches;
+        Field(ByteBlock*) byteCodeBlock;                // Function byte-code for script functions
+        Field(FunctionEntryPointList*) entryPoints;
+        Field(Var*) m_constTable;
+        Field(void**) inlineCaches;
         InlineCachePointerArray<PolymorphicInlineCache> polymorphicInlineCaches; // Contains the latest polymorphic inline caches
-        Pointer(PropertyId) cacheIdToPropertyIdMap;
+        Field(PropertyId*) cacheIdToPropertyIdMap;
 
 #if DBG
 #define InlineCacheTypeNone         0x00
 #define InlineCacheTypeInlineCache  0x01
 #define InlineCacheTypeIsInst       0x02
-            Pointer(byte) m_inlineCacheTypes;
+            Field(byte*) m_inlineCacheTypes;
 #endif
     public:
         PropertyId * GetCacheIdToPropertyIdMap()
@@ -2149,7 +2149,7 @@ namespace Js
 #ifdef IR_VIEWER
         // whether IR Dump is enabled for this function (used by parseIR)
         bool m_isIRDumpEnabled : 1;
-        Pointer(Js::DynamicObject) m_irDumpBaseObject;
+        Field(Js::DynamicObject*) m_irDumpBaseObject;
 #endif /* IR_VIEWER */
 
         Field(uint8) bailOnMisingProfileCount;
@@ -2181,14 +2181,14 @@ namespace Js
         // copied in FunctionBody::Clone
         //
 
-        PointerNoBarrier(Js::ByteCodeCache) byteCodeCache;  // Not GC allocated so naked pointer
+        FieldNoBarrier(Js::ByteCodeCache*) byteCodeCache;   // Not GC allocated so naked pointer
 #ifdef ENABLE_DEBUG_CONFIG_OPTIONS
         static bool shareInlineCaches;
 #endif
-        Pointer(FunctionEntryPointInfo) defaultFunctionEntryPointInfo;
+        Field(FunctionEntryPointInfo*) defaultFunctionEntryPointInfo;
 
 #if ENABLE_PROFILE_INFO
-        Pointer(DynamicProfileInfo) dynamicProfileInfo;
+        Field(DynamicProfileInfo*) dynamicProfileInfo;
 #endif
 
 
@@ -2642,7 +2642,7 @@ namespace Js
         bool IsSimpleJitOriginalEntryPoint() const;
 
 #if DYNAMIC_INTERPRETER_THUNK
-        static BYTE GetOffsetOfDynamicInterpreterThunk() { return offsetof(FunctionBody, m_dynamicInterpreterThunk); }
+        static BYTE GetOffsetOfDynamicInterpreterThunk() { return static_cast<BYTE>(offsetof(FunctionBody, m_dynamicInterpreterThunk)); }
         void* GetDynamicInterpreterEntryPoint() const
         {
             return m_dynamicInterpreterThunk;

lib/Runtime/Language/EvalMapRecord.h

@@ -131,7 +131,7 @@ namespace Js
 
         bool TryGetValue(const Key& key, Value* value)
         {
-            EntryRecord** entryRecord;
+            EntryRecord* const * entryRecord;
             Key cachedKey;
             int index;
             bool success = dictionary->TryGetReference(key, &entryRecord, &index);
@@ -168,7 +168,7 @@ namespace Js
 
         void Add(const Key& key, Value value)
         {
-            EntryRecord** entryRecord;
+            EntryRecord* const * entryRecord;
             int index;
             bool success = dictionary->TryGetReference(key, &entryRecord, &index);
             if (success && ((*entryRecord) != nullptr))