Bug 1401624: Part 2: Improve comments in TraceDataRelocations r=sfink

It took me a while to convince myself that this code was still okay for 0-tagged object Values. Adding a comment to make it clearer for future readers (and in the hope that a reviewer will notice my mistake if I am wrong.)

Differential Revision: https://phabricator.services.mozilla.com/D29052

--HG--
extra : moz-landing-system : lando

js/src/jit/arm/Assembler-arm.h

@@ -1264,6 +1264,8 @@ class Assembler : public AssemblerShared {
   }
 
   void writeDataRelocation(BufferOffset offset, ImmGCPtr ptr) {
+    // Raw GC pointer relocations and Value relocations both end up in
+    // Assembler::TraceDataRelocations.
     if (ptr.value) {
       if (gc::IsInsideNursery(ptr.value)) {
         embedsNurseryPointers_ = true;

js/src/jit/arm64/Assembler-arm64.cpp

@@ -624,9 +624,13 @@ void Assembler::TraceDataRelocations(JSTracer* trc, JitCode* code,
     uintptr_t* literalAddr = load->LiteralAddress<uintptr_t*>();
     uintptr_t literal = *literalAddr;
 
-    // All pointers on AArch64 will have the top bits cleared.
-    // If those bits are not cleared, this must be a Value.
+    // Data relocations can be for Values or for raw pointers. If a Value is
+    // zero-tagged, we can trace it as if it were a raw pointer. If a Value
+    // is not zero-tagged, we have to interpret it as a Value to ensure that the
+    // tag bits are masked off to recover the actual pointer.
+
     if (literal >> JSVAL_TAG_SHIFT) {
+      // This relocation is a Value with a non-zero tag.
       Value v = Value::fromRawBits(literal);
       TraceManuallyBarrieredEdge(trc, &v, "ion-masm-value");
       if (*literalAddr != v.asRawBits()) {
@@ -637,6 +641,7 @@ void Assembler::TraceDataRelocations(JSTracer* trc, JitCode* code,
       continue;
     }
 
+    // This relocation is a raw pointer or a Value with a zero tag.
     // No barriers needed since the pointers are constants.
     TraceManuallyBarrieredGenericPointerEdge(
         trc, reinterpret_cast<gc::Cell**>(literalAddr), "ion-masm-ptr");

js/src/jit/arm64/MacroAssembler-arm64.h

@@ -1919,6 +1919,8 @@ class MacroAssemblerCompat : public vixl::MacroAssembler {
 
   // load: offset to the load instruction obtained by movePatchablePtr().
   void writeDataRelocation(ImmGCPtr ptr, BufferOffset load) {
+    // Raw GC pointer relocations and Value relocations both end up in
+    // Assembler::TraceDataRelocations.
     if (ptr.value) {
       if (gc::IsInsideNursery(ptr.value)) {
         embedsNurseryPointers_ = true;
@@ -1927,6 +1929,8 @@ class MacroAssemblerCompat : public vixl::MacroAssembler {
     }
   }
   void writeDataRelocation(const Value& val, BufferOffset load) {
+    // Raw GC pointer relocations and Value relocations both end up in
+    // Assembler::TraceDataRelocations.
     if (val.isGCThing()) {
       gc::Cell* cell = val.toGCThing();
       if (cell && gc::IsInsideNursery(cell)) {

js/src/jit/x64/MacroAssembler-x64.h

@@ -85,6 +85,8 @@ class MacroAssemblerX64 : public MacroAssemblerX86Shared {
   // X64 helpers.
   /////////////////////////////////////////////////////////////////
   void writeDataRelocation(const Value& val) {
+    // Raw GC pointer relocations and Value relocations both end up in
+    // Assembler::TraceDataRelocations.
     if (val.isGCThing()) {
       gc::Cell* cell = val.toGCThing();
       if (cell && gc::IsInsideNursery(cell)) {

js/src/jit/x86-shared/Assembler-x86-shared.cpp

@@ -47,10 +47,14 @@ void AssemblerX86Shared::TraceDataRelocations(JSTracer* trc, JitCode* code,
     void* data = X86Encoding::GetPointer(src);
 
 #ifdef JS_PUNBOX64
-    // All pointers on x64 will have the top bits cleared. If those bits
-    // are not cleared, this must be a Value.
+    // Data relocations can be for Values or for raw pointers. If a Value is
+    // zero-tagged, we can trace it as if it were a raw pointer. If a Value
+    // is not zero-tagged, we have to interpret it as a Value to ensure that the
+    // tag bits are masked off to recover the actual pointer.
+
     uintptr_t word = reinterpret_cast<uintptr_t>(data);
     if (word >> JSVAL_TAG_SHIFT) {
+      // This relocation is a Value with a non-zero tag.
       Value value = Value::fromRawBits(word);
       MOZ_ASSERT_IF(value.isGCThing(),
                     gc::IsCellPointerValid(value.toGCThing()));
@@ -64,6 +68,7 @@ void AssemblerX86Shared::TraceDataRelocations(JSTracer* trc, JitCode* code,
     }
 #endif
 
+    // This relocation is a raw pointer or a Value with a zero tag.
     gc::Cell* cell = static_cast<gc::Cell*>(data);
     MOZ_ASSERT(gc::IsCellPointerValid(cell));
     TraceManuallyBarrieredGenericPointerEdge(trc, &cell, "jit-masm-ptr");

js/src/jit/x86-shared/Assembler-x86-shared.h

@@ -272,6 +272,8 @@ class AssemblerX86Shared : public AssemblerShared {
   CompactBufferWriter dataRelocations_;
 
   void writeDataRelocation(ImmGCPtr ptr) {
+    // Raw GC pointer relocations and Value relocations both end up in
+    // Assembler::TraceDataRelocations.
     if (ptr.value) {
       if (gc::IsInsideNursery(ptr.value)) {
         embedsNurseryPointers_ = true;