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

build: re-enable ASAN Action using clang #32776

Closed
wants to merge 2 commits into from

Conversation

mmarchini
Copy link
Contributor

@mmarchini mmarchini commented Apr 11, 2020

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

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 <[email protected]>
@nodejs-github-bot 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 mmarchini changed the title Reenable asan build: re-enable ASAN Action using clang Apr 11, 2020
@richardlau
Copy link
Member

This deliberately doesn’t run any js tests?

@mmarchini
Copy link
Contributor Author

mmarchini commented Apr 11, 2020

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

mmarchini commented Apr 11, 2020

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.

mmarchini added a commit that referenced this pull request Apr 13, 2020
Signed-off-by: Matheus Marchini <[email protected]>

PR-URL: #32776
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
mmarchini added a commit that referenced this pull request Apr 13, 2020
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 <[email protected]>

PR-URL: #32776
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@mmarchini
Copy link
Contributor Author

Landed in 61e589f...81c03bc

@mmarchini mmarchini closed this Apr 13, 2020
BethGriggs pushed a commit that referenced this pull request Apr 14, 2020
Signed-off-by: Matheus Marchini <[email protected]>

PR-URL: #32776
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
BethGriggs pushed a commit that referenced this pull request Apr 14, 2020
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 <[email protected]>

PR-URL: #32776
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
Signed-off-by: Matheus Marchini <[email protected]>

PR-URL: nodejs#32776
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@addaleax
Copy link
Member

addaleax commented Apr 28, 2020

@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
Copy link
Member

@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?

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
Copy link
Member

@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
Copy link
Contributor Author

@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
Copy link
Member

@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
Copy link
Contributor Author

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 :)

BridgeAR pushed a commit that referenced this pull request Apr 28, 2020
Signed-off-by: Matheus Marchini <[email protected]>

PR-URL: #32776
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
BridgeAR pushed a commit that referenced this pull request Apr 28, 2020
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 <[email protected]>

PR-URL: #32776
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Apr 28, 2020
targos pushed a commit that referenced this pull request Apr 28, 2020
Signed-off-by: Matheus Marchini <[email protected]>

PR-URL: #32776
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
addaleax added a commit to addaleax/node that referenced this pull request Apr 28, 2020
Running tests with an ASAN build leads to increased memory usage,
rendering the memory usage assumptions in the test invalid.

Refs: nodejs#32776 (comment)
addaleax added a commit that referenced this pull request Apr 30, 2020
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this pull request May 4, 2020
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this pull request May 7, 2020
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this pull request May 13, 2020
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
addaleax added a commit to addaleax/node that referenced this pull request May 19, 2020
addaleax added a commit that referenced this pull request May 19, 2020
Refs: #32776

PR-URL: #33170
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
codebytere pushed a commit that referenced this pull request May 21, 2020
Refs: #32776

PR-URL: #33170
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants