test: add test for postmortem metadata validation #17685

cjihrig · 2017-12-14T17:48:36Z

This commit adds a test to validate postmortem metadata. There are a lot more constants to be added, but I wanted to get feedback on the idea before spending the time on all of the constants.

The idea is that we can include a list of metadata constants used by llnode, mdb_v8, and any other postmortem tools (if there are any). When this test runs, it will let us know if the constants have gone missing. Currently, we only find that out if the ustack helper compilation breaks, or someone reports a bug to llnode/mdb_v8.

This test doesn't need to run on all platforms (it currently depends on nm), but it would be great if it ran on at least one platform that the V8 team tests on.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

test

richardlau · 2017-12-14T18:28:41Z

cc @nodejs/diagnostics @nodejs/post-mortem

richardlau · 2017-12-14T18:29:21Z

This test doesn't need to run on all platforms (it currently depends on nm),

Perhaps on Windows we could use dumpbin.

Trott · 2017-12-14T23:17:06Z

Nit: Maybe include a comment at the top of the test explaining what the test does?

bnoordhuis

I have some misgivings about this test:

It only catches deletions, not semantic changes. Some might say "better than nothing" but I say "false sense of security." Integration testing with llnode's test suite will give better results.
It's going to be tempting to just add everything from nm node | grep v8dbg even though llnode and mbd use only a fraction of it. That's not useful, just maintenance make-work, but so is updating it after every llnode or mdb change.
Who is going to fix it when it starts failing and in what time frame? I've mentioned elsewhere that I don't think updating the metadata script should be the responsibility of the V8 upgrader. Neither should it block upgrades.

bnoordhuis · 2017-12-15T02:38:22Z

test/parallel/test-postmortem-metadata.js

+if (nm.error && nm.error.errno === 'ENOENT')
+  common.skip('nm not found on system');
+
+const symbolRe = /\s(_v8dbg_.+)$/;


The underscore prefix is a macos-ism.

cjihrig · 2017-12-15T03:06:32Z

@bnoordhuis regarding your second point - I thought of that, and I've only added the metadata used by the most recent version of llnode. I just haven't pushed it up yet because there is already a missing constant.

Regarding your first point - I agree that test suite integration would be more in depth.
Regarding your third point - For the past few months I've been fixing these in nodejs/node-v8 because they only register as SmartOS compilation issues, which isn't the real underlying problem. Other than the original TurboFan upgrade, it's been a fairly smooth process. Even if this doesn't run in CI (although it could be marked flakey), I think it would be useful to have until we have something better.

UPDATE: pushed this up. The missing constant was v8dbg_frametype_JavaScriptFrame, which no longer exists (and doesn't appear to be needed) in V8 6.3. The only other difference from Node 8 is that v8dbg_frametype_ConstructEntryFrame would be replaced with v8dbg_frametype_EntryConstructFrame. So, theoretically, this could be backported to Node 8, although that probably isn't necessary.

joyeecheung

LGTM with the underscore prefix removed. Also we should mark this as flaky to avoid blocking V8 upgrades while being able to get notified when the constants were missing.

joyeecheung · 2017-12-15T03:36:31Z

cc @nodejs/post-mortem

bnoordhuis

LGTM if you mark it FLAKY.

bnoordhuis · 2017-12-15T04:56:13Z

test/parallel/test-postmortem-metadata.js

+const symbols = nm.stdout.toString().split('\n').map((line) => {
+  const match = line.match(symbolRe);
+
+  return match && match[1].replace(/^_/, '');


The _ isn't captured, the replace is a no-op.

cjihrig · 2017-12-15T15:25:26Z

nm seems to behave a little differently on AIX. I don't have access to any AIX machines though.

richardlau · 2017-12-15T17:52:43Z

nm seems to behave a little differently on AIX. I don't have access to any AIX machines though.

-bash-4.3$ nm /tmp/usenode.riclau/node-v8.9.3-aix-ppc64/bin/node
0654-210 /tmp/usenode.riclau/node-v8.9.3-aix-ppc64/bin/node is not valid in the current object file mode.
        Use the -X option to specify the desired object mode.
-bash-4.3$

nm on AIX reference: https://www.ibm.com/support/knowledgecenter/en/ssw_aix_71/com.ibm.aix.cmds4/nm.htm

The default is to process 32-bit object files (ignore 64-bit objects). The mode can also be set with the OBJECT_MODE environment variable. For example, OBJECT_MODE=64 causes nm to process any 64-bit objects and ignore 32-bit objects. The -X flag overrides the OBJECT_MODE variable.

So probably want either -X64 or -Xany. Also probably -B as well (so that the names are at the end of the line so the regular expression will correctly match), otherwise you get output like this:

v8dbg_bit_field3_dictionary_map_shift D   537608848

instead of (with -B):

 537608848 D v8dbg_bit_field3_dictionary_map_shift

cc @nodejs/platform-aix

cjihrig · 2017-12-15T18:16:01Z

Thanks @richardlau! CI with your suggestions: ~~https://ci.nodejs.org/job/node-test-pull-request/12137/~~ https://ci.nodejs.org/job/node-test-pull-request/12138/

cjihrig · 2017-12-18T13:02:14Z

All nits should be addressed. Also replaced a map()-filter() sequence with a single reduce(). CI: https://ci.nodejs.org/job/node-test-pull-request/12171/

This commit adds a test to validate postmortem debugging metadata. When this test runs, it can check for the presence of metadata constants used by tools such as llnode and mdb and report if any have accidentally been removed. PR-URL: nodejs#17685 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Richard Lau <[email protected]>

This commit adds a test to validate postmortem debugging metadata. When this test runs, it can check for the presence of metadata constants used by tools such as llnode and mdb and report if any have accidentally been removed. PR-URL: #17685 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Richard Lau <[email protected]>

This commit adds a test to validate postmortem debugging metadata. When this test runs, it can check for the presence of metadata constants used by tools such as llnode and mdb and report if any have accidentally been removed. PR-URL: nodejs/node#17685 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Richard Lau <[email protected]>

MylesBorins · 2018-01-23T07:11:48Z

This lands cleanly on v8.x and the tests pass so I landed in on to staging. For v6.x it was easy to resolve the conflict but tests failed locally

=== release test-postmortem-metadata ===
Path: parallel/test-postmortem-metadata
assert.js:84
  throw new assert.AssertionError({
  ^
AssertionError: Missing constants: v8dbg_class_FixedTypedArrayBase__external_pointer__Object,v8dbg_class_JSObject__internal_fields__uintptr_t,v8dbg_class_JSReceiver__raw_properties_or_hash__Object,v8dbg_class_SharedFunctionInfo__compiler_hints__int,v8dbg_class_SharedFunctionInfo__end_position__int,v8dbg_class_SharedFunctionInfo__internal_formal_parameter_count__int,v8dbg_class_SharedFunctionInfo__raw_name__Object,v8dbg_class_SharedFunctionInfo__start_position_and_type__int,v8dbg_class_ThinString__actual__String,v8dbg_context_idx_closure,v8dbg_context_idx_prev,v8dbg_context_min_slots,v8dbg_frametype_ConstructEntryFrame,v8dbg_namedictionary_prefix_start_index,v8dbg_namedictionaryshape_entry_size,v8dbg_namedictionaryshape_prefix_size,v8dbg_prop_attributes_mask,v8dbg_prop_attributes_shift,v8dbg_prop_attributes_DONT_ENUM,v8dbg_prop_attributes_DONT_ENUM,v8dbg_prop_attributes_NONE,v8dbg_prop_attributes_READ_ONLY,v8dbg_prop_kind_mask,v8dbg_prop_kind_Accessor,v8dbg_prop_kind_Data,v8dbg_prop_location_mask,v8dbg_prop_location_shift,v8dbg_prop_location_Descriptor,v8dbg_prop_location_Field,v8dbg_APIObjectType,v8dbg_SpecialAPIObjectType,v8dbg_ThinStringTag
    at Object.<anonymous> (/Users/mborins/code/node/v6.x/test/parallel/test-postmortem-metadata.js:34:8)
    at Module._compile (module.js:577:32)
    at Object.Module._extensions..js (module.js:586:10)
    at Module.load (module.js:494:32)
    at tryModuleLoad (module.js:453:12)
    at Function.Module._load (module.js:445:3)
    at Module.runMain (module.js:611:10)
    at run (bootstrap_node.js:387:7)
    at startup (bootstrap_node.js:153:9)
    at bootstrap_node.js:500:3
Command: out/Release/node /Users/mborins/code/node/v6.x/test/parallel/test-postmortem-metadata.js

Looks like it might be a pain to backport. Feel free to put dont-land-on-v6.x

This commit adds a test to validate postmortem debugging metadata. When this test runs, it can check for the presence of metadata constants used by tools such as llnode and mdb and report if any have accidentally been removed. PR-URL: #17685 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Richard Lau <[email protected]>

gibfahn · 2018-01-24T02:45:56Z

Depends on #16413, so will only go into 8.x if #16413 does.

This commit adds a test to validate postmortem debugging metadata. When this test runs, it can check for the presence of metadata constants used by tools such as llnode and mdb and report if any have accidentally been removed. PR-URL: #17685 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Richard Lau <[email protected]>

test-postmortem-metadata does not ever seem to fail on master. According to Michaël Zasso, when it fails locally for him during a V8 upgrade, he pings Colin Ihrig who fixes it almost immediately. There was some discussion about marking it flaky when it first landed but it's not clear to me that it is necessary. (We can always mark it flaky again if it turns out that it is necessary.) The reason I want this test removed from the status file for flaky tests is that I believe that file can and should be used as a list of tests to fix. But this test doesn't need any fixing. Refs: nodejs#17685

This commit adds a test to validate postmortem debugging metadata. When this test runs, it can check for the presence of metadata constants used by tools such as llnode and mdb and report if any have accidentally been removed. PR-URL: #17685 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Richard Lau <[email protected]>

jasnell approved these changes Dec 14, 2017

View reviewed changes

addaleax approved these changes Dec 14, 2017

View reviewed changes

bnoordhuis reviewed Dec 15, 2017

View reviewed changes

cjihrig force-pushed the metadata-test branch from 5120567 to 96fc168 Compare December 15, 2017 03:10

joyeecheung approved these changes Dec 15, 2017

View reviewed changes

cjihrig force-pushed the metadata-test branch 2 times, most recently from dfd6168 to 8efc0a5 Compare December 15, 2017 04:22

bnoordhuis approved these changes Dec 15, 2017

View reviewed changes

cjihrig force-pushed the metadata-test branch from 8efc0a5 to e70a787 Compare December 15, 2017 15:23

cjihrig force-pushed the metadata-test branch from e70a787 to f6000f9 Compare December 15, 2017 18:14

cjihrig force-pushed the metadata-test branch 4 times, most recently from b05f5d2 to 5708da8 Compare December 18, 2017 12:49

richardlau approved these changes Dec 18, 2017

View reviewed changes

cjihrig force-pushed the metadata-test branch from 5708da8 to 68c63a9 Compare December 18, 2017 14:25

cjihrig merged commit 68c63a9 into nodejs:master Dec 18, 2017

cjihrig deleted the metadata-test branch December 18, 2017 14:27

MylesBorins mentioned this pull request Jan 10, 2018

v9.4.0 proposal #18069

Merged

MylesBorins added land-on-v6.x and removed land-on-v6.x labels Jan 23, 2018

cjihrig added dont-land-on-v4.x and removed backport-requested-v6.x labels Jan 23, 2018

gibfahn mentioned this pull request Jan 24, 2018

[v8.x] deps: update V8 to 6.2 #16413

Closed

2 tasks

mmarchini mentioned this pull request Apr 24, 2018

Periodic test against master and canary nodejs/llnode#187

Closed

Trott mentioned this pull request Jun 2, 2018

test: remove postmortem test from flaky list #21092

Closed

2 tasks

MylesBorins mentioned this pull request Jul 9, 2018

v8.12.0 proposal #21593

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add test for postmortem metadata validation #17685

test: add test for postmortem metadata validation #17685

cjihrig commented Dec 14, 2017

richardlau commented Dec 14, 2017

richardlau commented Dec 14, 2017

Trott commented Dec 14, 2017

bnoordhuis left a comment •

edited

Loading

bnoordhuis Dec 15, 2017

cjihrig commented Dec 15, 2017 •

edited

Loading

joyeecheung left a comment

joyeecheung commented Dec 15, 2017

bnoordhuis left a comment

bnoordhuis Dec 15, 2017

cjihrig commented Dec 15, 2017

richardlau commented Dec 15, 2017

cjihrig commented Dec 15, 2017 •

edited

Loading

cjihrig commented Dec 18, 2017

MylesBorins commented Jan 23, 2018

gibfahn commented Jan 24, 2018

test: add test for postmortem metadata validation #17685

test: add test for postmortem metadata validation #17685

Conversation

cjihrig commented Dec 14, 2017

Checklist

Affected core subsystem(s)

richardlau commented Dec 14, 2017

richardlau commented Dec 14, 2017

Trott commented Dec 14, 2017

bnoordhuis left a comment • edited Loading

Choose a reason for hiding this comment

bnoordhuis Dec 15, 2017

Choose a reason for hiding this comment

cjihrig commented Dec 15, 2017 • edited Loading

joyeecheung left a comment

Choose a reason for hiding this comment

joyeecheung commented Dec 15, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Dec 15, 2017

Choose a reason for hiding this comment

cjihrig commented Dec 15, 2017

richardlau commented Dec 15, 2017

cjihrig commented Dec 15, 2017 • edited Loading

cjihrig commented Dec 18, 2017

MylesBorins commented Jan 23, 2018

gibfahn commented Jan 24, 2018

bnoordhuis left a comment •

edited

Loading

cjihrig commented Dec 15, 2017 •

edited

Loading

cjihrig commented Dec 15, 2017 •

edited

Loading