Is it chromium or clang/llvm that changed...

Last passing:  https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/ToTLinuxASanLibfuzzer/1667
First failing: https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/ToTLinuxASanLibfuzzer/1668

I can reproduce the failure, but it also fails for me with the revisions used in the successful build :-(

Aha! The build config changed:

---
Reland "Switch all libFuzzer and AFL builds (except Windows) to is_component_build=true."

This is a reland of 446c61f501b4b971c63ba40e1667e5b0d8ca76c8

The reason for the previous revert should be fixed now by https://crrev.com/c/1362457

Original change's description:
> Switch all libFuzzer and AFL builds (except Windows) to is_component_build=true.
>
> Bug: 701825
> Change-Id: I4b361daa1eae576f402818b467684efa5380df88
> Reviewed-on: https://chromium-review.googlesource.com/c/1361297
> Commit-Queue: Max Moroz <mmoroz@chromium.org>
> Reviewed-by: Jonathan Metzman <metzman@chromium.org>
> Reviewed-by: Ben Pastene <bpastene@chromium.org>
> Reviewed-by: Abhishek Arya <inferno@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#613700}

Bug: 701825
Change-Id: I6f6336f7203600a62f760866a54ad03281dd6c13
Reviewed-on: https://chromium-review.googlesource.com/c/1365084
Reviewed-by: Abhishek Arya <inferno@chromium.org>
Reviewed-by: John Budorick <jbudorick@chromium.org>
Commit-Queue: Max Moroz <mmoroz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614439}
---

Looks like a similar problem was addressed here: https://chromium-review.googlesource.com/c/1366323

Fixing with https://chromium-review.googlesource.com/c/chromium/src/+/1367652

Hans, do you have any idea why our buildbot [1] as well as the CQ bot [2] do not fail after that change, and only ToT bot does?

1: https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Libfuzzer%20Upload%20Linux%20ASan

2: https://ci.chromium.org/p/chromium/builders/luci.chromium.try/linux-libfuzzer-asan-rel

I'm just worried that revert to non-component build won't solve the issue here, I'd rather suspect some clang regression.

I see that you ended up reverting only ToT bot, thanks, that works for us. Should we set a reminder to switch it back to the component build some time later?

> I'm just worried that revert to non-component build won't solve the issue here, I'd rather suspect some clang regression.

I don't think it's a clang regression.



> Hans, do you have any idea why our buildbot [1] as well as the CQ bot [2] do not fail after that change, and only ToT bot does?
>
> 1: https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Libfuzzer%20Upload%20Linux%20ASan

Looking at https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Libfuzzer%20Upload%20Linux%20ASan/30129
It doesn't build libcrashpad_snapshot_test_both_dt_hash_styles.so which is the target that failed here. I don't know if anything is special about it.


> 2: https://ci.chromium.org/p/chromium/builders/luci.chromium.try/linux-libfuzzer-asan-rel

Looking at https://ci.chromium.org/p/chromium/builders/luci.chromium.try/linux-libfuzzer-asan-rel/59834
The gn flags contain is_component_build = false

The ToT bots build all targets, while most other bots don't.

Before switching component buildiness, I recommend doing a full build in that config locally.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/81ac59ee7223e11dbe230ae78c9b1c9c441ed78c

commit 81ac59ee7223e11dbe230ae78c9b1c9c441ed78c
Author: Hans Wennborg <hans@chromium.org>
Date: Fri Dec 07 15:26:45 2018

Switch ToTLinuxASanLibfuzzer back to non-component builds

It currently fails to link, see first bug.

TBR=mmoroz

Bug: 912919, 701825
Change-Id: I9491ebd1adf970173a4f2d009ca80254e7c6c7df
Reviewed-on: https://chromium-review.googlesource.com/c/1367652
Reviewed-by: Hans Wennborg <hans@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Hans Wennborg <hans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614701}
[modify] https://crrev.com/81ac59ee7223e11dbe230ae78c9b1c9c441ed78c/tools/mb/mb_config.pyl

Why does ToT libFuzzer ASan bot builds "all targets", when it's supposed to build only fuzz targets?

I'd rather call it a luck that it was successfully building unrelated targets using build config intended for building fuzzers only.

I think "ninja all" should work in all build configs.

If some targets are not meant to be built in some configuration, they shouldn't be part of "all".

+1. Just building 'all' (which is the default target if you don't pass any explicit targets) is supposed to work everywhere, and does work everywhere except currently this bot. Like Hans says, if some targets shouldn't build in your build config, they shouldn't be part of all in that config.

Thanks for clarifying. Is this documented anywhere? How can I change the "all" config?

'all' is all targets reachable from src/BUILD.gn. You can e.g. change it by adding deps only if !is_fuzzer_build.

Adding a visibility attribute would fix the failure mentioned in c#0:

--- a/third_party/crashpad/crashpad/snapshot/hash_types_test.cc
+++ b/third_party/crashpad/crashpad/snapshot/hash_types_test.cc
@@ -12,6 +12,6 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-int main() {
+int __attribute__((visibility("default"))) main() {
   return 0;
 }



but there are many more others across blink, V8, WebRTC, etc. I'm trying to eliminate those one by one as part of issue 911856, but I've no idea how many more there are and when I'll finish it.

Can you expand on why adding the visibility attribute helps? Having to explicitly set default visibility on main seems odd. Maybe it suggests we should change something in the tools so that's not necessary?

Hans, my understanding is the following:

1) that file is compiled into a dynamic library (GN's loadable_module type)
2) the function (despite having name main()) is neither called explicitly nor exported
3) we compile with -fvisibility=hidden

As a result, the function gets stripped out (or something like that) by clang?

Why do we compile something with a main() into a shared_library instead of into an executable?

Haha, I had the same thought, but no clue :)

Looking at the use https://cs.chromium.org/chromium/src/third_party/crashpad/crashpad/snapshot/elf/elf_image_reader_test.cc?type=cs&q=crashpad_snapshot_test_both_dt_hash_styles&sq=package:chromium&g=0&l=300 and the gn file https://cs.chromium.org/chromium/src/third_party/crashpad/crashpad/snapshot/BUILD.gn?type=cs&q=hash_types_test&sq=package:chromium&g=0&l=490 this just needs any exported symbol. So calling that main() is probably incorrect, and putting an export macro (visibility("default"), essentially) is the right fix. Not having any function might be fine too, since that's what essentially is happening at the moment.


But why is this only broken in libfuzzer builds? I guess the fuzzer builds makes some assumption that's broken here? Which assumption is that? Maybe we could fix that?

I think "-fsanitize=fuzzer-no-link " is the key here, because the error in c#0 complains about sancov symbols.

However, the problem is much bigger. I've tried switching UBSan config (which uses -fsanitize=vptr) to use component build, and it revealed a ton of issues with visibility :(

If you're curious, the combination of the following CLs allowed me to do `ninja all` in that new UBSan config, but there were multiple issues reported during runtime.

1) https://chromium-review.googlesource.com/c/chromium/src/+/1372267
2) https://chromium-review.googlesource.com/c/v8/v8/+/1372072
3) https://webrtc-review.googlesource.com/c/src/+/114041 -- got -1 because it's not that easy
4) https://chromium-review.googlesource.com/c/chromium/src/+/1375637 -- this is just a hacky local version that doesn't necessary use proper macros

_Why_ does -fsanitize=fuzzer-no-link require different visibility though?

If the fuzzer configs require differently-looking c++ files than regular component builds, they'll break a lot and slow everyone down. Ideally we can fix the root cause somehow instead of requiring a ton of chrome-side, v8-side, webrtc-side changes.

Just to clarify, I believe all the changes in c#23 were caused by `vptr` flag, because the errors were like "undefined symbol: typeinfo for %CLASSNAME%".

The error from c#0 is the only one that was happening even without "vptr" flag.

I can't say whether sanitizers are doing a wrong thing or not, but it looks like some parts of Chromium code do not properly specify the dependencies / exported symbols, and that's why these issues come up once we enable "vptr". WebRTC developers seem to realize it, and that's why switching to component build is not that easy: https://webrtc-review.googlesource.com/c/src/+/114041/2#message-3ec29fbe32d55fe7533232e8a67b4e4d1dcb8499

Ah ok, vptr requires -frtti which nothing else requires. We probably don't want the CFI bots do component builds then I imagine, since the build config is so different.

Comment 17 sounded like ToTLinuxASanLibfuzzer had many more issues. If the crashpad one is the only one for ToTLinuxASanLibfuzzer , we should probably just change that one file and switch the tot bot back to component builds.

I've just tried `ninja all` with ToTLinuxASanLibfuzzer + is_component_build, and there was one more problem:

$ autoninja -C out/lfasan_comp/
/usr/local/google/home/mmoroz/Projects/depot_tools/ninja -C out/lfasan_comp/ -j 1120
ninja: Entering directory `out/lfasan_comp/'
[47407/47988] ACTION //tools/v8_context_snapshot:generate_v8_context_snapshot(//build/toolchain/linux:clang_x64)
FAILED: v8_context_snapshot.bin 
python ../../build/gn_run_binary.py ./v8_context_snapshot_generator --output_file=v8_context_snapshot.bin
=================================================================
==44133==ERROR: AddressSanitizer: odr-violation (0x55d924735760):
  [1] size=8 'webrtc::EncodedImage::kBufferPaddingBytesH264' ../../third_party/webrtc/api/video/encoded_image.cc:19:28
  [2] size=8 'webrtc::EncodedImage::kBufferPaddingBytesH264' ../../third_party/webrtc/api/video/encoded_image.cc:19:28
These globals were registered at these points:
  [1]:
    #0 0x55d9247ff7ae in __asan_register_globals /b/swarming/w/ir/kitchen-workdir/src/third_party/llvm/compiler-rt/lib/asan/asan_globals.cc:359:3
    #1 0x7ff99f7a193b in asan.module_ctor (/usr/local/google/home/mmoroz/Projects/new/chromium/src/out/lfasan_comp/./libmojo_base_mojom_blink.so+0x2de93b)

  [2]:
    #0 0x55d9247ff7ae in __asan_register_globals /b/swarming/w/ir/kitchen-workdir/src/third_party/llvm/compiler-rt/lib/asan/asan_globals.cc:359:3
    #1 0x7ff9a974b57b in asan.module_ctor (/usr/local/google/home/mmoroz/Projects/new/chromium/src/out/lfasan_comp/./libnetwork_cpp_base.so+0x9d457b)

==44133==HINT: if you don't care about these errors you may set ASAN_OPTIONS=detect_odr_violation=0
SUMMARY: AddressSanitizer: odr-violation: global 'webrtc::EncodedImage::kBufferPaddingBytesH264' at ../../third_party/webrtc/api/video/encoded_image.cc:19:28
==44133==ABORTING
./v8_context_snapshot_generator failed with exit code 1
[47409/47988] CXX obj/v8/test/cctest/cctest_sources/test-api.o
ninja: build stopped: subcommand failed.


Which could be overcome by using ASAN_OPTIONS=detect_odr_violation=0 (that's what we also use when running fuzzers), but I'm not sure whether GN's action allows us to control env variables?

can you change https://cs.chromium.org/chromium/src/build/sanitizers/sanitizer_options.cc ? that should change the defaults.

Or we could fix the bugs? Changing the defaults seems like that would prevent the chrome memory waterfall from finding such odr violations.

We try to disable sanitizers for non-default_toolchain builds (https://cs.chromium.org/chromium/src/build/config/sanitizers/sanitizers.gni?q=fsanitize+file:%5C.gn&sq=package:chromium&dr=C&l=111); host tools shouldn't get it.

I suppose on that config host and target toolchain are the same though, hmm.

But isn't that just regular asan? Why would that be a problem for a fuzzer build but not a regular asan build?

Hm, do we have regular asan bots doing component builds?

I think webrtc having ODR violations in component builds is a somewhat longstanding issue that mbonedai was working on. I don't know what the current status of that is.

I'm more concerned about the fuzzer bots doing asan+components while the regular *san bots don't do component builds. We should probably use the same type of build in both setups? Else our test matrix gains yet another entry.

I'm guessing the argument for the sanitizer bots not doing component bots is "that's closer to what we ship" (?)

Correct, in Q3 I worked on it and I completed a local build with WebRTC honoring the component_build flag (Chromium will depend on a WebRTC component and not on WebRTC's static_libraries).
Before finishing, I hit some problems with pnacl and I haven't been able to really work on it this quarter but it will be fixed early January.

Thanks for the update, Mirko! I think early January sounds great.

> I'm guessing the argument for the sanitizer bots not doing component bots is "that's closer to what we ship" (?)

I don't know, to be frank. For fuzzers, we've started with static builds hoping to have monolithic binaries that can be easily distributed and used for fuzzing / reproducing the crashes. Regular sanitizer builds might've had the same goal in mind, but I'm not 100% sure. Your assumption sounds totally reasonable as well.

(no longer blocking the roll since we undid the bot config change i think)

Re #34: While working on the WebRTC component build I hit another problem (crbug.com/919085). It turns out that //third_party/libjingle_xmpp uses WebRTC's GN templates (+ some dependencies on internal stuff) so when I tried to make WebRTC targets only visibile to the Chromium WebRTC component I ended up breaking libjingle_xmpp. :(