build: re-enable ASAN Action using clang #32776

mmarchini · 2020-04-11T01:41:53Z

clang's linker seems to use considerably less memory than gcc, allowing
us to run on Actions without running out of memory.

Checklist

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

Signed-off-by: Matheus Marchini <mmarchini@netflix.com>

clang's linker seems to use considerably less memory than gcc, allowing us to run on Actions without running out of memory. Signed-off-by: Matheus Marchini <mmarchini@netflix.com>

richardlau · 2020-04-11T03:00:16Z

This deliberately doesn’t run any js tests?

mmarchini · 2020-04-11T05:54:04Z

This deliberately doesn’t run any js tests?

Yes, only cctests are passing for now since we still have asan violations to fix in our codebase. Once we fix those we can enable js tests as well. This just makes sure we don't regress our build our cctests in the future.

richardlau · 2020-04-11T11:17:36Z

It looks like the re-added ASAN workflow completed in 50mins, which is much better than before when it was run in debug mode (and took 2.5+ hours) and is in line with the other build and test workflows. Still do we want to consider only running this for non-doc-only changes?

gengjiawen · 2020-04-11T13:29:30Z

It looks like the re-added ASAN workflow completed in 50mins, which is much better than before when it was run in debug mode (and took 2.5+ hours) and is in line with the other build and test workflows. Still do we want to consider only running this for non-doc-only changes?

+1 for this. But I am thinking add js test back with continue-on-error, remove it when we resolve all of it.

mmarchini · 2020-04-11T19:32:59Z

Running only on non-doc-only changes makes sense as long as we do it for the other build/test workflows as well. This should be a separate pull request.

I don't think continue on error is useful until GitHub distinguishes it from actual successful results, so I'm not introducing it on this pull request.

Signed-off-by: Matheus Marchini <mmarchini@netflix.com> PR-URL: #32776 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

clang's linker seems to use considerably less memory than gcc, allowing us to run on Actions without running out of memory. Signed-off-by: Matheus Marchini <mmarchini@netflix.com> PR-URL: #32776 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

mmarchini · 2020-04-13T22:42:11Z

Landed in 61e589f...81c03bc

Signed-off-by: Matheus Marchini <mmarchini@netflix.com> PR-URL: #32776 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

clang's linker seems to use considerably less memory than gcc, allowing us to run on Actions without running out of memory. Signed-off-by: Matheus Marchini <mmarchini@netflix.com> PR-URL: #32776 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Signed-off-by: Matheus Marchini <mmarchini@netflix.com> PR-URL: nodejs#32776 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

addaleax · 2020-04-28T03:35:41Z

@mmarchini Btw, with

the ASAN build passes for me with the exception of 2 tests that measure memory usage… do you have any plans to extend the ASAN build to a full test run?

gengjiawen · 2020-04-28T03:48:04Z

@mmarchini Btw, with

wasi: use free() to release preopen array #33110

src: use unique_ptr for CachedData in ContextifyScript::New #33113

test: add missing calls to napi_async_destroy #33114

test: fix out-of-bound reads from invalid sizeof usage #33115

chromium-review.googlesource.com/c/v8/v8/+/2169646

the ASAN build passes for me with the exception of 2 tests that measure memory usage… do you have any plans to extend the ASAN build to a full test run?

Which env are you using, last time I checked, 30 test failed.

  System:
    OS: Linux 5.0 Ubuntu 19.10 (Eoan Ermine)
    CPU: (2) x64 Intel(R) Xeon(R) CPU E5-2673 v4 @ 2.30GHz
    Memory: 3.02 GB / 6.78 GB
    Container: Yes
    Shell: 5.0.3 - /bin/bash
  Binaries:
    Node: 13.12.0 - /usr/local/bin/node
    Yarn: 1.22.4 - /usr/local/bin/yarn
    npm: 6.14.4 - /usr/local/bin/npm
  Managers:
    Apt: 1.9.4 - /usr/bin/apt
    pip3: 20.0.2 - /usr/local/bin/pip3
  Utilities:
    CMake: 3.16.3 - /usr/local/bin/cmake
    Make: 4.2.1 - /usr/bin/make
    GCC: 9.2.1 - /usr/bin/gcc
    Git: 2.20.1 - /usr/bin/git
    Clang: 9.0.0-2 - /usr/bin/clang

https://github.com/gengjiawen/node-github-workflow/runs/551284714?check_suite_focus=true

addaleax · 2020-04-28T03:56:21Z

@gengjiawen The system I tested on uses Clang 8 and Ubuntu 16.04, but I’d assume that the failures aren’t all too platform-dependent.

mmarchini · 2020-04-28T05:25:01Z

@gengjiawen are you using GCC? I noticed some likely false negatives when building with gcc.

@addaleax yes, the plan is to enable more test suites. Thank you for looking into it! Maybe we can allow the remaining two tests to fail until we figure them out. Or just enable a subset of JS tests until we fix those, for example, assuming the failing tests are on parallel we only enable sequential.

gengjiawen · 2020-04-28T06:26:55Z

@gengjiawen are you using GCC? I noticed some likely false negatives when building with gcc.

Nope, clang 9. But I am using debug asan build. Perhaps this the reason since it's more slower.

Maybe we can allow the remaining two tests to fail until we figure them out.

Another option is if we detect asan build (via process.config) in those two tests, skip the related test and add todo.

mmarchini · 2020-04-28T06:48:56Z

Another option is if we detect asan build (via process.config) in those two tests, skip the related test and add todo

Yes, that's what I meant :)

Signed-off-by: Matheus Marchini <mmarchini@netflix.com> PR-URL: #32776 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

clang's linker seems to use considerably less memory than gcc, allowing us to run on Actions without running out of memory. Signed-off-by: Matheus Marchini <mmarchini@netflix.com> PR-URL: #32776 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Signed-off-by: Matheus Marchini <mmarchini@netflix.com> PR-URL: #32776 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Running tests with an ASAN build leads to increased memory usage, rendering the memory usage assumptions in the test invalid. Refs: nodejs#32776 (comment)

Running tests with an ASAN build leads to increased memory usage, rendering the memory usage assumptions in the test invalid. Refs: #32776 (comment) PR-URL: #33129 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

Refs: nodejs#32776

Refs: #32776 PR-URL: #33170 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me>

mmarchini added 2 commits April 10, 2020 16:07

build: use same flags as V8 for ASAN

1264045

Signed-off-by: Matheus Marchini <mmarchini@netflix.com>

build: re-enable ASAN Action using clang

c39affd

clang's linker seems to use considerably less memory than gcc, allowing us to run on Actions without running out of memory. Signed-off-by: Matheus Marchini <mmarchini@netflix.com>

nodejs-github-bot added build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. labels Apr 11, 2020

mmarchini changed the title ~~Reenable asan~~ build: re-enable ASAN Action using clang Apr 11, 2020

addaleax approved these changes Apr 11, 2020

View reviewed changes

gengjiawen approved these changes Apr 11, 2020

View reviewed changes

cjihrig approved these changes Apr 11, 2020

View reviewed changes

mmarchini closed this Apr 13, 2020

BethGriggs mentioned this pull request Apr 14, 2020

2020-04-21, Version 14.0.0 (Current) #32181

Merged

targos added dont-land-on-v10.x labels Apr 25, 2020

BridgeAR mentioned this pull request Apr 28, 2020

v13.14.0 proposal #33122

Merged

addaleax mentioned this pull request Apr 28, 2020

test: skip memory usage tests when ASAN is enabled #33129

Closed

3 tasks

addaleax mentioned this pull request Apr 30, 2020

build: run full test suite in ASAN action #33170

Closed

2 tasks

targos mentioned this pull request May 2, 2020

v12.17.0 release proposal #33197

Merged

addaleax added a commit to addaleax/node that referenced this pull request May 19, 2020

build: run full test suite in ASAN action

a53c11d

Refs: nodejs#32776

addaleax added a commit that referenced this pull request May 19, 2020

build: run full test suite in ASAN action

7c755b0

Refs: #32776 PR-URL: #33170 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me>

codebytere pushed a commit that referenced this pull request May 21, 2020

build: run full test suite in ASAN action

02e8c14

Refs: #32776 PR-URL: #33170 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me>

mmarchini mentioned this pull request Jan 29, 2022

build: use Clang 12 for test-asan workflow #40238

Closed

aduh95 mentioned this pull request Jun 11, 2022

gcc fails to compile with ASAN enabled #43370

Closed

H4ad mentioned this pull request Mar 21, 2024

build: use clang over g++ for ubsan #52169

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: re-enable ASAN Action using clang #32776

build: re-enable ASAN Action using clang #32776

mmarchini commented Apr 11, 2020 •

edited

richardlau commented Apr 11, 2020

mmarchini commented Apr 11, 2020 •

edited

richardlau commented Apr 11, 2020

gengjiawen commented Apr 11, 2020

mmarchini commented Apr 11, 2020 •

edited

mmarchini commented Apr 13, 2020

addaleax commented Apr 28, 2020 •

edited

gengjiawen commented Apr 28, 2020

addaleax commented Apr 28, 2020

mmarchini commented Apr 28, 2020

gengjiawen commented Apr 28, 2020

mmarchini commented Apr 28, 2020

build: re-enable ASAN Action using clang #32776

build: re-enable ASAN Action using clang #32776

Conversation

mmarchini commented Apr 11, 2020 • edited

Checklist

richardlau commented Apr 11, 2020

mmarchini commented Apr 11, 2020 • edited

richardlau commented Apr 11, 2020

gengjiawen commented Apr 11, 2020

mmarchini commented Apr 11, 2020 • edited

mmarchini commented Apr 13, 2020

addaleax commented Apr 28, 2020 • edited

gengjiawen commented Apr 28, 2020

addaleax commented Apr 28, 2020

mmarchini commented Apr 28, 2020

gengjiawen commented Apr 28, 2020

mmarchini commented Apr 28, 2020

mmarchini commented Apr 11, 2020 •

edited

mmarchini commented Apr 11, 2020 •

edited

mmarchini commented Apr 11, 2020 •

edited

addaleax commented Apr 28, 2020 •

edited