Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2981233002:
Add .def file for the clang instrumented integration tests dll. (Closed)
DescriptionAdd .def file for the clang instrumented integration tests dll and a flag to the linker to generate the pdb file.
BUG=
Review-Url: https://codereview.chromium.org/2981233002
Committed: https://github.com/google/syzygy/commit/21968e40c08a1c95d00862422ff30dda237317e7
Patch Set 1 #
Total comments: 6
Patch Set 2 : Add def file and a flag to the linker to generate the pdb file. #
Total comments: 7
Patch Set 3 : Add def file and a flag to the linker to generate the pdb file. #Patch Set 4 : Add def file and a flag to the linker to generate the pdb file. #
Total comments: 2
Patch Set 5 : Add def file and a flag to the linker to generate the pdb file. #
Messages
Total messages: 30 (19 generated)
The CQ bit was checked by njanevsk@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add .def file for the clang instrumented integration tests dll. BUG= ========== to ========== Add .def file for the clang instrumented integration tests dll. BUG= ==========
njanevsk@google.com changed reviewers: + chrisha@chromium.org, sebmarchand@chromium.org
I added a .def file for the integration_tests_clang_dll.dll file. I inspected the generated file with dumpbin and it contains the EndToEndTest method.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2981233002/diff/1/syzygy/integration_tests/in... File syzygy/integration_tests/integration_tests_clang_dll.def (right): https://codereview.chromium.org/2981233002/diff/1/syzygy/integration_tests/in... syzygy/integration_tests/integration_tests_clang_dll.def:1: ; Copyright 2013 Google Inc. All Rights Reserved. 2017. https://codereview.chromium.org/2981233002/diff/1/syzygy/integration_tests/in... syzygy/integration_tests/integration_tests_clang_dll.def:15: ; Export declarations for the test DLL. Mention that this is the Clang version of this DLL. https://codereview.chromium.org/2981233002/diff/1/syzygy/integration_tests/ma... File syzygy/integration_tests/make_integration_tests_clang.py (right): https://codereview.chromium.org/2981233002/diff/1/syzygy/integration_tests/ma... syzygy/integration_tests/make_integration_tests_clang.py:89: '/def:' + def_file, You also want to produce a PDB, otherwise you won't have any debug info.
Description was changed from ========== Add .def file for the clang instrumented integration tests dll. BUG= ========== to ========== Add .def file for the clang instrumented integration tests dll and a flag to the linker to generate the pdb file. BUG= ==========
PTAL.
https://codereview.chromium.org/2981233002/diff/20001/syzygy/integration_test... File syzygy/integration_tests/integration_tests.gyp (right): https://codereview.chromium.org/2981233002/diff/20001/syzygy/integration_test... syzygy/integration_tests/integration_tests.gyp:55: 'integration_tests_dll', There should be a dependency on integration_tests_clang_dll https://codereview.chromium.org/2981233002/diff/20001/syzygy/integration_test... syzygy/integration_tests/integration_tests.gyp:125: 'outputs': ['<(PRODUCT_DIR)/integration_tests_clang_dll.dll'], Add the PDB to the list of outputs. https://codereview.chromium.org/2981233002/diff/20001/syzygy/integration_test... File syzygy/integration_tests/make_integration_tests_clang.py (right): https://codereview.chromium.org/2981233002/diff/20001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:89: '/debug:full', You also need to specify /Zi during the compile step for the debug information to be emitted in the obj files. There's no need to specify :full, "/DEBUG" is enough as full is the default value. https://codereview.chromium.org/2981233002/diff/20001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:96: print linker_command Remove the debugging code.
The CQ bit was checked by njanevsk@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL https://codereview.chromium.org/2981233002/diff/20001/syzygy/integration_test... File syzygy/integration_tests/integration_tests.gyp (right): https://codereview.chromium.org/2981233002/diff/20001/syzygy/integration_test... syzygy/integration_tests/integration_tests.gyp:55: 'integration_tests_dll', On 2017/07/19 15:27:26, Sébastien Marchand wrote: > There should be a dependency on integration_tests_clang_dll Done. https://codereview.chromium.org/2981233002/diff/20001/syzygy/integration_test... syzygy/integration_tests/integration_tests.gyp:125: 'outputs': ['<(PRODUCT_DIR)/integration_tests_clang_dll.dll'], On 2017/07/19 15:27:26, Sébastien Marchand wrote: > Add the PDB to the list of outputs. Done. https://codereview.chromium.org/2981233002/diff/20001/syzygy/integration_test... File syzygy/integration_tests/make_integration_tests_clang.py (right): https://codereview.chromium.org/2981233002/diff/20001/syzygy/integration_test... syzygy/integration_tests/make_integration_tests_clang.py:96: print linker_command On 2017/07/19 15:27:26, Sébastien Marchand wrote: > Remove the debugging code. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_dbg_try on master.tryserver.client.syzygy (JOB_FAILED, https://build.chromium.org/p/tryserver.client.syzygy/builders/win_dbg_try/bui...)
The CQ bit was checked by njanevsk@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
You didn't addressed the comment on patchset #1
I somehow didn't see the comments for the first patch set. Updated now. https://codereview.chromium.org/2981233002/diff/1/syzygy/integration_tests/in... File syzygy/integration_tests/integration_tests_clang_dll.def (right): https://codereview.chromium.org/2981233002/diff/1/syzygy/integration_tests/in... syzygy/integration_tests/integration_tests_clang_dll.def:1: ; Copyright 2013 Google Inc. All Rights Reserved. On 2017/07/19 13:57:29, Sébastien Marchand wrote: > 2017. Done. https://codereview.chromium.org/2981233002/diff/1/syzygy/integration_tests/in... syzygy/integration_tests/integration_tests_clang_dll.def:15: ; Export declarations for the test DLL. On 2017/07/19 13:57:29, Sébastien Marchand wrote: > Mention that this is the Clang version of this DLL. Done. https://codereview.chromium.org/2981233002/diff/1/syzygy/integration_tests/ma... File syzygy/integration_tests/make_integration_tests_clang.py (right): https://codereview.chromium.org/2981233002/diff/1/syzygy/integration_tests/ma... syzygy/integration_tests/make_integration_tests_clang.py:89: '/def:' + def_file, On 2017/07/19 13:57:29, Sébastien Marchand wrote: > You also want to produce a PDB, otherwise you won't have any debug info. Added /debug which produces a PDB and in the compile command /Zi
lgtm https://codereview.chromium.org/2981233002/diff/60001/syzygy/integration_test... File syzygy/integration_tests/integration_tests.gyp (right): https://codereview.chromium.org/2981233002/diff/60001/syzygy/integration_test... syzygy/integration_tests/integration_tests.gyp:56: 'integration_tests_clang_dll', Keep in alphabetical order.
Done! https://codereview.chromium.org/2981233002/diff/60001/syzygy/integration_test... File syzygy/integration_tests/integration_tests.gyp (right): https://codereview.chromium.org/2981233002/diff/60001/syzygy/integration_test... syzygy/integration_tests/integration_tests.gyp:56: 'integration_tests_clang_dll', On 2017/07/20 14:37:25, Sébastien Marchand wrote: > Keep in alphabetical order. Done.
The CQ bit was checked by njanevsk@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sebmarchand@chromium.org Link to the patchset: https://codereview.chromium.org/2981233002/#ps80001 (title: "Add def file and a flag to the linker to generate the pdb file.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1500561832663250,
"parent_rev": "190dbfe74c6f5b5913820fa66d9176877924d7c5", "commit_rev":
"21968e40c08a1c95d00862422ff30dda237317e7"}
Message was sent while issue was closed.
Description was changed from ========== Add .def file for the clang instrumented integration tests dll and a flag to the linker to generate the pdb file. BUG= ========== to ========== Add .def file for the clang instrumented integration tests dll and a flag to the linker to generate the pdb file. BUG= Review-Url: https://codereview.chromium.org/2981233002 Committed: https://github.com/google/syzygy/commit/21968e40c08a1c95d00862422ff30dda237317e7 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://github.com/google/syzygy/commit/21968e40c08a1c95d00862422ff30dda237317e7 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
