Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2155713002:
Fix net/ fuzzers. (Closed)
DescriptionFix net/ fuzzers.
They were broken in https://codereview.chromium.org/2151013002. They
depend on a global that's never directly used being linked in, which
that CL broke.
BUG=628503
Committed: https://crrev.com/35a30017c0177c14a272e4517a9e83d7c41ebea1
Cr-Commit-Position: refs/heads/master@{#405813}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add comment #Messages
Total messages: 25 (12 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by mmenke@chromium.org to run a CQ dry run
mmenke@chromium.org changed reviewers: + brettw@chromium.org, eroman@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm, although I don't quite understand the difference between the two.
On 2016/07/15 16:56:36, eroman wrote: > lgtm, although I don't quite understand the difference between the two. The broken code makes an .so file, I believe, while the functional code links in the CC files directly. For some reason, when making an so file, InitGlobals is not run from the *.so file (https://cs.chromium.org/chromium/src/net/base/fuzzer_test_support.cc?q=initgl...)
https://codereview.chromium.org/2155713002/diff/20001/net/BUILD.gn File net/BUILD.gn (right): https://codereview.chromium.org/2155713002/diff/20001/net/BUILD.gn#newcode1752 net/BUILD.gn:1752: source_set("net_fuzzer_test_support") { You should comment why this needs to be the case or I will likely change it back in my next pass!
https://codereview.chromium.org/2155713002/diff/20001/net/BUILD.gn File net/BUILD.gn (right): https://codereview.chromium.org/2155713002/diff/20001/net/BUILD.gn#newcode1752 net/BUILD.gn:1752: source_set("net_fuzzer_test_support") { On 2016/07/15 17:01:38, brettw (ping after 24h) wrote: > You should comment why this needs to be the case or I will likely change it back > in my next pass! Is something like "Making this a static_library breaks initialization of globals in fuzzer_test_support.cc, so this must be a source_set" sufficient? I'm not good enough with compiler/linker-foo to give a more detailed comment.
I'd write something a little more like the CL description: "This has a global <maybe name it?> that must always be linked in, so must be a source set instead of a static library." LGTM with something along those lines.
On 2016/07/15 17:18:53, brettw (ping after 24h) wrote: > I'd write something a little more like the CL description: > > "This has a global <maybe name it?> that must always be linked in, so must be a > source set instead of a static library." > > LGTM with something along those lines. Done, thanks!
Description was changed from ========== Fix net/ fuzzers. They were broken in https://codereview.chromium.org/2151013002. They depend on a global that's never used being initialized, which that CL seems to have broken. BUG=628503 ========== to ========== Fix net/ fuzzers. They were broken in https://codereview.chromium.org/2151013002. They depend on a global that's never directly used being initialized, which that CL seems to have broken. BUG=628503 ==========
Description was changed from ========== Fix net/ fuzzers. They were broken in https://codereview.chromium.org/2151013002. They depend on a global that's never directly used being initialized, which that CL seems to have broken. BUG=628503 ========== to ========== Fix net/ fuzzers. They were broken in https://codereview.chromium.org/2151013002. They depend on a global that's never directly used being linked in, which that CL broke. BUG=628503 ==========
The CQ bit was checked by mmenke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org, eroman@chromium.org Link to the patchset: https://codereview.chromium.org/2155713002/#ps40001 (title: "Add comment")
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
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by mmenke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix net/ fuzzers. They were broken in https://codereview.chromium.org/2151013002. They depend on a global that's never directly used being linked in, which that CL broke. BUG=628503 ========== to ========== Fix net/ fuzzers. They were broken in https://codereview.chromium.org/2151013002. They depend on a global that's never directly used being linked in, which that CL broke. BUG=628503 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Fix net/ fuzzers. They were broken in https://codereview.chromium.org/2151013002. They depend on a global that's never directly used being linked in, which that CL broke. BUG=628503 ========== to ========== Fix net/ fuzzers. They were broken in https://codereview.chromium.org/2151013002. They depend on a global that's never directly used being linked in, which that CL broke. BUG=628503 Committed: https://crrev.com/35a30017c0177c14a272e4517a9e83d7c41ebea1 Cr-Commit-Position: refs/heads/master@{#405813} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/35a30017c0177c14a272e4517a9e83d7c41ebea1 Cr-Commit-Position: refs/heads/master@{#405813} |
