Fix undefined behaviour on CommentOperator

The {CommentOperator}, used for implementing the --code-comments flag,
is not UBSan-safe. This CL fixes this and adds a test which uses code
comments.

R=bmeurer@chromium.org

Bug: v8:7744
Change-Id: Ia6ec509e77d998df085ac7377cb24854354e3aa2
Reviewed-on: https://chromium-review.googlesource.com/1051235
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#53100}

src/compiler/arm/code-generator-arm.cc

@@ -880,11 +880,9 @@ CodeGenerator::CodeGenResult CodeGenerator::AssembleArchInstruction(
     case kArchDebugBreak:
       __ stop("kArchDebugBreak");
       break;
-    case kArchComment: {
-      Address comment_string = i.InputExternalReference(0).address();
-      __ RecordComment(reinterpret_cast<const char*>(comment_string));
+    case kArchComment:
+      __ RecordComment(reinterpret_cast<const char*>(i.InputInt32(0)));
       break;
-    }
     case kArchThrowTerminator:
       DCHECK_EQ(LeaveCC, i.OutputSBit());
       unwinding_info_writer_.MarkBlockWillExit();

src/compiler/arm64/code-generator-arm64.cc

@@ -795,11 +795,9 @@ CodeGenerator::CodeGenResult CodeGenerator::AssembleArchInstruction(
     case kArchDebugBreak:
       __ Debug("kArchDebugBreak", 0, BREAK);
       break;
-    case kArchComment: {
-      Address comment_string = i.InputExternalReference(0).address();
-      __ RecordComment(reinterpret_cast<const char*>(comment_string));
+    case kArchComment:
+      __ RecordComment(reinterpret_cast<const char*>(i.InputInt64(0)));
       break;
-    }
     case kArchThrowTerminator:
       unwinding_info_writer_.MarkBlockWillExit();
       break;

src/compiler/ia32/code-generator-ia32.cc

@@ -771,11 +771,9 @@ CodeGenerator::CodeGenResult CodeGenerator::AssembleArchInstruction(
     case kArchTableSwitch:
       AssembleArchTableSwitch(instr);
       break;
-    case kArchComment: {
-      Address comment_string = i.InputExternalReference(0).address();
-      __ RecordComment(reinterpret_cast<const char*>(comment_string));
+    case kArchComment:
+      __ RecordComment(reinterpret_cast<const char*>(i.InputInt32(0)));
       break;
-    }
     case kArchDebugAbort:
       DCHECK(i.InputRegister(0) == edx);
       if (!frame_access_state()->has_frame()) {

src/compiler/instruction-selector-impl.h

@@ -299,8 +299,15 @@ class OperandGenerator {
       case IrOpcode::kNumberConstant:
         return Constant(OpParameter<double>(node->op()));
       case IrOpcode::kExternalConstant:
-      case IrOpcode::kComment:
         return Constant(OpParameter<ExternalReference>(node->op()));
+      case IrOpcode::kComment: {
+        // We cannot use {intptr_t} here, since the Constant constructor would
+        // be ambiguous on some architectures.
+        using ptrsize_int_t =
+            std::conditional<kPointerSize == 8, int64_t, int32_t>::type;
+        return Constant(reinterpret_cast<ptrsize_int_t>(
+            OpParameter<const char*>(node->op())));
+      }
       case IrOpcode::kHeapConstant:
         return Constant(HeapConstantOf(node->op()));
       case IrOpcode::kDeadValue: {

src/compiler/mips/code-generator-mips.cc

@@ -768,11 +768,9 @@ CodeGenerator::CodeGenResult CodeGenerator::AssembleArchInstruction(
     case kArchDebugBreak:
       __ stop("kArchDebugBreak");
       break;
-    case kArchComment: {
-      Address comment_string = i.InputExternalReference(0).address();
-      __ RecordComment(reinterpret_cast<const char*>(comment_string));
+    case kArchComment:
+      __ RecordComment(reinterpret_cast<const char*>(i.InputInt32(0)));
       break;
-    }
     case kArchNop:
     case kArchThrowTerminator:
       // don't emit code for nops.

src/compiler/mips64/code-generator-mips64.cc

@@ -788,11 +788,9 @@ CodeGenerator::CodeGenResult CodeGenerator::AssembleArchInstruction(
     case kArchDebugBreak:
       __ stop("kArchDebugBreak");
       break;
-    case kArchComment: {
-      Address comment_string = i.InputExternalReference(0).address();
-      __ RecordComment(reinterpret_cast<const char*>(comment_string));
+    case kArchComment:
+      __ RecordComment(reinterpret_cast<const char*>(i.InputInt64(0)));
       break;
-    }
     case kArchNop:
     case kArchThrowTerminator:
       // don't emit code for nops.

src/compiler/x64/code-generator-x64.cc

@@ -879,11 +879,9 @@ CodeGenerator::CodeGenResult CodeGenerator::AssembleArchInstruction(
     case kArchTableSwitch:
       AssembleArchTableSwitch(instr);
       break;
-    case kArchComment: {
-      Address comment_string = i.InputExternalReference(0).address();
-      __ RecordComment(reinterpret_cast<const char*>(comment_string));
+    case kArchComment:
+      __ RecordComment(reinterpret_cast<const char*>(i.InputInt64(0)));
       break;
-    }
     case kArchDebugAbort:
       DCHECK(i.InputRegister(0) == rdx);
       if (!frame_access_state()->has_frame()) {

test/mjsunit/code-comments.js

@@ -0,0 +1,26 @@
+// Copyright 2018 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --code-comments --print-code
+
+(function simple_test() {
+  function fib(n) {
+    return n < 2 ? n : fib(n - 1) + fib(n - 2);
+  }
+
+  // Call a number of times to trigger optimization.
+  for (let i = 0; i < 100; ++i) {
+    fib(8);
+  }
+})();
+
+(function test_asm() {
+  function asm() {
+    'use asm';
+    function f() {}
+    return f;
+  }
+
+  var m = asm();
+})();

test/mjsunit/mjsunit.status

@@ -418,6 +418,10 @@
   # https://bugs.chromium.org/p/v8/issues/detail?id=7102
   # Flaky due to huge string allocation.
   'regress/regress-748069': [SKIP],
+
+  # https://crbug.com/v8/7738
+  # Code comments currently leak memory.
+  'code-comments': [SKIP],
 }],  # 'asan == True'
 
 ##############################################################################