Re: [PATCH] intel-iommu: fix build with CONFIG_BRANCH_TRACER=y

David Woodhouse
2009-04-07T13:16:43Z

On Tue, 2009-04-07 at 14:57 +0200, Ingo Molnar wrote:
> * David Woodhouse <dwmw2@infradead.org> wrote:
>
> > On Tue, 2009-04-07 at 14:14 +0200, Ingo Molnar wrote:
> > > Well, i consider it a feature that it flags weird if (x, y)
> > > constructs: and yes, these iterators you introduced, while they are
> > > legit C, definitely count as 'weird'. If regular code was doing it,
> > > not a loop abstraction, i'd call it non-obvious and borderline
> > > broken straight away.
> > >
> > > We should _never ever_ put comma statements into if ()
> > > constructs without a _really_ good reason - and if yes, we can
> > > flag that we know what we are doing, via extra parentheses.
> >
> > I disagree. I don't think we should be declaring valid C syntax as
> > 'off limits', however rare it is.
> >
> > _Especially_ if it only actually fails with a fairly esoteric
> > config option set. That's just asking for build breakage.
>
> It failed within 10 minutes of testing for me. And i did not say
> 'off limits', i said 'needs a second look and a i-know-what-i-am
> doing flag'.
>
> Anyway, we apparently disagree about what constitutes acceptable
> code and what not. I am more cautious about "weird looking"
> constructs, and for a good reason: often a 'hm, this looks a bit
> weird' sub-conscious feeling is the only sign we have of a serious
> bug in the making.
>
> So making code not look weird and teaching people to not use weird
> looking patterns in usual code is a prime goal - at least to me.
>
> Does such an approach limit the C language? Yes, of course it does,
> that's the whole point. You appear to be more of the "if it's valid
> C it's fine" camp.

Well... "it's fine" implies a judgement call about its acceptability.
I'd certainly go for "if it's valid C, then it should _work_".

But failing that, I'll go for "if it's valid C and you want to make it
fail, then at least make it fail _consistently_ and give a coherent
error".

> > > and if yes, we can flag that we know what we are doing, via
> > > extra parentheses.
> >
> > That's hardly much of a barrier. The requirement to sprinkle
> > gratuitous-looking extra parentheses around the place really isn't
> > going to give us much of a _benefit_ in return for the build
> > breakage.
>
> The thing is, the branch-tracer might be even more weird than your
> iterator (it certainly is, and we might even remove it if it's
> causing undue problems), but it has been upstream for some time
> already and it is certainly useful for certain types of
> difficult-to-analyze codepaths.

I wouldn't suggest removing the branch-tracer. Just fixing it.

> Also, it is not breaking the build currently [with any given
> combination of weird .config combos] - so there's no 'sprinking'
> anywhere except your arguably under-tested iterator ;-)

It was lightly tested because it was obviously correct™.

For some weird reason I thought I was programming in C -- and it was a
trivial change to simplify the previous iterators which made me want to
poke my eyes out.

To be honest, it doesn't matter much -- it's such an unusual construct
that it's unlikely to come up again. So it doesn't _really_ matter if we
take the fix that I believe to be more correct, or the one I believe to
be a crappy workaround.

But I would ask that if you we do the latter, you make it fail in _all_
cases, not just with CONFIG_PROFILE_ALL_BRANCHES.

Making valid C code fall over, but _only_ when a certain esoteric config
option is set, is a _really_ bad idea.

--
David Woodhouse Open Source Technology Centre
David.Woodhouse@intel.com Intel Corporation