2011-03-14 11:02:50

by Sedat Dilek

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

[QUOTE]
(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
[/QUOTE]

Nice to see there is an offer for a fix from binutils-side.

The followers of this "issue" like me have read the arguments from
kernel-developers.
Unfortunately, the Open Source world is not the linux-kernel alone.
I have built in the meantime a lot of Xorg stuff from GIT, etc. with a
binutils 2.21-snapshot (plus additional backported patches from
upstream).

If the goal is to catch real BUGs in the kernel, the current behaviour
of binutils/as is IMHO correct.
That's why I am on linux-next to squash bugs, not to ignore "warnings"

BTW "warnings", did someone tried gcc-4.6?
I used a snapshot from mid February (from Debian/experimental):
My linux-next build-log and the amount of warnings doubled or even
more (unfortunately, I have thrown away that logs and binaries).
Are all of these warnings ignoreable?
Which of them are really severe?

So, H.J. was pro-active in the meantime by offering this patch.
>From kernel-side it's getting IMHO more and more some sort of "burning
of witches".

Thus some questions to the kernel folks:

[1] Jan, what do you mean by "side-effects". Can you explain that a
bit more precisely?

[2] Where can someone set a "global behaviour" (hardcoded options) for
his/her assembler in the kernel's build-system (speaking of
"--size-check=[error|warning]")?

[3] Can the kernel-buildsystem check for system's binutils/as version
and/or its features/options? If yes, where would that be and can you
offer a snippet for a solution?

Answering and/or offering solutions for my askings can help to handle
things from kernel-side.

My 0,02EUR.

- Sedat -


2011-03-14 11:25:17

by Jan Beulich

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

>>> On 14.03.11 at 12:02, Sedat Dilek <[email protected]> wrote:
> [1] Jan, what do you mean by "side-effects". Can you explain that a
> bit more precisely?

The -B compiler option controls more than just finding "helper" binaries.

> [2] Where can someone set a "global behaviour" (hardcoded options) for
> his/her assembler in the kernel's build-system (speaking of
> "--size-check=[error|warning]")?

Nowhere, selecting behavior is possible only via the command line.

> [3] Can the kernel-buildsystem check for system's binutils/as version
> and/or its features/options? If yes, where would that be and can you
> offer a snippet for a solution?

Making the kernel build system check for certain newly introduced
gas options would again require changes to the kernel sources,
which is precisely what is impossible to do for past kernel releases
(and bisection in particular).

Jan

2011-03-14 11:35:42

by Ingo Molnar

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


* Jan Beulich <[email protected]> wrote:

> >>> On 14.03.11 at 12:02, Sedat Dilek <[email protected]> wrote:
> > [1] Jan, what do you mean by "side-effects". Can you explain that a
> > bit more precisely?
>
> The -B compiler option controls more than just finding "helper" binaries.
>
> > [2] Where can someone set a "global behaviour" (hardcoded options) for
> > his/her assembler in the kernel's build-system (speaking of
> > "--size-check=[error|warning]")?
>
> Nowhere, selecting behavior is possible only via the command line.
>
> > [3] Can the kernel-buildsystem check for system's binutils/as version
> > and/or its features/options? If yes, where would that be and can you
> > offer a snippet for a solution?
>
> Making the kernel build system check for certain newly introduced
> gas options would again require changes to the kernel sources,
> which is precisely what is impossible to do for past kernel releases
> (and bisection in particular).

Yes, and all the counter-arguments here continue to miss that very simple point.
That point was made in the first post about this topic and it's still not
acknowledged - let alone addressed.

This breakage is unnecessary and retroactively goes back 130,000 commits. A warning
combined with not issuing the debug symbol would be just as fine and would still
result in valid output.

Thanks,

Ingo

2011-03-14 11:38:59

by Sedat Dilek

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

[ Changing Alan's Email to valid <[email protected]> in CC ]

On Mon, Mar 14, 2011 at 12:26 PM, Jan Beulich <[email protected]> wrote:
>>>> On 14.03.11 at 12:02, Sedat Dilek <[email protected]> wrote:
[...]
>> [2] Where can someone set a "global behaviour" (hardcoded options) for
>> his/her assembler in the kernel's build-system (speaking of
>> "--size-check=[error|warning]")?
>
> Nowhere, selecting behavior is possible only via the command line.
>

Via command-line, something like this would do it?

$ export AS="/usr/bin/as --size-check=warning
[...]

- Sedat -

2011-03-14 11:52:53

by Sedat Dilek

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

On Mon, Mar 14, 2011 at 12:38 PM, Sedat Dilek
<[email protected]> wrote:
> [ Changing Alan's Email to valid <[email protected]> in CC ]
>
> On Mon, Mar 14, 2011 at 12:26 PM, Jan Beulich <[email protected]> wrote:
>>>>> On 14.03.11 at 12:02, Sedat Dilek <[email protected]> wrote:
> [...]
>>> [2] Where can someone set a "global behaviour" (hardcoded options) for
>>> his/her assembler in the kernel's build-system (speaking of
>>> "--size-check=[error|warning]")?
>>
>> Nowhere, selecting behavior is possible only via the command line.
>>
>
> Via command-line, something like this would do it?
>
>     $ export AS="/usr/bin/as --size-check=warning
> [...]
>
> - Sedat -
>

By looking through the binutils source-code, I have seen ASFLAGS.

$ ASFLAGS="--size-check=warning" ; export ASFLAGS

Would that work?

- Sedat -

2011-03-14 12:20:58

by Jan Beulich

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

>>> On 14.03.11 at 12:52, Sedat Dilek <[email protected]> wrote:
> On Mon, Mar 14, 2011 at 12:38 PM, Sedat Dilek
> <[email protected]> wrote:
>> [ Changing Alan's Email to valid <[email protected]> in CC ]
>>
>> On Mon, Mar 14, 2011 at 12:26 PM, Jan Beulich <[email protected]> wrote:
>>>>>> On 14.03.11 at 12:02, Sedat Dilek <[email protected]> wrote:
>> [...]
>>>> [2] Where can someone set a "global behaviour" (hardcoded options) for
>>>> his/her assembler in the kernel's build-system (speaking of
>>>> "--size-check=[error|warning]")?
>>>
>>> Nowhere, selecting behavior is possible only via the command line.
>>>
>>
>> Via command-line, something like this would do it?
>>
>> $ export AS="/usr/bin/as --size-check=warning
>> [...]

No - $(AS) isn't being used to translate .S files, $(CC) is being used
instead. That's why I pointed at the necessary -B option (so that
the compiler would be able to locate the wrapper H.J. suggested).

> By looking through the binutils source-code, I have seen ASFLAGS.
>
> $ ASFLAGS="--size-check=warning" ; export ASFLAGS

Where did you find that? All places where I see references to this
variable are in the testsuite.

Jan

2011-03-14 12:38:22

by Sedat Dilek

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

Hi H.J.,

against which binutils source-code is your patch?
I tried binutils-from-git (last commit 83e680a: "daily update") and
your binutils-2.21.51.0.7.

None of them have a gas/testsuite/gas/i386/bad-size.d file, so...

diff --git a/gas/testsuite/gas/i386/bad-size.d
b/gas/testsuite/gas/i386/bad-size.d
index be9655e..0bcf381 100644
--- a/gas/testsuite/gas/i386/bad-size.d
+++ b/gas/testsuite/gas/i386/bad-size.d
@@ -1,7 +1,7 @@
#as: --size-check=warning
#objdump: -dw
#name: Check bad size directive
-#error-output: bad-size.err
+#error-output: bad-size.warn

.*: +file format .*

... cannot be applied.

Also, there are no gas/ChangeLog.x86 and gas/testsuite/ChangeLog.x86
files (but w/o *.x86).

Can you resend a proper patch against binutils-from-upstream?
Thanks.

Regards,
- Sedat -

2011-03-14 12:51:10

by Sedat Dilek

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

On Mon, Mar 14, 2011 at 1:21 PM, Jan Beulich <[email protected]> wrote:
>>>> On 14.03.11 at 12:52, Sedat Dilek <[email protected]> wrote:
>> On Mon, Mar 14, 2011 at 12:38 PM, Sedat Dilek
>> <[email protected]> wrote:
>>> [ Changing Alan's Email to valid <[email protected]> in CC ]
>>>
>>> On Mon, Mar 14, 2011 at 12:26 PM, Jan Beulich <[email protected]> wrote:
>>>>>>> On 14.03.11 at 12:02, Sedat Dilek <[email protected]> wrote:
>>> [...]
>>>>> [2] Where can someone set a "global behaviour" (hardcoded options) for
>>>>> his/her assembler in the kernel's build-system (speaking of
>>>>> "--size-check=[error|warning]")?
>>>>
>>>> Nowhere, selecting behavior is possible only via the command line.
>>>>
>>>
>>> Via command-line, something like this would do it?
>>>
>>>     $ export AS="/usr/bin/as --size-check=warning
>>> [...]
>
> No - $(AS) isn't being used to translate .S files, $(CC) is being used
> instead. That's why I pointed at the necessary -B option (so that
> the compiler would be able to locate the wrapper H.J. suggested).
>

OK, now I am enlightened in things of -B option.
Anyway, can I see the correct command line to use?
(OMG, I am glad this is not the $500.000 question @ "Who Wants to Be a
Millionaire?" quiz show. Jan wanna be my telephone joker :-)?)

>> By looking through the binutils source-code, I have seen ASFLAGS.
>>
>>      $ ASFLAGS="--size-check=warning" ; export ASFLAGS
>
> Where did you find that? All places where I see references to this
> variable are in the testsuite.
>
> Jan
>

WildWildWeb, IIRC I saw ASFLAGS used in a Gentoo package and AS in
Cross-LFS/binutils build instructions.

- Sedat -

2011-03-14 12:57:13

by Ingo Molnar

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


* Sedat Dilek <[email protected]> wrote:

> Nice to see there is an offer for a fix from binutils-side.

Agreed.

> That's why I am on linux-next to squash bugs, not to ignore "warnings"

We x86 arch maintainers definitely do not ignore warnings from the assembler.
Assembler warnings were pretty good historically and seldom produce false positives.

> BTW "warnings", did someone tried gcc-4.6? I used a snapshot from mid February
> (from Debian/experimental): My linux-next build-log and the amount of warnings
> doubled or even more (unfortunately, I have thrown away that logs and binaries).
> Are all of these warnings ignoreable? Which of them are really severe?

Most of those are -Wunused-but-set-variable warnings, right? I'm definitely not
ignoring the ones that affect the code i maintain - so they are very much useful.

But if GCC broke the build unnecessarily, just to over-eagerly warn about something
that worked fine before, that would be extremely counter-productive! In such a
situation the Linux kernel project would likely be fed up enough to build its own
sane compiler/assembler/linker combo and would aim to become entirely independent in
terms of its build environment.

Thanks,

Ingo

2011-03-14 15:56:39

by Ian Lance Taylor

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

"Jan Beulich" <[email protected]> writes:

> No - $(AS) isn't being used to translate .S files, $(CC) is being used
> instead. That's why I pointed at the necessary -B option (so that
> the compiler would be able to locate the wrapper H.J. suggested).

You could also just put a -Wa option in your $(CC). That seems simpler
than a -B option.

(I don't really care about this issue one way or another.)

Ian

2011-03-14 16:33:32

by H. Peter Anvin

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

On 03/14/2011 04:26 AM, Jan Beulich wrote:
>
> Making the kernel build system check for certain newly introduced
> gas options would again require changes to the kernel sources,
> which is precisely what is impossible to do for past kernel releases
> (and bisection in particular).
>

But something like "make CC='gcc -Wa,--size-check=warning'" should work,
I believe (tweaking may be required, but that's the idea). Passing an
option to the assembler is a helluva lot easier than redirecting to a
different assembler.

-hpa

--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.

2011-03-14 18:02:50

by H. Peter Anvin

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

On 03/14/2011 08:56 AM, Ian Lance Taylor wrote:
> "Jan Beulich" <[email protected]> writes:
>
>> No - $(AS) isn't being used to translate .S files, $(CC) is being used
>> instead. That's why I pointed at the necessary -B option (so that
>> the compiler would be able to locate the wrapper H.J. suggested).
>
> You could also just put a -Wa option in your $(CC). That seems simpler
> than a -B option.
>
> (I don't really care about this issue one way or another.)
>

-Wa is definitely the sane way to do it. The -B thing came in if you
have to run a patched version of as, which is extremely painful.

-hpa

2011-03-14 19:24:49

by Steven Rostedt

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

On Mon, Mar 14, 2011 at 09:32:56AM -0700, H. Peter Anvin wrote:
> On 03/14/2011 04:26 AM, Jan Beulich wrote:
> >
> > Making the kernel build system check for certain newly introduced
> > gas options would again require changes to the kernel sources,
> > which is precisely what is impossible to do for past kernel releases
> > (and bisection in particular).
> >
>
> But something like "make CC='gcc -Wa,--size-check=warning'" should work,
> I believe (tweaking may be required, but that's the idea). Passing an
> option to the assembler is a helluva lot easier than redirecting to a
> different assembler.
>
> -hpa
>

Even if the above does work, how do we go about educating users doing
bisects with latest binutils? This is a very common practice among
kernel developers with users that hit bugs. And right now there's just a
handful of people that know of this work-around.

It will become a huge burden to us and our users (which is everyone
using Linux), if we do not understand the reason a build breaks when
doing a bisect, just because some "bug" in asm which binutils use to
work with now errors on.

If it was a bug in asm, but binutils can cope with it, then it should be
a warning. If binutils can't cope, then error.

-- Steve

2011-03-14 19:59:36

by Linus Torvalds

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

On Mon, Mar 14, 2011 at 12:24 PM, Steven Rostedt <[email protected]> wrote:
>
> If it was a bug in asm, but binutils can cope with it, then it should be
> a warning. If binutils can't cope, then error.

I think this is the really fundamental issue: anybody who makes a hard
error out of something that is recoverable is a total moron.

We have that in the kernel too - I've berated people for using
BUG_ON() _much_ too eagerly. If you make a hard error out of
something, that just makes things much much harder to handle, and is
just a big inconvenience for everybody. Why do it?

In the kernel, a hard error (like BUG_ON()) tends to result in a
system that is unusable and makes logging things harder. And in
development tools, a hard error just means that you stop _everybody_,
whether the user is a developer or just a tester who can't
realistically fix it (or a developer who is not involved in that
area). And even for developers who _are_ directly involved, a hard
error stops the build, instead of just letting it continue and help
him see if there are perhaps _other_ cases that should also be fixed.

So anybody who makes something a hard error when it's not required is
just being a STUPID. It hurts everybody. Don't do it.

Linus