Skip to content

Commit 1995335

Browse files
backesCommit Bot
backes
authored and
Commit Bot
committed
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. [email protected] Bug: v8:7744 Change-Id: Ia6ec509e77d998df085ac7377cb24854354e3aa2 Reviewed-on: https://chromium-review.googlesource.com/1051235 Commit-Queue: Clemens Hammacher <[email protected]> Reviewed-by: Tobias Tebbi <[email protected]> Cr-Commit-Position: refs/heads/master@{#53100}
1 parent d951495 commit 1995335

9 files changed

Lines changed: 50 additions & 25 deletions

File tree

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -880,11 +880,9 @@ CodeGenerator::CodeGenResult CodeGenerator::AssembleArchInstruction(
880880
case kArchDebugBreak:
881881
__ stop("kArchDebugBreak");
882882
break;
883-
case kArchComment: {
884-
Address comment_string = i.InputExternalReference(0).address();
885-
__ RecordComment(reinterpret_cast<const char*>(comment_string));
883+
case kArchComment:
884+
__ RecordComment(reinterpret_cast<const char*>(i.InputInt32(0)));
886885
break;
887-
}
888886
case kArchThrowTerminator:
889887
DCHECK_EQ(LeaveCC, i.OutputSBit());
890888
unwinding_info_writer_.MarkBlockWillExit();

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -795,11 +795,9 @@ CodeGenerator::CodeGenResult CodeGenerator::AssembleArchInstruction(
795795
case kArchDebugBreak:
796796
__ Debug("kArchDebugBreak", 0, BREAK);
797797
break;
798-
case kArchComment: {
799-
Address comment_string = i.InputExternalReference(0).address();
800-
__ RecordComment(reinterpret_cast<const char*>(comment_string));
798+
case kArchComment:
799+
__ RecordComment(reinterpret_cast<const char*>(i.InputInt64(0)));
801800
break;
802-
}
803801
case kArchThrowTerminator:
804802
unwinding_info_writer_.MarkBlockWillExit();
805803
break;

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -771,11 +771,9 @@ CodeGenerator::CodeGenResult CodeGenerator::AssembleArchInstruction(
771771
case kArchTableSwitch:
772772
AssembleArchTableSwitch(instr);
773773
break;
774-
case kArchComment: {
775-
Address comment_string = i.InputExternalReference(0).address();
776-
__ RecordComment(reinterpret_cast<const char*>(comment_string));
774+
case kArchComment:
775+
__ RecordComment(reinterpret_cast<const char*>(i.InputInt32(0)));
777776
break;
778-
}
779777
case kArchDebugAbort:
780778
DCHECK(i.InputRegister(0) == edx);
781779
if (!frame_access_state()->has_frame()) {

‎src/compiler/instruction-selector-impl.h‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -299,8 +299,15 @@ class OperandGenerator {
299299
case IrOpcode::kNumberConstant:
300300
return Constant(OpParameter<double>(node->op()));
301301
case IrOpcode::kExternalConstant:
302-
case IrOpcode::kComment:
303302
return Constant(OpParameter<ExternalReference>(node->op()));
303+
case IrOpcode::kComment: {
304+
// We cannot use {intptr_t} here, since the Constant constructor would
305+
// be ambiguous on some architectures.
306+
using ptrsize_int_t =
307+
std::conditional<kPointerSize == 8, int64_t, int32_t>::type;
308+
return Constant(reinterpret_cast<ptrsize_int_t>(
309+
OpParameter<const char*>(node->op())));
310+
}
304311
case IrOpcode::kHeapConstant:
305312
return Constant(HeapConstantOf(node->op()));
306313
case IrOpcode::kDeadValue: {

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -768,11 +768,9 @@ CodeGenerator::CodeGenResult CodeGenerator::AssembleArchInstruction(
768768
case kArchDebugBreak:
769769
__ stop("kArchDebugBreak");
770770
break;
771-
case kArchComment: {
772-
Address comment_string = i.InputExternalReference(0).address();
773-
__ RecordComment(reinterpret_cast<const char*>(comment_string));
771+
case kArchComment:
772+
__ RecordComment(reinterpret_cast<const char*>(i.InputInt32(0)));
774773
break;
775-
}
776774
case kArchNop:
777775
case kArchThrowTerminator:
778776
// don't emit code for nops.

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -788,11 +788,9 @@ CodeGenerator::CodeGenResult CodeGenerator::AssembleArchInstruction(
788788
case kArchDebugBreak:
789789
__ stop("kArchDebugBreak");
790790
break;
791-
case kArchComment: {
792-
Address comment_string = i.InputExternalReference(0).address();
793-
__ RecordComment(reinterpret_cast<const char*>(comment_string));
791+
case kArchComment:
792+
__ RecordComment(reinterpret_cast<const char*>(i.InputInt64(0)));
794793
break;
795-
}
796794
case kArchNop:
797795
case kArchThrowTerminator:
798796
// don't emit code for nops.

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -879,11 +879,9 @@ CodeGenerator::CodeGenResult CodeGenerator::AssembleArchInstruction(
879879
case kArchTableSwitch:
880880
AssembleArchTableSwitch(instr);
881881
break;
882-
case kArchComment: {
883-
Address comment_string = i.InputExternalReference(0).address();
884-
__ RecordComment(reinterpret_cast<const char*>(comment_string));
882+
case kArchComment:
883+
__ RecordComment(reinterpret_cast<const char*>(i.InputInt64(0)));
885884
break;
886-
}
887885
case kArchDebugAbort:
888886
DCHECK(i.InputRegister(0) == rdx);
889887
if (!frame_access_state()->has_frame()) {

‎test/mjsunit/code-comments.js‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
// Copyright 2018 the V8 project authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
// Flags: --code-comments --print-code
6+
7+
(function simple_test() {
8+
function fib(n) {
9+
return n < 2 ? n : fib(n - 1) + fib(n - 2);
10+
}
11+
12+
// Call a number of times to trigger optimization.
13+
for (let i = 0; i < 100; ++i) {
14+
fib(8);
15+
}
16+
})();
17+
18+
(function test_asm() {
19+
function asm() {
20+
'use asm';
21+
function f() {}
22+
return f;
23+
}
24+
25+
var m = asm();
26+
})();

‎test/mjsunit/mjsunit.status‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -418,6 +418,10 @@
418418
# https://bugs.chromium.org/p/v8/issues/detail?id=7102
419419
# Flaky due to huge string allocation.
420420
'regress/regress-748069': [SKIP],
421+
422+
# https://crbug.com/v8/7738
423+
# Code comments currently leak memory.
424+
'code-comments': [SKIP],
421425
}], # 'asan == True'
422426

423427
##############################################################################

0 commit comments

Comments
 (0)