Work around VC++ /analyze internal compiler error

Avoid a /analyze internal compiler error with MakeIndexSequence

The recently added MakeIndexSequence templates cause a VC++ internal
compiler error when building with /analyze. This happens on dozens of
source files so disabling /analyze for those files is impractical. This
change conditionally replaces the template recursion with a
straightforward implementation for values from zero to five. This is
sufficient to build Chrome at the moment.

The code which VC++ doesn't like was introduced in change:
https://codereview.chromium.org/693693005

It's an ugly fix, but justified by the desire to allow running
/analyze. The /analyze version of the code will never be run.

A VC++ bug has been filed:

https://connect.microsoft.com/VisualStudio/feedback/details/1053626

BUG=427616
Committed: https://crrev.com/b132ccda867e4fc8479450db3baf96f1b713b827
Cr-Commit-Position: refs/heads/master@{#312469}

[brucedawson, 2014-12-15 21:44:14]

brucedawson@chromium.org changed reviewers:
+ mdempsky@chromium.org

[Nico, 2014-12-15 22:14:40]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2014-12-15 22:14:40]

Urgh, this is horrible. Is _PREFAST_ only enabled in /analyze mode? How
important is it that we keep /analyze working?

If we want analyze and it causes issues like this, we should ban variadic
templates. Having to add workarounds like this is horrible. Variadic templates
so far have allowed us to remove thousands of lines of code, so I'd prefer to
not ban them. (We want to change bind.h to depend on this tuple, and I want to
change ipc/ to use the new way of spelling them instead of TupleN, so this
workaround likely won't stay enough.)

[mdempsky, 2014-12-15 22:17:55]

https://codereview.chromium.org/803183004/diff/20001/base/tuple.h
File base/tuple.h (right):

https://codereview.chromium.org/803183004/diff/20001/base/tuple.h#newcode101
base/tuple.h:101: struct MakeIndexSequenceImpl<N, Ns...>
I previously wrote this as:

    template <size_t N, size_t... Ns>
    struct MakeIndexSequenceImpl<N, Ns...> {
      using Type = typename MakeIndexSequenceImpl<N - 1, N - 1, Ns...>::Type;
    };

but later changed it to use inheritance just to be a bit more concise.  Did the
old form have problems with /analyze too?  If not, we could just revert to that
maybe?

[brucedawson, 2014-12-15 22:29:16]

My previous replies got lost -- trying again:

_PREFAST_ is only enabled in /analyze mode. /analyze has found 35 bugs that
we've decided to fix (and some more that are on my list to fix) so I don't want
to abandon it too easily.

I don't know what it is about this particular code sequence that made /analyze
fail. The entire code base was building for a month or two (well, after working
around three other unrelated internal compiler errors).



Unfortunately the non-inheritance version fails in the same way.

BTW, don't think of accepting this change as permanently committing to changing
the code to support /analyze forever. It's more like changing the code to allow
the /analyze experiment to continue. It's not clear yet how high the ongoing
value is, and these compiler errors make it impractical to continue
experimenting.

[mdempsky, 2014-12-15 22:38:18]

On 2014/12/15 22:29:16, brucedawson wrote:
> Unfortunately the non-inheritance version fails in the same way.

Darn.

The change seems technically fine by me (for what it's worth), but I'll defer to
base/OWNERS to decide whether this looks good to commit.

[Nico, 2014-12-15 23:33:51]

On 2014/12/15 22:29:16, brucedawson wrote:
> My previous replies got lost -- trying again:
> 
> _PREFAST_ is only enabled in /analyze mode. /analyze has found 35 bugs that
> we've decided to fix (and some more that are on my list to fix) so I don't
want
> to abandon it too easily.
> 
> BTW, don't think of accepting this change as permanently committing to
changing
> the code to support /analyze forever. It's more like changing the code to
allow
> the /analyze experiment to continue. It's not clear yet how high the ongoing
> value is, and these compiler errors make it impractical to continue
> experimenting.

There's no /analyze bot at the moment, it's just you running it locally, right?
Can you carry this patch locally? This file shouldn't change all that often, so
this probably shouldn't be a ton of ongoing work.

We're having a pretty hard time with all our toolchains already, so personally I
think it's more useful to improve our existing toolchains to do most of the
things /analyze does instead of depending on another msvc thing that we can't
control.

(I'm happy to lgtm this specific change if you think it helps a lot, but I'd
expect that there will be further ripple effects down the road, and I don't want
to end up in a state where we end up with #ifdef _PREFAST_ follow-up CLs after
every other CL that uses Tuple in more places – like for example the Bind()
stuff tzik wants to do.)

[brucedawson, 2014-12-15 23:50:03]

I'll hold off for now. To avoid polluting my existing repos it means that I have
to have a separate repo on my machine just for /analyze, but that's probably a
good idea anyway. I do want to get a /analyze bot running eventually so when I
do I may revisit this request.

I definitely agree with the benefit of getting the analysis into clang, but that
needs as much history of typical /analyze output as possible.

BTW, since you typed the four magic letters you technically *did* approve the
change :-)

[Nico, 2014-12-15 23:52:16]

On 2014/12/15 23:50:03, brucedawson wrote:
> I'll hold off for now. To avoid polluting my existing repos it means that I
have
> to have a separate repo on my machine just for /analyze, but that's probably a
> good idea anyway. I do want to get a /analyze bot running eventually so when I
> do I may revisit this request.
> 
> I definitely agree with the benefit of getting the analysis into clang, but
that
> needs as much history of typical /analyze output as possible.
> 
> BTW, since you typed the four magic letters you technically *did* approve the
> change :-)

Yes, I'm aware :-)

[brucedawson, 2014-12-16 18:25:07]

When building all of Chromium (no target specified instead of 'chrome' as the
target) the workaround requires explicit implementations of
MakeIndexSequenceImpl all the way up to 7 -- two higher than previously
required. The size bloat could be constrained somewhat with different formatting
but it's still an annoyingly large blob. I have not updated the patch set to
include this.

I'll update this bug over the coming weeks with any new variants of this problem
that I hit.

[brucedawson, 2015-01-21 00:25:44]

I'm just about ready to fire up a /analyze fyi bot and that will require having
some variation of this patch landed. I was able to simplify the patch
significantly (getting rid of some types, and tightening up what remains).

It's also notable that in the last five weeks or so there have been no new
internal compiler errors, from variadic templates or from other sources. Even
the newly added variadic templates in pdfium caused no problems for /analyze.

What do you think?

[Nico, 2015-01-21 00:38:39]

On 2015/01/21 00:25:44, brucedawson wrote:
> I'm just about ready to fire up a /analyze fyi bot and that will require
having
> some variation of this patch landed. I was able to simplify the patch
> significantly (getting rid of some types, and tightening up what remains).
> 
> It's also notable that in the last five weeks or so there have been no new
> internal compiler errors, from variadic templates or from other sources. Even
> the newly added variadic templates in pdfium caused no problems for /analyze.
> 
> What do you think?

* Can you patch in https://codereview.chromium.org/743853002/ and check that
things are still happy then?

* How well does analyze handle more than 7 parameters? People complain about
Bind() not supporting enough parameters from time to time
(http://crbug.com/449236) so it's kind of nice to not be limited to 7 due to
this. (Then again, if you need that many parameters, chances are you're holding
it wrong)

If things still work with https://codereview.chromium.org/743853002/ patched in,
i'm happy with landing this change here.

[brucedawson, 2015-01-21 00:50:20]

I'll do a run tonight with https://codereview.chromium.org/743853002/.

More than 7 parameters can be handled as long as I add more explicit types.
Adding a couple more prior to checking in would give us some breathing room. I
could go to ten, or eleven, pretty easily. I stopped at seven because that is
how many Chrome currently needs, but it sounds like more will soon be wanted.
Any preferences for how high/crazy to go?

[Nico, 2015-01-21 00:52:06]

On 2015/01/21 00:50:20, brucedawson wrote:
> I'll do a run tonight with https://codereview.chromium.org/743853002/.
> 
> More than 7 parameters can be handled as long as I add more explicit types.
> Adding a couple more prior to checking in would give us some breathing room. I
> could go to ten, or eleven, pretty easily. I stopped at seven because that is
> how many Chrome currently needs, but it sounds like more will soon be wanted.
> Any preferences for how high/crazy to go?

This tuple should go to eleven.

[brucedawson, 2015-01-21 01:05:56]

> This tuple should go to eleven.

Not sure if serious... but why not?

[brucedawson, 2015-01-21 18:23:28]

The run with https://codereview.chromium.org/743853002/ worked with no errors or
new warnings, and my change now goes to eleven. Any remaining concerns?

[Nico, 2015-01-21 19:27:06]

On 2015/01/21 18:23:28, brucedawson wrote:
> The run with https://codereview.chromium.org/743853002/ worked with no errors
or
> new warnings, and my change now goes to eleven. Any remaining concerns?

Lgtm :-/

[brucedawson, 2015-01-21 19:38:39]

The CQ bit was checked by brucedawson@chromium.org

[commit-bot: I haz the power, 2015-01-21 19:40:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/803183004/100001

[commit-bot: I haz the power, 2015-01-21 22:02:42]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-01-21 22:03:48]

Patchset 6 (id:??) landed as
https://crrev.com/b132ccda867e4fc8479450db3baf96f1b713b827
Cr-Commit-Position: refs/heads/master@{#312469}

[Nico, 2016-11-15 19:17:00]

Is this still needed with vs2015?

[brucedawson, 2016-11-15 19:52:20]

On 2016/11/15 19:17:00, Nico wrote:
> Is this still needed with vs2015?

Unknown. We'd have to try removing it. It does seem likely that Update 3 of VS
2015 would have a fix for this.