2011-03-14 09:55:47

by Ingo Molnar

[permalink] [raw]
Subject: Re: PATCH: Add --size-check=[error|warning]


(H.J. Lu, did you drop me from the Cc: line?)

* Jan Beulich <[email protected]> wrote:

> >> Please make it so that it'll be a warning by default, and an error
> >> upon programmer request. Otherwise, for the very purpose of
> >
> > I disagree. It should be error by default since the input is bogus,
> > Otherwise, those assembly bugs, benign or not, may not get
> > fixed.
> >
> >> bisection, it won't help much as you would have to override
> >> compiler/assembler flags during that process.
> >>
> >
> > They can use a wrapper to pass --size-check=warning to
> > assembler. I think it is a small price to pay for those mistakes.
>
> "Small" being relative here - it could be dozens if not hundreds of
> people having to learn that this is necessary, many of them
> possibly rather unfamiliar with gas and its command line options.
>
> Also, using a wrapper gets further complicated by the fact that
> you may have to pass an extra -B to the compiler (not everyone
> has full control over the file system of all the machines used to
> do development), making sure this doesn't have any other
> unwanted side effects.

Correct. In reality if the kernel does not build or boot then most people just wont
continue with the bisection. So this change actively degrades debuggability, for no
good reason.

The thing is, it is absolutely, breath-takingy incompetent for the new binutils
version to break the Linux kernel build for 4 years of Linux kernel history
retroactively (130,000 commits), just to 'warn' about a size bug in a few debug
symbols that has no functional effects whatsoever and which few people care about.

The correct solution is to turn it into a warning as me and others have suggested.

No argument was offered *why* the build must be aborted. A warning serves the
purpose of informing the developer just as much - and does not inconvenience the
tester.

Thanks,

Ingo


2011-03-14 10:41:44

by Alan Modra

[permalink] [raw]
Subject: Re: PATCH: Add --size-check=[error|warning]

On Mon, Mar 14, 2011 at 10:55:34AM +0100, Ingo Molnar wrote:
> The thing is, it is absolutely, breath-takingy incompetent for

kernel developers to write such poor asm! And not notice the error
for 4 years! Oh, and the binutils developers to write such a poor
assembler in the first place. ;-)

Seriously, you are complaining because something is fixed??

> The correct solution is to turn it into a warning as me and others have suggested.

I disagree. The whole world is not the linux kernel. I think HJ is
bending over backwards to even offer a switch that turns the error
into a warning.

--
Alan Modra
Australia Development Lab, IBM

2011-03-14 10:50:46

by Pekka Enberg

[permalink] [raw]
Subject: Re: PATCH: Add --size-check=[error|warning]

Hi Alan,

On Mon, Mar 14, 2011 at 12:41 PM, Alan Modra <[email protected]> wrote:
> On Mon, Mar 14, 2011 at 10:55:34AM +0100, Ingo Molnar wrote:
>> The thing is, it is absolutely, breath-takingy incompetent for
>
> kernel developers to write such poor asm! ?And not notice the error
> for 4 years! ?Oh, and the binutils developers to write such a poor
> assembler in the first place. ?;-)
>
> Seriously, you are complaining because something is fixed??
>
>> The correct solution is to turn it into a warning as me and others have suggested.
>
> I disagree. ?The whole world is not the linux kernel. ?I think HJ is
> bending over backwards to even offer a switch that turns the error
> into a warning.

So what do you suggest that testers who want to, say, build old Linux
kernel versions with new binutils do?

Pekka

2011-03-14 10:52:15

by Jan Beulich

[permalink] [raw]
Subject: Re: PATCH: Add --size-check=[error|warning]

>>> On 14.03.11 at 11:41, Alan Modra <[email protected]> wrote:
> On Mon, Mar 14, 2011 at 10:55:34AM +0100, Ingo Molnar wrote:
>> The thing is, it is absolutely, breath-takingy incompetent for
>
> kernel developers to write such poor asm! And not notice the error
> for 4 years! Oh, and the binutils developers to write such a poor
> assembler in the first place. ;-)
>
> Seriously, you are complaining because something is fixed??
>
>> The correct solution is to turn it into a warning as me and others have
> suggested.
>
> I disagree. The whole world is not the linux kernel. I think HJ is
> bending over backwards to even offer a switch that turns the error
> into a warning.

Just to repeat what I said in a previous reply - an error should be
issued if it is impossible for the assembler to produce valid output.
Anything less severe should be a warning.

In the case given, as also stated before, simply not issuing anything
to the object file if .size has an invalid operand is quite feasible for
the assembler to do, and won't produce invalid output. The warning
tells the programmer that not everything (s)he intended to be in
the object file actually made it there.

Jan

2011-03-14 12:23:53

by Ingo Molnar

[permalink] [raw]
Subject: Re: PATCH: Add --size-check=[error|warning]


* Alan Modra <[email protected]> wrote:

> On Mon, Mar 14, 2011 at 10:55:34AM +0100, Ingo Molnar wrote:
> > The thing is, it is absolutely, breath-takingy incompetent for
>
> kernel developers to write such poor asm! And not notice the error
> for 4 years! [...]

It is not 'poor asm'.

The 'bug' is just a slight assymetry in ENTRY()/END() debug-symbols sequences, with
lots of assembly code between the ENTRY() and the END(). Here's an example:

ENTRY(xen_do_hypervisor_callback) # do_hypervisor_callback(struct *pt_regs)
...
END(do_hypervisor_callback)

Human reviewers almost never catch such small mismatches, and binutils never even
warned about it either - for over a decade.

Now kernel bisections are insta-broken on latest binutils, and there's nothing to do
about it on the kernel side as during bisection all later fixes are unfolded. The
fix itself i already applied - but my argument was not about that:

> [...] Oh, and the binutils developers to write such a poor assembler in the first
> place. ;-)
>
> Seriously, you are complaining because something is fixed??

No, i reported this bug because the kernel build gets broken going back 130,000
commits, breaking bisection and causing other damage - while issuing a warning
message would achieve the same effect of warning the developer about the mismatch.

> > The correct solution is to turn it into a warning as me and others have suggested.
>
> I disagree. The whole world is not the linux kernel. I think HJ is
> bending over backwards to even offer a switch that turns the error
> into a warning.

It's not about a switch at all - it's to not break builds by default. I.e. the
default behavior should be to issue a warning and ignore the directive.

This is a very simple concept of compatibility: the build environment should always
be very permissive - stuff that build fine before should be allowed to build.

Also, i hope you are not suggesting to break projects just because they are not
important to you personally? The fix is exceedingly simple to do for the binutils
project - and impossible to do for the kernel project (because during bisection -
which is a very powerful debugging tool - older versions of the source get checked
out).

Thanks,

Ingo

2011-03-14 12:25:54

by Ingo Molnar

[permalink] [raw]
Subject: Re: PATCH: Add --size-check=[error|warning]


(resend, fixed the To line)

* Alan Modra <[email protected]> wrote:

> On Mon, Mar 14, 2011 at 10:55:34AM +0100, Ingo Molnar wrote:
> > The thing is, it is absolutely, breath-takingy incompetent for
>
> kernel developers to write such poor asm! And not notice the error
> for 4 years! [...]

It is not 'poor asm'.

The 'bug' is just a slight assymetry in ENTRY()/END() debug-symbols sequences, with
lots of assembly code between the ENTRY() and the END(). Here's an example:

ENTRY(xen_do_hypervisor_callback) # do_hypervisor_callback(struct *pt_regs)
...
END(do_hypervisor_callback)

Human reviewers almost never catch such small mismatches, and binutils never even
warned about it either - for over a decade.

Now kernel bisections are insta-broken on latest binutils, and there's nothing to do
about it on the kernel side as during bisection all later fixes are unfolded. The
fix itself i already applied - but my argument was not about that:

> [...] Oh, and the binutils developers to write such a poor assembler in the first
> place. ;-)
>
> Seriously, you are complaining because something is fixed??

No, i reported this bug because the kernel build gets broken going back 130,000
commits, breaking bisection and causing other damage - while issuing a warning
message would achieve the same effect of warning the developer about the mismatch.

> > The correct solution is to turn it into a warning as me and others have suggested.
>
> I disagree. The whole world is not the linux kernel. I think HJ is
> bending over backwards to even offer a switch that turns the error
> into a warning.

It's not about a switch at all - it's to not break builds by default. I.e. the
default behavior should be to issue a warning and ignore the directive.

This is a very simple concept of compatibility: the build environment should always
be very permissive - stuff that build fine before should be allowed to build.

Also, i hope you are not suggesting to break projects just because they are not
important to you personally? The fix is exceedingly simple to do for the binutils
project - and impossible to do for the kernel project (because during bisection -
which is a very powerful debugging tool - older versions of the source get checked
out).

Thanks,

Ingo

2011-03-14 13:16:30

by Ralf Wildenhues

[permalink] [raw]
Subject: git bisect plus fixes (was: PATCH: Add --size-check=[error|warning])

[ adding the git list; this is
http://thread.gmane.org/gmane.comp.gnu.binutils/52601/focus=1112779 ]

Hello,

* Ingo Molnar wrote on Mon, Mar 14, 2011 at 01:23:42PM CET:
> Also, i hope you are not suggesting to break projects just because
> they are not important to you personally? The fix is exceedingly
> simple to do for the binutils project - and impossible to do for the
> kernel project (because during bisection - which is a very powerful
> debugging tool - older versions of the source get checked out).

FWIW, I don't have an opinion on this particular binutils issue, but
it would be very helpful if 'git bisect' made it easy to denote
"when going back, you might also need some of these changes".
(I'd just use a patch -p1 with a here-file in the bisect script, but
that might not be enough for all practical use cases.)

This issue has come up several times with high-profile issues.

Thanks,
Ralf

2011-03-14 18:04:27

by Alan

[permalink] [raw]
Subject: Re: PATCH: Add --size-check=[error|warning]

> > I disagree. ?The whole world is not the linux kernel. ?I think HJ is
> > bending over backwards to even offer a switch that turns the error
> > into a warning.
>
> So what do you suggest that testers who want to, say, build old Linux
> kernel versions with new binutils do?

Use an older binutils. It's already the case you can't build old kernels
with current gcc and binutils, there have been repeated such events over
time and nobody has had too much trouble.

Alan