PATCH: Add --size-check=[error|warning]

classic Classic list List threaded Threaded
36 messages Options
12
Reply | Threaded
Open this post in threaded view
|

PATCH: Add --size-check=[error|warning]

H.J. Lu
Hi,

Issue an error for bad ELF .size directive on Linux kernel bisect where
the bad assembly codes aren't fixed.  This patch adds
--size-check=[error|warning] so that we can issue a warning instead of
an error.  OK to install?

Thanks.


H.J.
---
diff --git a/gas/ChangeLog.x86 b/gas/ChangeLog.x86
index 33bf5bb..2787c65 100644
--- a/gas/ChangeLog.x86
+++ b/gas/ChangeLog.x86
@@ -1,3 +1,15 @@
+2011-03-11  H.J. Lu  <[hidden email]>
+
+ * as.c (show_usage): Add --size-check=.
+ (parse_args): Add and handle OPTION_SIZE_CHECK.
+
+ * as.h (flag_size_check): New.
+
+ * config/obj-elf.c (elf_frob_symbol): Use as_bad to report
+ bad .size directive only for --size-check=error.
+
+ * doc/as.texinfo: Document --size-check=.
+
 2011-01-15  H.J. Lu  <[hidden email]>
 
  * config/tc-i386.c (disallow_64bit_disp): New.
diff --git a/gas/as.c b/gas/as.c
index 2c55047..380259c 100644
--- a/gas/as.c
+++ b/gas/as.c
@@ -284,6 +284,9 @@ Options:\n\
   --execstack             require executable stack for this object\n"));
   fprintf (stream, _("\
   --noexecstack           don't require executable stack for this object\n"));
+  fprintf (stream, _("\
+  --size-check=[error|warning]\n\
+  ELF .size directive check (default --size-check=error)\n"));
 #endif
   fprintf (stream, _("\
   -f                      skip whitespace and comment preprocessing\n"));
@@ -443,6 +446,7 @@ parse_args (int * pargc, char *** pargv)
       OPTION_TARGET_HELP,
       OPTION_EXECSTACK,
       OPTION_NOEXECSTACK,
+      OPTION_SIZE_CHECK,
       OPTION_ALTERNATE,
       OPTION_AL,
       OPTION_HASH_TABLE_SIZE,
@@ -476,6 +480,7 @@ parse_args (int * pargc, char *** pargv)
 #if defined OBJ_ELF || defined OBJ_MAYBE_ELF
     ,{"execstack", no_argument, NULL, OPTION_EXECSTACK}
     ,{"noexecstack", no_argument, NULL, OPTION_NOEXECSTACK}
+    ,{"size-check", required_argument, NULL, OPTION_SIZE_CHECK}
 #endif
     ,{"fatal-warnings", no_argument, NULL, OPTION_WARN_FATAL}
     ,{"gdwarf-2", no_argument, NULL, OPTION_GDWARF2}
@@ -813,6 +818,15 @@ This program has absolutely no warranty.\n"));
   flag_noexecstack = 1;
   flag_execstack = 0;
   break;
+
+ case OPTION_SIZE_CHECK:
+  if (strcasecmp (optarg, "error") == 0)
+    flag_size_check = size_check_error;
+  else if (strcasecmp (optarg, "warning") == 0)
+    flag_size_check = size_check_warning;
+  else
+    as_fatal (_("Invalid --size-check= option: `%s'"), optarg);
+  break;
 #endif
  case 'Z':
   flag_always_generate_output = 1;
diff --git a/gas/as.h b/gas/as.h
index 7c16382..5408e1a 100644
--- a/gas/as.h
+++ b/gas/as.h
@@ -575,6 +575,16 @@ COMMON unsigned int  found_comment;
 COMMON char *        found_comment_file;
 #endif
 
+#if defined OBJ_ELF || defined OBJ_MAYBE_ELF
+/* If .size directive failure should be error or warning.  */
+COMMON enum
+  {
+    size_check_error = 0,
+    size_check_warning
+  }
+flag_size_check;
+#endif
+
 #ifndef DOLLAR_AMBIGU
 #define DOLLAR_AMBIGU 0
 #endif
diff --git a/gas/config/obj-elf.c b/gas/config/obj-elf.c
index d43409a..37a8020 100644
--- a/gas/config/obj-elf.c
+++ b/gas/config/obj-elf.c
@@ -1898,6 +1898,12 @@ elf_frob_symbol (symbolS *symp, int *puntp)
  {
   const char *op_name = NULL;
   const char *add_name = NULL;
+  PRINTF_LIKE ((*as_error));
+
+  if (flag_size_check == size_check_error)
+    as_error = as_bad;
+  else
+    as_error = as_warn;
 
   if (size->X_op == O_subtract)
     {
@@ -1909,9 +1915,9 @@ elf_frob_symbol (symbolS *symp, int *puntp)
  add_name = NULL;
 
       if (op_name && add_name)
- as_bad (_(".size expression with symbols `%s' and `%s' "
-  "does not evaluate to a constant"),
- op_name, add_name);
+ as_error (_(".size expression with symbols `%s' and "
+    "`%s' does not evaluate to a constant"),
+  op_name, add_name);
       else
  {
   const char *name;
@@ -1924,13 +1930,15 @@ elf_frob_symbol (symbolS *symp, int *puntp)
     name = NULL;
 
   if (name)
-    as_bad (_(".size expression with symbol `%s' "
-      "does not evaluate to a constant"), name);
+    as_error (_(".size expression with symbol `%s' "
+ "does not evaluate to a constant"),
+      name);
  }
     }
   
   if (!op_name && !add_name)
-    as_bad (_(".size expression does not evaluate to a constant"));
+    as_error (_(".size expression does not evaluate to a "
+ "constant"));
  }
       free (sy_obj->size);
       sy_obj->size = NULL;
diff --git a/gas/doc/as.texinfo b/gas/doc/as.texinfo
index 748c96c..bb7f063 100644
--- a/gas/doc/as.texinfo
+++ b/gas/doc/as.texinfo
@@ -243,6 +243,7 @@ gcc(1), ld(1), and the Info entries for @file{binutils} and @file{ld}.
  @var{objfile}] [@b{-R}] [@b{--reduce-memory-overheads}] [@b{--statistics}]
  [@b{-v}] [@b{-version}] [@b{--version}] [@b{-W}] [@b{--warn}]
  [@b{--fatal-warnings}] [@b{-w}] [@b{-x}] [@b{-Z}] [@b{@@@var{FILE}}]
+ [@b{--size-check=[error|warning]}]
  [@b{--target-help}] [@var{target-options}]
  [@b{--}|@var{files} @dots{}]
 @c
@@ -611,6 +612,10 @@ Generate DWARF2 debugging information for each assembler line.  This
 may help debugging assembler code, if the debugger can handle it.  Note---this
 option is only supported by some targets, not all of them.
 
+@item --size-check=error
+@itemx --size-check=warning
+Issue an error or warning for invalid ELF .size directive.
+
 @item --help
 Print a summary of the command line options and exit.
 
diff --git a/gas/testsuite/ChangeLog.x86 b/gas/testsuite/ChangeLog.x86
index 01e62bc..bf76320 100644
--- a/gas/testsuite/ChangeLog.x86
+++ b/gas/testsuite/ChangeLog.x86
@@ -1,3 +1,11 @@
+2011-03-11  H.J. Lu  <[hidden email]>
+
+ * gas/i386/bad-size.d: New.
+ * gas/i386/bad-size.s: Likewise.
+ * gas/i386/bad-size.warn: Likewise.
+
+ * gas/i386/i386.exp: Run bad-size for ELF targets.
+
 2011-01-15  H.J. Lu  <[hidden email]>
 
  * gas/i386/ilp32/ilp32.exp: Run inval.
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 .*
 
diff --git a/gas/testsuite/gas/i386/i386.exp b/gas/testsuite/gas/i386/i386.exp
index 306da65..ea5cdac 100644
--- a/gas/testsuite/gas/i386/i386.exp
+++ b/gas/testsuite/gas/i386/i386.exp
@@ -228,6 +228,8 @@ if [expr ([istarget "i*86-*-*"] ||  [istarget "x86_64-*-*"]) && [gas_32_check]]
  run_dump_test "debug1"
 
  run_dump_test "dw2-compress-2"
+
+ run_dump_test "bad-size"
     }
 
     # This is a PE specific test.
Reply | Threaded
Open this post in threaded view
|

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

Jan Beulich
>>> On 11.03.11 at 17:58, "H.J. Lu" <[hidden email]> wrote:
> Issue an error for bad ELF .size directive on Linux kernel bisect where
> the bad assembly codes aren't fixed.  This patch adds
> --size-check=[error|warning] so that we can issue a warning instead of
> an error.  OK to install?

Please make it so that it'll be a warning by default, and an error
upon programmer request. Otherwise, for the very purpose of
bisection, it won't help much as you would have to override
compiler/assembler flags during that process.

Jan

> diff --git a/gas/ChangeLog.x86 b/gas/ChangeLog.x86
> index 33bf5bb..2787c65 100644
> --- a/gas/ChangeLog.x86
> +++ b/gas/ChangeLog.x86
> @@ -1,3 +1,15 @@
> +2011-03-11  H.J. Lu  <[hidden email]>
> +
> + * as.c (show_usage): Add --size-check=.
> + (parse_args): Add and handle OPTION_SIZE_CHECK.
> +
> + * as.h (flag_size_check): New.
> +
> + * config/obj-elf.c (elf_frob_symbol): Use as_bad to report
> + bad .size directive only for --size-check=error.
> +
> + * doc/as.texinfo: Document --size-check=.
> +
>  2011-01-15  H.J. Lu  <[hidden email]>
>  
>   * config/tc-i386.c (disallow_64bit_disp): New.
> diff --git a/gas/as.c b/gas/as.c
> index 2c55047..380259c 100644
> --- a/gas/as.c
> +++ b/gas/as.c
> @@ -284,6 +284,9 @@ Options:\n\
>    --execstack             require executable stack for this object\n"));
>    fprintf (stream, _("\
>    --noexecstack           don't require executable stack for this
> object\n"));
> +  fprintf (stream, _("\
> +  --size-check=[error|warning]\n\
> +  ELF .size directive check (default --size-check=error)\n"));
>  #endif
>    fprintf (stream, _("\
>    -f                      skip whitespace and comment preprocessing\n"));
> @@ -443,6 +446,7 @@ parse_args (int * pargc, char *** pargv)
>        OPTION_TARGET_HELP,
>        OPTION_EXECSTACK,
>        OPTION_NOEXECSTACK,
> +      OPTION_SIZE_CHECK,
>        OPTION_ALTERNATE,
>        OPTION_AL,
>        OPTION_HASH_TABLE_SIZE,
> @@ -476,6 +480,7 @@ parse_args (int * pargc, char *** pargv)
>  #if defined OBJ_ELF || defined OBJ_MAYBE_ELF
>      ,{"execstack", no_argument, NULL, OPTION_EXECSTACK}
>      ,{"noexecstack", no_argument, NULL, OPTION_NOEXECSTACK}
> +    ,{"size-check", required_argument, NULL, OPTION_SIZE_CHECK}
>  #endif
>      ,{"fatal-warnings", no_argument, NULL, OPTION_WARN_FATAL}
>      ,{"gdwarf-2", no_argument, NULL, OPTION_GDWARF2}
> @@ -813,6 +818,15 @@ This program has absolutely no warranty.\n"));
>    flag_noexecstack = 1;
>    flag_execstack = 0;
>    break;
> +
> + case OPTION_SIZE_CHECK:
> +  if (strcasecmp (optarg, "error") == 0)
> +    flag_size_check = size_check_error;
> +  else if (strcasecmp (optarg, "warning") == 0)
> +    flag_size_check = size_check_warning;
> +  else
> +    as_fatal (_("Invalid --size-check= option: `%s'"), optarg);
> +  break;
>  #endif
>   case 'Z':
>    flag_always_generate_output = 1;
> diff --git a/gas/as.h b/gas/as.h
> index 7c16382..5408e1a 100644
> --- a/gas/as.h
> +++ b/gas/as.h
> @@ -575,6 +575,16 @@ COMMON unsigned int  found_comment;
>  COMMON char *        found_comment_file;
>  #endif
>  
> +#if defined OBJ_ELF || defined OBJ_MAYBE_ELF
> +/* If .size directive failure should be error or warning.  */
> +COMMON enum
> +  {
> +    size_check_error = 0,
> +    size_check_warning
> +  }
> +flag_size_check;
> +#endif
> +
>  #ifndef DOLLAR_AMBIGU
>  #define DOLLAR_AMBIGU 0
>  #endif
> diff --git a/gas/config/obj-elf.c b/gas/config/obj-elf.c
> index d43409a..37a8020 100644
> --- a/gas/config/obj-elf.c
> +++ b/gas/config/obj-elf.c
> @@ -1898,6 +1898,12 @@ elf_frob_symbol (symbolS *symp, int *puntp)
>   {
>    const char *op_name = NULL;
>    const char *add_name = NULL;
> +  PRINTF_LIKE ((*as_error));
> +
> +  if (flag_size_check == size_check_error)
> +    as_error = as_bad;
> +  else
> +    as_error = as_warn;
>  
>    if (size->X_op == O_subtract)
>      {
> @@ -1909,9 +1915,9 @@ elf_frob_symbol (symbolS *symp, int *puntp)
>   add_name = NULL;
>  
>        if (op_name && add_name)
> - as_bad (_(".size expression with symbols `%s' and `%s' "
> -  "does not evaluate to a constant"),
> - op_name, add_name);
> + as_error (_(".size expression with symbols `%s' and "
> +    "`%s' does not evaluate to a constant"),
> +  op_name, add_name);
>        else
>   {
>    const char *name;
> @@ -1924,13 +1930,15 @@ elf_frob_symbol (symbolS *symp, int *puntp)
>      name = NULL;
>  
>    if (name)
> -    as_bad (_(".size expression with symbol `%s' "
> -      "does not evaluate to a constant"), name);
> +    as_error (_(".size expression with symbol `%s' "
> + "does not evaluate to a constant"),
> +      name);
>   }
>      }
>    
>    if (!op_name && !add_name)
> -    as_bad (_(".size expression does not evaluate to a constant"));
> +    as_error (_(".size expression does not evaluate to a "
> + "constant"));
>   }
>        free (sy_obj->size);
>        sy_obj->size = NULL;
> diff --git a/gas/doc/as.texinfo b/gas/doc/as.texinfo
> index 748c96c..bb7f063 100644
> --- a/gas/doc/as.texinfo
> +++ b/gas/doc/as.texinfo
> @@ -243,6 +243,7 @@ gcc(1), ld(1), and the Info entries for @file{binutils}
> and @file{ld}.
>   @var{objfile}] [@b{-R}] [@b{--reduce-memory-overheads}] [@b{--statistics}]
>   [@b{-v}] [@b{-version}] [@b{--version}] [@b{-W}] [@b{--warn}]
>   [@b{--fatal-warnings}] [@b{-w}] [@b{-x}] [@b{-Z}] [@b{@@@var{FILE}}]
> + [@b{--size-check=[error|warning]}]
>   [@b{--target-help}] [@var{target-options}]
>   [@b{--}|@var{files} @dots{}]
>  @c
> @@ -611,6 +612,10 @@ Generate DWARF2 debugging information for each assembler
> line.  This
>  may help debugging assembler code, if the debugger can handle it.  Note---this
>  option is only supported by some targets, not all of them.
>  
> +@item --size-check=error
> +@itemx --size-check=warning
> +Issue an error or warning for invalid ELF .size directive.
> +
>  @item --help
>  Print a summary of the command line options and exit.
>  
> diff --git a/gas/testsuite/ChangeLog.x86 b/gas/testsuite/ChangeLog.x86
> index 01e62bc..bf76320 100644
> --- a/gas/testsuite/ChangeLog.x86
> +++ b/gas/testsuite/ChangeLog.x86
> @@ -1,3 +1,11 @@
> +2011-03-11  H.J. Lu  <[hidden email]>
> +
> + * gas/i386/bad-size.d: New.
> + * gas/i386/bad-size.s: Likewise.
> + * gas/i386/bad-size.warn: Likewise.
> +
> + * gas/i386/i386.exp: Run bad-size for ELF targets.
> +
>  2011-01-15  H.J. Lu  <[hidden email]>
>  
>   * gas/i386/ilp32/ilp32.exp: Run inval.
> 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 .*
>  
> diff --git a/gas/testsuite/gas/i386/i386.exp b/gas/testsuite/gas/i386/i386.exp
> index 306da65..ea5cdac 100644
> --- a/gas/testsuite/gas/i386/i386.exp
> +++ b/gas/testsuite/gas/i386/i386.exp
> @@ -228,6 +228,8 @@ if [expr ([istarget "i*86-*-*"] ||  [istarget "x86_64-*-*"]) &&
> [gas_32_check]]
>   run_dump_test "debug1"
>  
>   run_dump_test "dw2-compress-2"
> +
> + run_dump_test "bad-size"
>      }
>  
>      # This is a PE specific test.


Reply | Threaded
Open this post in threaded view
|

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

H.J. Lu-30
On Fri, Mar 11, 2011 at 9:05 AM, Jan Beulich <[hidden email]> wrote:
>>>> On 11.03.11 at 17:58, "H.J. Lu" <[hidden email]> wrote:
>> Issue an error for bad ELF .size directive on Linux kernel bisect where
>> the bad assembly codes aren't fixed.  This patch adds
>> --size-check=[error|warning] so that we can issue a warning instead of
>> an error.  OK to install?
>
> 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.


--
H.J.
Reply | Threaded
Open this post in threaded view
|

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

Jan Beulich
>>> On 11.03.11 at 18:19, "H.J. Lu" <[hidden email]> wrote:
> On Fri, Mar 11, 2011 at 9:05 AM, Jan Beulich <[hidden email]> wrote:
>>>>> On 11.03.11 at 17:58, "H.J. Lu" <[hidden email]> wrote:
>>> Issue an error for bad ELF .size directive on Linux kernel bisect where
>>> the bad assembly codes aren't fixed.  This patch adds
>>> --size-check=[error|warning] so that we can issue a warning instead of
>>> an error.  OK to install?
>>
>> 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.

Jan

Reply | Threaded
Open this post in threaded view
|

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

Ingo Molnar

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

* Jan Beulich <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Alan Modra-3
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
Reply | Threaded
Open this post in threaded view
|

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

Pekka Enberg-4
Hi Alan,

On Mon, Mar 14, 2011 at 12:41 PM, Alan Modra <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Jan Beulich
In reply to this post by Alan Modra-3
>>> On 14.03.11 at 11:41, Alan Modra <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

Andreas Schwab-4
In reply to this post by Alan Modra-3
Pekka Enberg <[hidden email]> writes:

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

The same that testers have to do in order to build old Linux kernel
versions with current versions of make.

Andreas.

--
Andreas Schwab, [hidden email]
GPG Key fingerprint = D4E8 DBE3 3813 BB5D FA84  5EC7 45C6 250E 6F00 984E
"And now for something completely different."
Reply | Threaded
Open this post in threaded view
|

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

Sedat Dilek
In reply to this post by H.J. Lu
[QUOTE]
(H.J. Lu, did you drop me from the Cc: line?)

* Jan Beulich <[hidden email]> 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 -
Reply | Threaded
Open this post in threaded view
|

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

Pekka Enberg-4
In reply to this post by Andreas Schwab-4
Hi Andreas,

Pekka Enberg <[hidden email]> writes:
>> So what do you suggest that testers who want to, say, build old Linux
>> kernel versions with new binutils do?

On Mon, Mar 14, 2011 at 1:02 PM, Andreas Schwab <[hidden email]> wrote:
> The same that testers have to do in order to build old Linux kernel
> versions with current versions of make.

Are you saying it's OK to screw over binutils users because GNU Make
did that too?

                       Pekka
Reply | Threaded
Open this post in threaded view
|

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

Jan Beulich
In reply to this post by Sedat Dilek
>>> On 14.03.11 at 12:02, Sedat Dilek <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

Ingo Molnar

* Jan Beulich <[hidden email]> wrote:

> >>> On 14.03.11 at 12:02, Sedat Dilek <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Sedat Dilek
In reply to this post by Jan Beulich
[ Changing Alan's Email to valid <[hidden email]> in CC ]

On Mon, Mar 14, 2011 at 12:26 PM, Jan Beulich <[hidden email]> wrote:
>>>> On 14.03.11 at 12:02, Sedat Dilek <[hidden email]> 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 -
Reply | Threaded
Open this post in threaded view
|

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

Sedat Dilek
On Mon, Mar 14, 2011 at 12:38 PM, Sedat Dilek
<[hidden email]> wrote:

> [ Changing Alan's Email to valid <[hidden email]> in CC ]
>
> On Mon, Mar 14, 2011 at 12:26 PM, Jan Beulich <[hidden email]> wrote:
>>>>> On 14.03.11 at 12:02, Sedat Dilek <[hidden email]> 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 -
Reply | Threaded
Open this post in threaded view
|

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

Andreas Schwab-4
In reply to this post by Pekka Enberg-4
Pekka Enberg <[hidden email]> writes:

> Hi Andreas,
>
> Pekka Enberg <[hidden email]> writes:
>>> So what do you suggest that testers who want to, say, build old Linux
>>> kernel versions with new binutils do?
>
> On Mon, Mar 14, 2011 at 1:02 PM, Andreas Schwab <[hidden email]> wrote:
>> The same that testers have to do in order to build old Linux kernel
>> versions with current versions of make.
>
> Are you saying it's OK to screw over binutils users because GNU Make
> did that too?

I'm just telling you the facts.

Andreas.

--
Andreas Schwab, [hidden email]
GPG Key fingerprint = D4E8 DBE3 3813 BB5D FA84  5EC7 45C6 250E 6F00 984E
"And now for something completely different."
Reply | Threaded
Open this post in threaded view
|

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

Ingo Molnar
In reply to this post by Pekka Enberg-4

* Pekka Enberg <[hidden email]> wrote:

> Hi Andreas,
>
> Pekka Enberg <[hidden email]> writes:
> >> So what do you suggest that testers who want to, say, build old Linux
> >> kernel versions with new binutils do?
>
> On Mon, Mar 14, 2011 at 1:02 PM, Andreas Schwab <[hidden email]> wrote:
> > The same that testers have to do in order to build old Linux kernel
> > versions with current versions of make.
>
> Are you saying it's OK to screw over binutils users because GNU Make
> did that too?

The claim is not even true.

While really old Linux versions wont build with fresh versions of Make, something
like v2.6.30, which is a 2 years old kernel, will build just fine, using latest
Make.

So lets stop coming up with all the wrong reasons to not fix this problem ...

Thanks,

        Ingo
Reply | Threaded
Open this post in threaded view
|

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

Ingo Molnar
In reply to this post by Andreas Schwab-4

* Andreas Schwab <[hidden email]> wrote:

> Pekka Enberg <[hidden email]> writes:
>
> > Hi Andreas,
> >
> > Pekka Enberg <[hidden email]> writes:
> >>> So what do you suggest that testers who want to, say, build old Linux
> >>> kernel versions with new binutils do?
> >
> > On Mon, Mar 14, 2011 at 1:02 PM, Andreas Schwab <[hidden email]> wrote:
> >> The same that testers have to do in order to build old Linux kernel
> >> versions with current versions of make.
> >
> > Are you saying it's OK to screw over binutils users because GNU Make
> > did that too?
>
> I'm just telling you the facts.

And you are wrong - latest Make does not break reasonably old kernel builds such as
v2.6.30.

Latest binutils insta-breaks the build over 130,000 commits, including the latest
released kernel.

Please change that .size build error to a build warning, to avoid this unnecessary
collateral damage.

Thanks,

        Ingo
Reply | Threaded
Open this post in threaded view
|

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

Jan Beulich
In reply to this post by Sedat Dilek
>>> On 14.03.11 at 12:52, Sedat Dilek <[hidden email]> wrote:
> On Mon, Mar 14, 2011 at 12:38 PM, Sedat Dilek
> <[hidden email]> wrote:
>> [ Changing Alan's Email to valid <[hidden email]> in CC ]
>>
>> On Mon, Mar 14, 2011 at 12:26 PM, Jan Beulich <[hidden email]> wrote:
>>>>>> On 14.03.11 at 12:02, Sedat Dilek <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

Ingo Molnar
In reply to this post by Alan Modra-3

* Alan Modra <[hidden email]> 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
12