Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(276)

Issue 2986323003: Update the integration tests in preparation for Clang. (Closed)

Created:
3 years, 4 months ago by Sébastien Marchand
Modified:
3 years, 4 months ago
CC:
syzygy-changes_googlegroups.com, njanevsk
Target Ref:
refs/heads/master
Project:
syzygy
Visibility:
Public.

Description

Update the integration tests in preparation for Clang. There's 2 way to pass some options to the SyzyAsan runtime: - At instrumentation time by specifying them on the command line (e.g.: 'instrument.exe --mode=asan --asan-rtl-options="--foo --bar") - At runtime via an environment variable (SYZYGY_ASAN_OPTIONS) Only the environment variable approach works with Clang (yet, we might consider making it possible to specify these options at instrumentation time one day). Most of the tests are failing with the Clang instrumented version of the test DLL because they rely on the Asan options passed on the command line at instrumentation time, there's no particular reason for this except that it was cleaner than messing up with environment variables. As we want to support Clang we'll need to switch to using the env-var method. Review-Url: https://codereview.chromium.org/2986323003 Committed: https://github.com/google/syzygy/commit/22eea4e8748ce2ec1f1497691f0ada84390aed2d

Patch Set 1 #

Patch Set 2 : Fixes #

Patch Set 3 : nits #

Total comments: 2

Patch Set 4 : Address Siggi's comment. #

Total comments: 2

Patch Set 5 : Address Siggi's comment. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+61 lines, -38 lines) Patch
M syzygy/common/asan_parameters.h View 1 chunk +3 lines, -0 lines 0 comments Download
M syzygy/common/asan_parameters.cc View 1 chunk +1 line, -0 lines 0 comments Download
M syzygy/instrument/instrumenters/asan_instrumenter.cc View 1 chunk +3 lines, -3 lines 0 comments Download
M syzygy/instrument/instrumenters/asan_instrumenter_unittest.cc View 1 2 chunks +4 lines, -2 lines 0 comments Download
M syzygy/integration_tests/instrument_integration_test.cc View 1 2 3 4 17 chunks +50 lines, -33 lines 0 comments Download

Messages

Total messages: 19 (12 generated)
Sébastien Marchand
Hey Siggi, do you mind taking a look at this while Chris' away? I'd like ...
3 years, 4 months ago (2017-08-03 15:16:54 UTC) #9
Sigurður Ásgeirsson
looks nice, except for that nit. https://codereview.chromium.org/2986323003/diff/30001/syzygy/integration_tests/instrument_integration_test.cc File syzygy/integration_tests/instrument_integration_test.cc (right): https://codereview.chromium.org/2986323003/diff/30001/syzygy/integration_tests/instrument_integration_test.cc#newcode419 syzygy/integration_tests/instrument_integration_test.cc:419: void SetAsanOptions(const char* ...
3 years, 4 months ago (2017-08-03 15:27:36 UTC) #10
Sébastien Marchand
Thanks, PTAnL. https://codereview.chromium.org/2986323003/diff/30001/syzygy/integration_tests/instrument_integration_test.cc File syzygy/integration_tests/instrument_integration_test.cc (right): https://codereview.chromium.org/2986323003/diff/30001/syzygy/integration_tests/instrument_integration_test.cc#newcode419 syzygy/integration_tests/instrument_integration_test.cc:419: void SetAsanOptions(const char* value) { On 2017/08/03 ...
3 years, 4 months ago (2017-08-03 15:40:27 UTC) #11
Sigurður Ásgeirsson
lgtm https://codereview.chromium.org/2986323003/diff/50001/syzygy/integration_tests/instrument_integration_test.cc File syzygy/integration_tests/instrument_integration_test.cc (right): https://codereview.chromium.org/2986323003/diff/50001/syzygy/integration_tests/instrument_integration_test.cc#newcode426 syzygy/integration_tests/instrument_integration_test.cc:426: void SetAsanOptions(const char* value) { nit: this name ...
3 years, 4 months ago (2017-08-03 17:53:27 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2986323003/70001
3 years, 4 months ago (2017-08-03 18:26:32 UTC) #15
Sébastien Marchand
https://codereview.chromium.org/2986323003/diff/50001/syzygy/integration_tests/instrument_integration_test.cc File syzygy/integration_tests/instrument_integration_test.cc (right): https://codereview.chromium.org/2986323003/diff/50001/syzygy/integration_tests/instrument_integration_test.cc#newcode426 syzygy/integration_tests/instrument_integration_test.cc:426: void SetAsanOptions(const char* value) { On 2017/08/03 17:53:26, Sigurður ...
3 years, 4 months ago (2017-08-03 18:27:32 UTC) #16
commit-bot: I haz the power
3 years, 4 months ago (2017-08-03 19:09:00 UTC) #19
Message was sent while issue was closed.
Committed patchset #5 (id:70001) as
https://github.com/google/syzygy/commit/22eea4e8748ce2ec1f1497691f0ada84390aed2d

Powered by Google App Engine
This is Rietveld 408576698