hwennborg@, could you PTAL as the next week's go/chrome-clang-sheriff?

Yeah, that's why it's not so great when the bot is read for a long time; now it's a lot more painful to find the first build where this error appeared :-( First bad: https://luci-milo.appspot.com/buildbot/chromium.clang/ToTLinux/3684

(Hans is out today, and he's on European time so even if he was around he'd be out already)

clang regression range 342641:342644 ... no libc++ changes or clang changes in that range.

That build did include a webrtc roll: https://chromium-review.googlesource.com/c/chromium/src/+/1235837

Only contains this change though https://webrtc.googlesource.com/src.git/+/207cfdfbd8896e093f7088123eb729df174614d3%5E%21/#F0


Huh!

First build here https://ci.chromium.org/buildbot/chromium.clang/ToTLinux%20%28dbg%29/4591 same clang range, and only 6 chromium side commits (again including the webrtc roll)

https://ci.chromium.org/buildbot/chromium.clang/ToTAndroid/4624 also includes the webrtcroll

artit, would you mind reverting https://webrtc-review.googlesource.com/c/src/+/101041 upstream and rolling that in to see if it helps?

(that change is in a `is_ios || is_mac` conditional so I have no idea how it would affect linux, but I don't see anything better yet)

s/hwennborg@google/hans@chromium/

I'm stumped here. Nothing in clang, libc++ (which we pin on the linux and android bots anyhow), or chromium seems to have changed.

Phew, what is the error even? It complains about this code: https://cs.chromium.org/chromium/src/third_party/webrtc/pc/rtcstatscollector.cc?sq=package:chromium&g=0&l=1112

  std::map<std::string, std::vector<std::string>> track_ids;

  for (const auto& stats : transceiver_stats_infos_) {
    for (auto sender : stats.transceiver->senders()) {
      std::string track_id =
          RTCMediaStreamTrackStatsIDFromDirectionAndAttachment(
              kSender, sender->internal()->AttachmentId());
      for (auto& stream_id : sender->stream_ids()) {
        track_ids[stream_id].push_back(track_id);  // <-- here
      }
    }

track_ids[stream_id] resolves to a std::vector<std::string>, which we then push_back a string track_id to. sender->stream_ids() is a std::vector<std::string>, so stream_id is a std::string.

This error is very suspicious:

../../buildtools/third_party/libc++/trunk/include/tuple:375:9: note: candidate template ignored: substitution failure [with _Uf = <0>, _Tf = <const std::__1::basic_string<char> &>, _Ul = <>, _Tl = <>, _Up = <const std::__1::basic_string<char> &>]: pack expansion contains parameter pack '_Uf' that has a different length (3941636144 vs. 1) from outer parameter packs

where 3941636144 = 0xeaf09830 is unlikely to be a proper length!
It suggests a compiler issue, but I don't see yet what change could have triggered it.

> Nothing in clang, libc++ (which we pin on the linux and android bots anyhow), or chromium seems to have changed.

I'm confused here. These builds use 'llvm_force_head_revision = true'. Does't that mean clang/libc++ aren't pinned?

Also, this error appeared anytime between:
https://luci-milo.appspot.com/buildbot/chromium.clang/ToTLinux/3675 2018-09-20 6:02 AM (CEST)
https://luci-milo.appspot.com/buildbot/chromium.clang/ToTLinux/3684 2018-09-20 3:32 PM (CEST)

Builds from 3675 to 3683 show another compile error, which could mask the current one.

The dbg builder doesn't really give a better range.

 

For record, this build: https://luci-milo.appspot.com/buildbot/chromium.clang/ToTLinux/3687

shows the exact same error from entirely different lib:
mojo/public/cpp/bindings/tests/sync_method_unittest.cc

> I'm confused here. These builds use 'llvm_force_head_revision = true'. Does't that mean clang/libc++ aren't pinned?


Yes, the ToT bots build with trunk clang, so that we can check when it's safe to roll in a new clang.

Pinned clang doesn't have this issue, but we need to fix this before we can roll clang again.

Re c#6:

> artit, would you mind reverting https://webrtc-review.googlesource.com/c/src/+/101041 upstream and rolling that in to see if it helps?

Revert is done, waiting for new roll into chromium repo.

There is something strange going on here. I've tried hard to reproduce this locally, without any luck so far.

Is clang itself getting miscompiled somehow on the bot, or there something in the environment tickling this problem...

It's not just ToTLinux, it's all linux tot bots: https://ci.chromium.org/p/chromium/g/chromium.clang/console

So it's likely not bot-specific. The tot bots don't do bootstrap builds for perf reasons; maybe it's caused by the gcc host compiler we use there?

Pulling the clang binary off one of the tot linux bots, I can now reproduce locally. Just the binary was enough, no need for the bot's libstdc++ or anything like that.

I'm currently trying to get a smaller repro.

Currently running creduce with the gcc from the bot, but in the meantime, I was able to reproduce this with a locally built Clang:

1) Build gcc 4.8.4 locally

$ git checkout gcc-4_8_4-release
$ mkdir build && cd build
$ ../configure --prefix=/work/gccprefix --enable-languages=c,c++ --enable-checking=release --disable-bootstrap
(apply local hacks to make it build; attached)
$ make -j32
$ make install

2) Use that to build clang

$ CC=/work/gccprefix/bin/gcc CXX=/work/gccprefix/bin/g++ cmake -GNinja -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=ON ../llvm
$ ninja clang

3) Repro with the attached preprocessed source

$ /work/llvm.combined/build.gcc/bin/clang -cc1 -triple x86_64-unknown-linux-gnu -fsyntax-only -std=c++14 -x c++ /tmp/a.ii

Deleted: make_gcc_build.patch 1.1 KB

make_gcc_build.patch 1.1 KB Download

Deleted: a.ii 3.4 MB

a.ii 3.4 MB Download

Bisection over Clang points to this:

---
Author: d0k
Date: Thu Sep 20 05:21:24 2018
New Revision: 342643

URL: http://llvm.org/viewvc/llvm-project?rev=342643&view=rev
Log:
[ADT] Bring back memmove to make GCC 5.4 happy

All other GCCs look good so far. GCC 5.4 complains about strict
aliasing, so fix that.

Modified:
    llvm/trunk/include/llvm/ADT/Optional.h
---

Both "make GCC happy", "use memmove", and "strict aliasing" make it sound like it could be interesting :-)

The diagnostic is emitted from here:

    if (NewPackSize != *NumExpansions) {
      // C++0x [temp.variadic]p5:
      //   All of the parameter packs expanded by a pack expansion shall have
      //   the same number of arguments specified.
      if (HaveFirstPack)                         
        Diag(EllipsisLoc, diag::err_pack_expansion_length_conflict)
          << FirstPack.first << Name << *NumExpansions << NewPackSize
          << SourceRange(FirstPack.second) << SourceRange(i->second);                             
      else
        Diag(EllipsisLoc, diag::err_pack_expansion_length_conflict_multilevel)      <------------------                    
          << Name << *NumExpansions << NewPackSize                                                
          << SourceRange(i->second);
      return true;
    }

NumExpansions is a reference to an Optional<unsigned>.

d0k's follow-up commit in Clang r342723 suggests some GCC's miscompile Optional<enum>. Perhaps it applies to Optional<unsigned> too?


I'm preparing a revert of all the Optional changes from Friday.

Revert is in r342966.