Skip to content

Commit aee5fb0

Browse files
Vasili SkurydzinV8 LUCI CQ
Vasili Skurydzin
authored and
V8 LUCI CQ
committed
Reland "Use BigInts in processor.mjs and related code to avoid unsafe ints in calculations"
This is a reland of commit efc1a98 Changes since revert: - Handle "shared-library", "code-{deopt,move,delete}", "feedback-vector", "sfi-move" events Original change's description: > Use BigInts in processor.mjs and related code to avoid unsafe ints in calculations > > Bug: v8:13440 > Change-Id: Ie03b831b511a49fb475b9f303ef8662189bdaf3d > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4017455 > Reviewed-by: Camillo Bruni <[email protected]> > Commit-Queue: Camillo Bruni <[email protected]> > Cr-Commit-Position: refs/heads/main@{#84698} Change-Id: If45d38526cab887a59f60e3becfbcb084c3d41d0 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4086641 Reviewed-by: Camillo Bruni <[email protected]> Commit-Queue: Vasili Skurydzin <[email protected]> Cr-Commit-Position: refs/heads/main@{#84939}
1 parent 0b9fa06 commit aee5fb0

6 files changed

Lines changed: 143 additions & 45 deletions

File tree

‎test/mjsunit/tools/processor-bigint.mjs‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
// Copyright 2020 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: --logfile='+' --log --log-maps --log-ic --log-code
6+
// Flags: --log-function-events --no-predictable
7+
8+
import { Processor } from "../../../tools/system-analyzer/processor.mjs";
9+
10+
// log code start
11+
function doWork() {
12+
let array = [];
13+
for (let i = 0; i < 500; i++) {
14+
doWorkStep(i, array);
15+
}
16+
let sum = 0;
17+
for (let i = 0; i < 500; i++) {
18+
sum += array[i]["property" + i];
19+
}
20+
return sum;
21+
}
22+
23+
function doWorkStep(i, array) {
24+
const obj = {
25+
["property" + i]: i,
26+
};
27+
array.push(obj);
28+
obj.custom1 = 1;
29+
obj.custom2 = 2;
30+
}
31+
32+
const result = doWork();
33+
// log code end
34+
35+
const logString = d8.log.getAndStop();
36+
assertTrue(logString.length > 0);
37+
const useBigInts = true;
38+
const processor = new Processor(useBigInts);
39+
await processor.processChunk(logString);
40+
await processor.finalize();
41+
42+
const maps = processor.mapTimeline;
43+
const ics = processor.icTimeline;
44+
const scripts = processor.scripts;
45+
46+
(function testResults() {
47+
assertEquals(result, 124750);
48+
assertTrue(maps.length > 0);
49+
assertTrue(ics.length > 0);
50+
assertTrue(scripts.length > 0);
51+
})();
52+
53+
(function testIcKeys() {
54+
const keys = new Set();
55+
ics.forEach(ic => keys.add(ic.key));
56+
assertTrue(keys.has("custom1"));
57+
assertTrue(keys.has("custom2"));
58+
assertTrue(keys.has("push"));
59+
})();

‎tools/codemap.mjs‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,12 @@ export class CodeMap {
6565
*/
6666
pages_ = new Set();
6767

68+
constructor(useBigInt=false) {
69+
this.useBigInt = useBigInt;
70+
this.kPageSize = useBigInt ? BigInt(kPageSize) : kPageSize;
71+
this.kOne = useBigInt ? 1n : 1;
72+
this.kZero = useBigInt ? 0n : 0;
73+
}
6874

6975
/**
7076
* Adds a code entry that might overlap with static code (e.g. for builtins).
@@ -73,7 +79,7 @@ export class CodeMap {
7379
* @param {CodeEntry} codeEntry Code entry object.
7480
*/
7581
addAnyCode(start, codeEntry) {
76-
const pageAddr = (start / kPageSize) | 0;
82+
const pageAddr = (start / this.kPageSize) | this.kZero;
7783
if (!this.pages_.has(pageAddr)) return this.addCode(start, codeEntry);
7884
// We might have loaded static code (builtins, bytecode handlers)
7985
// and we get more information later in v8.log with code-creation events.
@@ -147,8 +153,8 @@ export class CodeMap {
147153
* @private
148154
*/
149155
markPages_(start, end) {
150-
for (let addr = start; addr <= end; addr += kPageSize) {
151-
this.pages_.add((addr / kPageSize) | 0);
156+
for (let addr = start; addr <= end; addr += this.kPageSize) {
157+
this.pages_.add((addr / this.kPageSize) | this.kZero);
152158
}
153159
}
154160

@@ -157,13 +163,13 @@ export class CodeMap {
157163
*/
158164
deleteAllCoveredNodes_(tree, start, end) {
159165
const to_delete = [];
160-
let addr = end - 1;
166+
let addr = end - this.kOne;
161167
while (addr >= start) {
162168
const node = tree.findGreatestLessThan(addr);
163169
if (node === null) break;
164170
const start2 = node.key, end2 = start2 + node.value.size;
165171
if (start2 < end && start < end2) to_delete.push(start2);
166-
addr = start2 - 1;
172+
addr = start2 - this.kOne;
167173
}
168174
for (let i = 0, l = to_delete.length; i < l; ++i) tree.remove(to_delete[i]);
169175
}
@@ -191,7 +197,7 @@ export class CodeMap {
191197
* @param {number} addr Address.
192198
*/
193199
findAddress(addr) {
194-
const pageAddr = (addr / kPageSize) | 0;
200+
const pageAddr = (addr / this.kPageSize) | this.kZero;
195201
if (this.pages_.has(pageAddr)) {
196202
// Static code entries can contain "holes" of unnamed code.
197203
// In this case, the whole library is assigned to this address.

‎tools/logreader.mjs‎

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,16 @@
3535
export function parseString(field) { return field };
3636
export const parseVarArgs = 'parse-var-args';
3737

38+
// Checks fields for numbers that are not safe integers. Returns true if any are
39+
// found.
40+
function containsUnsafeInts(fields) {
41+
for (let i = 0; i < fields.length; i++) {
42+
let field = fields[i];
43+
if ('number' == typeof(field) && !Number.isSafeInteger(field)) return true;
44+
}
45+
return false;
46+
}
47+
3848
/**
3949
* Base class for processing log files.
4050
*
@@ -44,7 +54,7 @@ export const parseVarArgs = 'parse-var-args';
4454
* @constructor
4555
*/
4656
export class LogReader {
47-
constructor(timedRange=false, pairwiseTimedRange=false) {
57+
constructor(timedRange=false, pairwiseTimedRange=false, useBigInt=false) {
4858
this.dispatchTable_ = new Map();
4959
this.timedRange_ = timedRange;
5060
this.pairwiseTimedRange_ = pairwiseTimedRange;
@@ -54,6 +64,11 @@ export class LogReader {
5464
// Variables for tracking of 'current-time' log entries:
5565
this.hasSeenTimerMarker_ = false;
5666
this.logLinesSinceLastTimerMarker_ = [];
67+
// Flag to parse all numeric fields as BigInt to avoid arithmetic errors
68+
// caused by memory addresses being greater than MAX_SAFE_INTEGER
69+
this.useBigInt = useBigInt;
70+
this.parseFrame = useBigInt ? BigInt : parseInt;
71+
this.hasSeenUnsafeIntegers = false;
5772
}
5873

5974
/**
@@ -180,11 +195,11 @@ export class LogReader {
180195
const firstChar = frame[0];
181196
if (firstChar === '+' || firstChar === '-') {
182197
// An offset from the previous frame.
183-
prevFrame += parseInt(frame, 16);
198+
prevFrame += this.parseFrame(frame);
184199
fullStack.push(prevFrame);
185200
// Filter out possible 'overflow' string.
186201
} else if (firstChar !== 'o') {
187-
fullStack.push(parseInt(frame, 16));
202+
fullStack.push(this.parseFrame(frame));
188203
} else {
189204
console.error(`Dropping unknown tick frame: ${frame}`);
190205
}
@@ -216,6 +231,12 @@ export class LogReader {
216231
parsedFields[i] = parser(fields[1 + i]);
217232
}
218233
}
234+
if (!this.useBigInt) {
235+
if (!this.hasSeenUnsafeIntegers && containsUnsafeInts(parsedFields)) {
236+
console.warn(`Log line containts unsafe integers: ${fields}`);
237+
this.hasSeenUnsafeIntegers = true;
238+
}
239+
}
219240
// Run the processor.
220241
await dispatch.processor(...parsedFields);
221242
}

‎tools/profile.mjs‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -305,14 +305,18 @@ const kProfileOperationTick = 2;
305305
* @constructor
306306
*/
307307
export class Profile {
308-
codeMap_ = new CodeMap();
309308
topDownTree_ = new CallTree();
310309
bottomUpTree_ = new CallTree();
311310
c_entries_ = {__proto__:null};
312311
scripts_ = [];
313312
urlToScript_ = new Map();
314313
warnings = new Set();
315314

315+
constructor(useBigInt=false) {
316+
this.useBigInt = useBigInt;
317+
this.codeMap_ = new CodeMap(useBigInt);
318+
}
319+
316320
serializeVMSymbols() {
317321
let result = this.codeMap_.getAllStaticEntriesWithAddresses();
318322
result.concat(this.codeMap_.getAllLibraryEntriesWithAddresses())
@@ -513,7 +517,7 @@ export class Profile {
513517
// it is safe to put them in a single code map.
514518
let func = this.codeMap_.findDynamicEntryByStartAddress(funcAddr);
515519
if (func === null) {
516-
func = new FunctionEntry(name);
520+
func = new FunctionEntry(name, this.useBigInt);
517521
this.codeMap_.addCode(funcAddr, func);
518522
} else if (func.name !== name) {
519523
// Function object has been overwritten with a new one.
@@ -961,8 +965,8 @@ class FunctionEntry extends CodeEntry {
961965
/** @type {Set<DynamicCodeEntry>} */
962966
_codeEntries = new Set();
963967

964-
constructor(name) {
965-
super(0, name);
968+
constructor(name, useBigInt=false) {
969+
super(useBigInt ? 0n : 0, name);
966970
const index = name.lastIndexOf(' ');
967971
this.functionName = 1 <= index ? name.substring(0, index) : '<anonymous>';
968972
}

‎tools/system-analyzer/log/tick.mjs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ export class TickLogEntry extends LogEntry {
4242
return 'Idle';
4343
}
4444
const topOfStack = processedStack[0];
45-
if (typeof topOfStack === 'number') {
45+
if (typeof topOfStack === 'number' || typeof topOfStack === 'bigint') {
4646
// TODO(cbruni): Handle VmStack and native ticks better.
4747
return 'Other';
4848
}

‎tools/system-analyzer/processor.mjs‎

Lines changed: 39 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,6 @@ class AsyncConsumer {
4747
}
4848

4949
export class Processor extends LogReader {
50-
_profile = new Profile();
5150
_codeTimeline = new Timeline();
5251
_deoptTimeline = new Timeline();
5352
_icTimeline = new Timeline();
@@ -70,12 +69,16 @@ export class Processor extends LogReader {
7069

7170
MAJOR_VERSION = 7;
7271
MINOR_VERSION = 6;
73-
constructor() {
74-
super();
72+
constructor(useBigInt = false) {
73+
super(false, false, useBigInt);
74+
this.useBigInt = useBigInt;
75+
this.kZero = useBigInt ? 0n : 0;
76+
this.parseAddress = useBigInt ? BigInt : parseInt;
7577
this._chunkConsumer =
7678
new AsyncConsumer((chunk) => this._processChunk(chunk));
79+
this._profile = new Profile(useBigInt);
7780
const propertyICParser = [
78-
parseInt, parseInt, parseInt, parseInt, parseString, parseString,
81+
this.parseAddress, parseInt, parseInt, parseInt, parseString, parseString,
7982
parseString, parseString, parseString, parseString
8083
];
8184
this.setDispatchTable({
@@ -88,58 +91,63 @@ export class Processor extends LogReader {
8891
processor: this.processV8Version,
8992
},
9093
'shared-library': {
91-
parsers: [parseString, parseInt, parseInt, parseInt],
94+
parsers: [
95+
parseString, this.parseAddress, this.parseAddress, this.parseAddress
96+
],
9297
processor: this.processSharedLibrary.bind(this),
9398
isAsync: true,
9499
},
95100
'code-creation': {
96101
parsers: [
97-
parseString, parseInt, parseInt, parseInt, parseInt, parseString,
98-
parseVarArgs
102+
parseString, parseInt, parseInt, this.parseAddress, this.parseAddress,
103+
parseString, parseVarArgs
99104
],
100105
processor: this.processCodeCreation
101106
},
102107
'code-deopt': {
103108
parsers: [
104-
parseInt, parseInt, parseInt, parseInt, parseInt, parseString,
105-
parseString, parseString
109+
parseInt, parseInt, this.parseAddress, parseInt, parseInt,
110+
parseString, parseString, parseString
106111
],
107112
processor: this.processCodeDeopt
108113
},
109-
'code-move':
110-
{parsers: [parseInt, parseInt], processor: this.processCodeMove},
111-
'code-delete': {parsers: [parseInt], processor: this.processCodeDelete},
114+
'code-move': {
115+
parsers: [this.parseAddress, this.parseAddress],
116+
processor: this.processCodeMove
117+
},
118+
'code-delete':
119+
{parsers: [this.parseAddress], processor: this.processCodeDelete},
112120
'code-source-info': {
113121
parsers: [
114-
parseInt, parseInt, parseInt, parseInt, parseString, parseString,
115-
parseString
122+
this.parseAddress, parseInt, parseInt, parseInt, parseString,
123+
parseString, parseString
116124
],
117125
processor: this.processCodeSourceInfo
118126
},
119127
'code-disassemble': {
120-
parsers: [
121-
parseInt,
122-
parseString,
123-
parseString,
124-
],
128+
parsers: [this.parseAddress, parseString, parseString],
125129
processor: this.processCodeDisassemble
126130
},
127131
'feedback-vector': {
128132
parsers: [
129-
parseInt, parseString, parseInt, parseInt, parseString, parseString,
130-
parseInt, parseInt, parseString
133+
parseInt, parseString, parseInt, this.parseAddress, parseString,
134+
parseString, parseInt, parseInt, parseString
131135
],
132136
processor: this.processFeedbackVector
133137
},
134138
'script-source': {
135139
parsers: [parseInt, parseString, parseString],
136140
processor: this.processScriptSource
137141
},
138-
'sfi-move':
139-
{parsers: [parseInt, parseInt], processor: this.processFunctionMove},
142+
'sfi-move': {
143+
parsers: [this.parseAddress, this.parseAddress],
144+
processor: this.processFunctionMove
145+
},
140146
'tick': {
141-
parsers:
142-
[parseInt, parseInt, parseInt, parseInt, parseInt, parseVarArgs],
147+
parsers: [
148+
this.parseAddress, parseInt, parseInt, this.parseAddress, parseInt,
149+
parseVarArgs
150+
],
143151
processor: this.processTick
144152
},
145153
'active-runtime-timer': undefined,
@@ -157,8 +165,8 @@ export class Processor extends LogReader {
157165
{parsers: [parseInt, parseString], processor: this.processMapCreate},
158166
'map': {
159167
parsers: [
160-
parseString, parseInt, parseString, parseString, parseInt, parseInt,
161-
parseInt, parseString, parseString
168+
parseString, parseInt, parseString, parseString, this.parseAddress,
169+
parseInt, parseInt, parseString, parseString
162170
],
163171
processor: this.processMap
164172
},
@@ -352,7 +360,7 @@ export class Processor extends LogReader {
352360
let profilerEntry;
353361
let stateName = '';
354362
if (maybe_func.length) {
355-
const funcAddr = parseInt(maybe_func[0]);
363+
const funcAddr = this.parseAddress(maybe_func[0]);
356364
stateName = maybe_func[1] ?? '';
357365
const state = Profile.parseState(maybe_func[1]);
358366
profilerEntry = this._profile.addFuncCode(
@@ -404,7 +412,7 @@ export class Processor extends LogReader {
404412
optimization_tier, invocation_count, profiler_ticks, fbv_string) {
405413
const profCodeEntry = this._profile.findEntry(instructionStart);
406414
if (!profCodeEntry) {
407-
console.warn('Didn\'t find code for FBV', {fbv, instructionStart});
415+
console.warn('Didn\'t find code for FBV', {fbv_string, instructionStart});
408416
return;
409417
}
410418
const fbv = new FeedbackVectorEntry(
@@ -439,13 +447,13 @@ export class Processor extends LogReader {
439447
// that a callback calls itself. Instead we use tos_or_external_callback,
440448
// as simply resetting PC will produce unaccounted ticks.
441449
pc = tos_or_external_callback;
442-
tos_or_external_callback = 0;
450+
tos_or_external_callback = this.kZero;
443451
} else if (tos_or_external_callback) {
444452
// Find out, if top of stack was pointing inside a JS function
445453
// meaning that we have encountered a frameless invocation.
446454
const funcEntry = this._profile.findEntry(tos_or_external_callback);
447455
if (!funcEntry?.isJSFunction?.()) {
448-
tos_or_external_callback = 0;
456+
tos_or_external_callback = this.kZero;
449457
}
450458
}
451459
const entryStack = this._profile.recordTick(

0 commit comments

Comments
 (0)