Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "src: enable detailed source positions in V8" #24394

Merged
merged 1 commit into from
Nov 17, 2018

Conversation

refack
Copy link
Contributor

@refack refack commented Nov 16, 2018

This reverts commit e2a8e32.
This reverts commit 715bbb9.

Fixes: #24393

Please 👍 to fast-track

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Nov 16, 2018
@refack refack added v8 engine Issues and PRs related to the V8 dependency. regression Issues related to regressions. labels Nov 16, 2018
@refack
Copy link
Contributor Author

refack commented Nov 16, 2018

@refack refack requested a review from targos November 16, 2018 14:46
@refack refack mentioned this pull request Nov 16, 2018
@refack
Copy link
Contributor Author

refack commented Nov 16, 2018

From #24393 (comment)

@targos:

Error in test compilation:

15:28:41 FAILED: obj/test/cctest/cctest_sources/test-cpu-profiler.o 
15:28:41 ../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF obj/test/cctest/cctest_sources/test-cpu-profiler.o.d -DV8_INTL_SUPPORT -DV8_DEPRECATION_WARNINGS -DUSE_UDEV -DUSE_AURA=1 -DUSE_GLIB=1 -DUSE_NSS_CERTS=1 -DUSE_X11=1 -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DCHROMIUM_BUILD -DFIELDTRIAL_TESTING_ENABLED -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_GNU_SOURCE -DCR_CLANG_REVISION=\"338452-1\" -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS -D_LIBCXXABI_DISABLE_VISIBILITY_ANNOTATIONS -DCR_LIBCXX_REVISION=332543 -DCR_LIBCXXABI_REVISION=331450 -DCR_SYSROOT_HASH=815a8c22f8657fe57d02e2c2d893bcdc25a243cf -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -DV8_DEPRECATION_WARNINGS -DV8_IMMINENT_DEPRECATION_WARNINGS -DV8_TYPED_ARRAY_MAX_SIZE_IN_HEAP=64 -DENABLE_GDB_JIT_INTERFACE -DENABLE_MINOR_MC -DV8_DEPRECATION_WARNINGS -DV8_IMMINENT_DEPRECATION_WARNINGS -DV8_INTL_SUPPORT -DV8_USE_SNAPSHOT -DV8_USE_EXTERNAL_STARTUP_DATA -DV8_CONCURRENT_MARKING -DV8_EMBEDDED_BUILTINS -DV8_TARGET_ARCH_X64 -DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 -DU_STATIC_IMPLEMENTATION -DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_FILE -DUCHAR_TYPE=uint16_t -I../.. -Igen -I../../include -Igen/include -I../.. -Igen -I../../include -I../../third_party/icu/source/common -I../../third_party/icu/source/i18n -fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector -Wno-builtin-macro-redefined -D__DATE__= -D__TIME__= -D__TIMESTAMP__= -funwind-tables -fPIC -B../../third_party/binutils/Linux_x64/Release/bin -pthread -fcolor-diagnostics -fmerge-all-constants -Xclang -mllvm -Xclang -instcombine-lower-dbg-declare=0 -no-canonical-prefixes -fcomplete-member-pointers -m64 -march=x86-64 -Wall -Werror -Wextra -Wimplicit-fallthrough -Wthread-safety -Wno-missing-field-initializers -Wno-unused-parameter -Wno-c++11-narrowing -Wno-covered-switch-default -Wno-unneeded-internal-declaration -Wno-undefined-var-template -Wno-nonportable-include-path -Wno-user-defined-warnings -Wno-unused-lambda-capture -Wno-null-pointer-arithmetic -Wno-enum-compare-switch -Wno-ignored-pragma-optimize -fno-omit-frame-pointer -g0 -fvisibility=hidden -Wheader-hygiene -Wstring-conversion -Wtautological-overlap-compare -Wmissing-field-initializers -Winconsistent-missing-override -Wunreachable-code -Wshorten-64-to-32 -O3 -fno-ident -fdata-sections -ffunction-sections -std=c++14 -fno-exceptions -fno-rtti -nostdinc++ -isystem../../buildtools/third_party/libc++/trunk/include -isystem../../buildtools/third_party/libc++abi/trunk/include --sysroot=../../build/linux/debian_sid_amd64-sysroot -fvisibility-inlines-hidden -c ../../test/cctest/test-cpu-profiler.cc -o obj/test/cctest/cctest_sources/test-cpu-profiler.o
15:28:41 ../../test/cctest/test-cpu-profiler.cc:2574:34: error: use of undeclared identifier 'GetSourcePositionEntryCount'
15:28:41     int non_detailed_positions = GetSourcePositionEntryCount(i_isolate, source);
15:28:41                                  ^
15:28:41 ../../test/cctest/test-cpu-profiler.cc:2579:30: error: use of undeclared identifier 'GetSourcePositionEntryCount'
15:28:41     int detailed_positions = GetSourcePositionEntryCount(i_isolate, source);
15:28:41                              ^
15:28:41 2 errors generated.

@refack refack added the fast-track PRs that do not need to wait for 48 hours to land. label Nov 16, 2018
@refack refack requested a review from Trott November 16, 2018 14:50
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rubber stamp LGTM.

@targos targos added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 17, 2018
@targos
Copy link
Member

targos commented Nov 17, 2018

@refack I believe this can land.

This reverts commit e2a8e32.
This reverts commit 715bbb9.

PR-URL: nodejs#24394
Fixes: nodejs#24393
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@refack
Copy link
Contributor Author

refack commented Nov 17, 2018

Extra sanity test V8 port land: https://ci.nodejs.org/job/node-test-commit-v8-linux/1866/

@refack refack removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 17, 2018
@refack refack merged commit 6adbe9a into nodejs:master Nov 17, 2018
@refack refack deleted the revert-v8-source-possition branch November 17, 2018 16:34
@refack
Copy link
Contributor Author

refack commented Nov 17, 2018

Post land CI shows a failed test:
mjsunit_wasm_jsapi_harness on ppcle-ubuntu1404

stdout:
/home/iojs/build/workspace/node-test-commit-v8-linux/nodes/ppcle-ubuntu1404/v8test/v8test/deps/v8/test/mjsunit/wasm/jsapi-harness.js:102: Error loading file
load("test/wasm-js/test/harness/wasm-constants.js");
^
Command: /home/iojs/build/workspace/node-test-commit-v8-linux/nodes/ppcle-ubuntu1404/v8test/v8test/deps/v8/out.gn/ppc64.release/d8 --test /home/iojs/build/workspace/node-test-commit-v8-linux/nodes/ppcle-ubuntu1404/v8test/v8test/deps/v8/test/mjsunit/mjsunit.js /home/iojs/build/workspace/node-test-commit-v8-linux/nodes/ppcle-ubuntu1404/v8test/v8test/deps/v8/test/mjsunit/wasm/jsapi-harness.js --random-seed=-50650634 --nohard-abort --expose-wasm --allow-natives-syntax

Rerunning: https://ci.nodejs.org/job/node-test-commit-v8-linux/1867/ ❌ (fail repetead in job 1867)

targos pushed a commit that referenced this pull request Nov 18, 2018
This reverts commit e2a8e32.
This reverts commit 715bbb9.

PR-URL: #24394
Fixes: #24393
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
addaleax pushed a commit that referenced this pull request Nov 23, 2018
Original commit message:

    [profiler] introduce API to enable detailed source positions

    This allows Node.js to enable detailed source positions for optimized code
    early on, without having to pass a flag string.

    R=petermarshall@chromium.org

    Change-Id: Ie74ea41f600cf6e31acbe802116df4976ccf1c75
    Reviewed-on: https://chromium-review.googlesource.com/c/1319757
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Reviewed-by: Peter Marshall <petermarshall@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#57380}

Refs: v8/v8@073073b

PR-URL: #24515
Refs: #24274
Refs: #24394
Refs: #24393
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Peter Marshall <petermarshall@chromium.org>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
addaleax pushed a commit that referenced this pull request Nov 23, 2018
PR-URL: #24515
Refs: #24274
Refs: #24394
Refs: #24393
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Peter Marshall <petermarshall@chromium.org>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
targos pushed a commit that referenced this pull request Nov 24, 2018
Original commit message:

    [profiler] introduce API to enable detailed source positions

    This allows Node.js to enable detailed source positions for optimized code
    early on, without having to pass a flag string.

    R=petermarshall@chromium.org

    Change-Id: Ie74ea41f600cf6e31acbe802116df4976ccf1c75
    Reviewed-on: https://chromium-review.googlesource.com/c/1319757
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Reviewed-by: Peter Marshall <petermarshall@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#57380}

Refs: v8/v8@073073b

PR-URL: #24515
Refs: #24274
Refs: #24394
Refs: #24393
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Peter Marshall <petermarshall@chromium.org>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
targos pushed a commit that referenced this pull request Nov 24, 2018
PR-URL: #24515
Refs: #24274
Refs: #24394
Refs: #24393
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Peter Marshall <petermarshall@chromium.org>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
This reverts commit e2a8e32.
This reverts commit 715bbb9.

PR-URL: #24394
Fixes: #24393
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
Original commit message:

    [profiler] introduce API to enable detailed source positions

    This allows Node.js to enable detailed source positions for optimized code
    early on, without having to pass a flag string.

    R=petermarshall@chromium.org

    Change-Id: Ie74ea41f600cf6e31acbe802116df4976ccf1c75
    Reviewed-on: https://chromium-review.googlesource.com/c/1319757
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Reviewed-by: Peter Marshall <petermarshall@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#57380}

Refs: v8/v8@073073b

PR-URL: #24515
Refs: #24274
Refs: #24394
Refs: #24393
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Peter Marshall <petermarshall@chromium.org>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
PR-URL: #24515
Refs: #24274
Refs: #24394
Refs: #24393
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Peter Marshall <petermarshall@chromium.org>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Dec 4, 2018
Original commit message:

    [profiler] introduce API to enable detailed source positions

    This allows Node.js to enable detailed source positions for optimized code
    early on, without having to pass a flag string.

    R=petermarshall@chromium.org

    Change-Id: Ie74ea41f600cf6e31acbe802116df4976ccf1c75
    Reviewed-on: https://chromium-review.googlesource.com/c/1319757
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Reviewed-by: Peter Marshall <petermarshall@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#57380}

Refs: v8/v8@073073b

PR-URL: nodejs#24515
Refs: nodejs#24274
Refs: nodejs#24394
Refs: nodejs#24393
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Peter Marshall <petermarshall@chromium.org>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Dec 5, 2018
4 tasks
pull bot pushed a commit to shakir-abdo/node that referenced this pull request Dec 6, 2018
Original commit message:

    [profiler] introduce API to enable detailed source positions

    This allows Node.js to enable detailed source positions for optimized code
    early on, without having to pass a flag string.

    R=petermarshall@chromium.org

    Change-Id: Ie74ea41f600cf6e31acbe802116df4976ccf1c75
    Reviewed-on: https://chromium-review.googlesource.com/c/1319757
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Reviewed-by: Peter Marshall <petermarshall@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#57380}

Refs: v8/v8@073073b

PR-URL: nodejs#24515
Refs: nodejs#24274
Refs: nodejs#24394
Refs: nodejs#24393
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Peter Marshall <petermarshall@chromium.org>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
Original commit message:

    [profiler] introduce API to enable detailed source positions

    This allows Node.js to enable detailed source positions for optimized code
    early on, without having to pass a flag string.

    R=petermarshall@chromium.org

    Change-Id: Ie74ea41f600cf6e31acbe802116df4976ccf1c75
    Reviewed-on: https://chromium-review.googlesource.com/c/1319757
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Reviewed-by: Peter Marshall <petermarshall@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#57380}

Refs: v8/v8@073073b

PR-URL: nodejs#24515
Refs: nodejs#24274
Refs: nodejs#24394
Refs: nodejs#24393
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Peter Marshall <petermarshall@chromium.org>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
PR-URL: nodejs#24515
Refs: nodejs#24274
Refs: nodejs#24394
Refs: nodejs#24393
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Peter Marshall <petermarshall@chromium.org>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
Original commit message:

    [profiler] introduce API to enable detailed source positions

    This allows Node.js to enable detailed source positions for optimized code
    early on, without having to pass a flag string.

    R=petermarshall@chromium.org

    Change-Id: Ie74ea41f600cf6e31acbe802116df4976ccf1c75
    Reviewed-on: https://chromium-review.googlesource.com/c/1319757
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Reviewed-by: Peter Marshall <petermarshall@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#57380}

Refs: v8/v8@073073b

PR-URL: nodejs#24515
Refs: nodejs#24274
Refs: nodejs#24394
Refs: nodejs#24393
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Peter Marshall <petermarshall@chromium.org>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fast-track PRs that do not need to wait for 48 hours to land. lib / src Issues and PRs related to general changes in the lib or src directory. regression Issues related to regressions. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

V8: test-v8 is broken
6 participants