Message-ID: <50915418.2030607@att.net>
Date:	Wed, 31 Oct 2012 11:38:48 -0500
From:	Daniel Santos <danielfsantos@....net>
To:	Borislav Petkov <bp@...en8.de>,
	Daniel Santos <daniel.santos@...ox.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Andi Kleen <ak@...ux.intel.com>,
	Andrea Arcangeli <aarcange@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Christopher Li <sparse@...isli.org>,
	David Daney <david.daney@...ium.com>,
	David Howells <dhowells@...hat.com>,
	Joe Perches <joe@...ches.com>,
	Josh Triplett <josh@...htriplett.org>,
	Konstantin Khlebnikov <khlebnikov@...nvz.org>,
	linux-sparse@...r.kernel.org,
	Michel Lespinasse <walken@...gle.com>,
	Paul Gortmaker <paul.gortmaker@...driver.com>,
	Pavel Pisa <pisa@....felk.cvut.cz>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Steven Rostedt <rostedt@...dmis.org>,
	David Rientjes <rientjes@...gle.com>
Subject: Re: [PATCH v4 6/9] compiler.h, bug.h: Prevent double error messages
 with BUILD_BUG{,_ON}

On 10/31/2012 06:06 AM, Borislav Petkov wrote:
> On Wed, Oct 31, 2012 at 12:34:45AM -0500, Daniel Santos wrote:
>> Yes, the __build_bug_on_failed message is much more informative.  This
>> will only increase with these patches.  For example, the line
>>
>> BUILD_BUG_ON(sizeof(*c) != 4);
>>
>> emits this error:
>>
>> arch/x86/kernel/cpu/amd.c: In function ‘early_init_amd’:
>> arch/x86/kernel/cpu/amd.c:486:2: error: call to
>> ‘__build_bug_on_failed_486’ declared with attribute error: BUILD_BUG_ON
>> failed: sizeof(*c) != 4
>> make[1]: *** [arch/x86/kernel/cpu/amd.o] Error 1
>> make: *** [arch/x86/kernel/cpu/amd.o] Error 2
>>
>> It's true that there is some redundancy in there as well as the
>> gibberish line number embedded in the function name, but the end of the
>> line spits out the exact statement that failed.
> I guess that's as good as it gets. But it's fine IMO, it tells you
> exactly what you need to know.
Yeah, that's what I'm thinking as well.  Of course, I'm *always* happy
for somebody to come up with a superior solution! :)
>
>> But as far as rather the fallback is first or the __compiletime_error
>> function is a matter of asthetics, since it's really an either/or
>> situation.  Either the __build_bug_on_failedxxx function will be
>> declared with __attribute__((error(message))) and the fallback will
>> expand to a no-op, or the fallback will produce code that (presumably
>> always?) breaks the build.  For insurance, a link-time error will occur
>> if the fallback code fails to break the build.
> Right, but my suggestion was to have the more informative message always
> trigger first, if possible and if gcc supports it (practically, more
> and more systems will be upgrading gcc which has the error attribute
> with time) and have the less informative one be the more seldom one. The
> "fallback" naming is just a minor issue.
>
> This way, the error message would be precise on most modern toolchains.
> Older toolchains will issue something about negative array size, which
> is not really helpful so one would have to fire up an editor and
> actually look at the code :).

lol! :)  Yeah, this is exactly how it should be behaving at this point,
although it's not too clear with the "fallback" macro being defined
elsewhere that it's doing nothing when the error attribute is
available.  I suppose this is another reason to move the whole mechanism
to compiler*.h or add some comments to clarify what's going on.
>> Realistically, a single macro could be defined in compiler*.h that
>> encapsulates the entirety of this mechanism and only exposes a "black
>> box" macro, that will simply expand to something that breaks the build
>> in the most appropriate fashion based upon the version of gcc.  In
>> essence, the new BUILD_BUG_ON_MSG macro attempts to fill that roll.
> Yes.

Hmmm, this gets tricky.  So I think you are talking about a single
function-like macro that will create the built-time error.  As I see it,
we'll need to move the entirety of __BUILD_BUG_INTERNAL (with the double
evaluation of condition fixed) into compiler*.h, even if we change the
name of the macro.  The alternatives are to a.) further spitting out the
pieces of it into separate little macros (like
__compiletime_error_fallback), which I'm not fond of or b.) allow bug.h
to hold details about compiler functionality, which doesn't seem right
at all.  Let me play with this some and see what I can figure out.

>
>> I guess I'll fix it up (and address the emails on the other patches)
>> and do a v5 then for the whole set? (is that the right way to resubmit
>> with these corrections?)
> Well, you could wait a couple of days first to gather feedback from
> other people and then resend. This way you give chance to people to take
> a look without them seeing too many versions of the patchset and getting
> confused.
>
> What I always do is send out the patchset, collect and discuss changes,
> add in the required changes and test it while the discussions go on.
> After they settle down (and they do in a couple of days, in most cases)
> I then send out the newly tested version out.
>
> That whole exercise takes more or less a week if you're doing other
> stuff in between :)

Ahh, helpful guidelines, thanks so much! :)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists