public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed
* PATCH: Add --size-check=[error|warning]
@ 2011-03-11 16:58 H.J. Lu
  2011-03-11 17:04 ` Jan Beulich
  2011-03-16 23:56 ` Alan Modra
  0 siblings, 2 replies; 24+ messages in thread
From: H.J. Lu @ 2011-03-11 16:58 UTC (permalink / raw)
  To: binutils; +Cc: amodra

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  <hongjiu.lu@intel.com>
+
+	* 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  <hongjiu.lu@intel.com>
 
 	* 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  <hongjiu.lu@intel.com>
+
+	* 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  <hongjiu.lu@intel.com>
 
 	* 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.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: PATCH: Add --size-check=[error|warning]
  2011-03-11 16:58 PATCH: Add --size-check=[error|warning] H.J. Lu
@ 2011-03-11 17:04 ` Jan Beulich
  2011-03-11 17:20   ` H.J. Lu
  2011-03-16 23:56 ` Alan Modra
  1 sibling, 1 reply; 24+ messages in thread
From: Jan Beulich @ 2011-03-11 17:04 UTC (permalink / raw)
  To: H.J. Lu; +Cc: amodra, binutils

>>> On 11.03.11 at 17:58, "H.J. Lu" <hongjiu.lu@intel.com> 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  <hongjiu.lu@intel.com>
> +
> +	* 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  <hongjiu.lu@intel.com>
>  
>  	* 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  <hongjiu.lu@intel.com>
> +
> +	* 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  <hongjiu.lu@intel.com>
>  
>  	* 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.


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: PATCH: Add --size-check=[error|warning]
  2011-03-11 17:04 ` Jan Beulich
@ 2011-03-11 17:20   ` H.J. Lu
  2011-03-14  8:44     ` Jan Beulich
  0 siblings, 1 reply; 24+ messages in thread
From: H.J. Lu @ 2011-03-11 17:20 UTC (permalink / raw)
  To: Jan Beulich; +Cc: amodra, binutils

On Fri, Mar 11, 2011 at 9:05 AM, Jan Beulich <JBeulich@novell.com> wrote:
>>>> On 11.03.11 at 17:58, "H.J. Lu" <hongjiu.lu@intel.com> 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.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: PATCH: Add --size-check=[error|warning]
  2011-03-11 17:20   ` H.J. Lu
@ 2011-03-14  8:44     ` Jan Beulich
  2011-03-14  9:55       ` Ingo Molnar
  2011-03-14 13:55       ` PATCH: Add --size-check=[error|warning] H.J. Lu
  0 siblings, 2 replies; 24+ messages in thread
From: Jan Beulich @ 2011-03-14  8:44 UTC (permalink / raw)
  To: H.J. Lu; +Cc: mingo, amodra, binutils

>>> On 11.03.11 at 18:19, "H.J. Lu" <hjl.tools@gmail.com> wrote:
> On Fri, Mar 11, 2011 at 9:05 AM, Jan Beulich <JBeulich@novell.com> wrote:
>>>>> On 11.03.11 at 17:58, "H.J. Lu" <hongjiu.lu@intel.com> 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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: PATCH: Add --size-check=[error|warning]
  2011-03-14  8:44     ` Jan Beulich
@ 2011-03-14  9:55       ` Ingo Molnar
  2011-03-14 10:41         ` Alan Modra
  2011-03-14 13:55       ` PATCH: Add --size-check=[error|warning] H.J. Lu
  1 sibling, 1 reply; 24+ messages in thread
From: Ingo Molnar @ 2011-03-14  9:55 UTC (permalink / raw)
  To: Jan Beulich
  Cc: H.J. Lu, amodra, binutils, linux-kernel, H. Peter Anvin,
	Andrew Morton, Linus Torvalds, Thomas Gleixner


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

* Jan Beulich <JBeulich@novell.com> 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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: PATCH: Add --size-check=[error|warning]
  2011-03-14  9:55       ` Ingo Molnar
@ 2011-03-14 10:41         ` Alan Modra
  2011-03-14 10:50           ` Pekka Enberg
                             ` (3 more replies)
  0 siblings, 4 replies; 24+ messages in thread
From: Alan Modra @ 2011-03-14 10:41 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Jan Beulich, H.J. Lu, binutils, linux-kernel, H. Peter Anvin,
	Andrew Morton, Linus Torvalds, Thomas Gleixner

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: PATCH: Add --size-check=[error|warning]
  2011-03-14 10:41         ` Alan Modra
@ 2011-03-14 10:50           ` Pekka Enberg
  2011-03-14 18:04             ` Alan Cox
  2011-03-14 10:52           ` Jan Beulich
                             ` (2 subsequent siblings)
  3 siblings, 1 reply; 24+ messages in thread
From: Pekka Enberg @ 2011-03-14 10:50 UTC (permalink / raw)
  To: Ingo Molnar, Jan Beulich, H.J. Lu, binutils, linux-kernel,
	H. Peter Anvin, Andrew Morton, Linus Torvalds, Thomas Gleixner
  Cc: Alan Modra

Hi Alan,

On Mon, Mar 14, 2011 at 12:41 PM, Alan Modra <amodra@gmail.com> 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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: PATCH: Add --size-check=[error|warning]
  2011-03-14 10:41         ` Alan Modra
  2011-03-14 10:50           ` Pekka Enberg
@ 2011-03-14 10:52           ` Jan Beulich
       [not found]           ` <AANLkTi=3AiLaw5Gnis8Ha4eRXirk0s-Cnk2zzN12YDpH__45869.1711457961$1300099861$gmane$org@mail.gmail.com>
  2011-03-14 12:23           ` Ingo Molnar
  3 siblings, 0 replies; 24+ messages in thread
From: Jan Beulich @ 2011-03-14 10:52 UTC (permalink / raw)
  To: Ingo Molnar, Alan Modra
  Cc: H.J. Lu, Thomas Gleixner, Andrew Morton, Linus Torvalds,
	binutils, linux-kernel, H. Peter Anvin

>>> On 14.03.11 at 11:41, Alan Modra <amodra@gmail.com> 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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: PATCH: Add --size-check=[error|warning]
       [not found]           ` <AANLkTi=3AiLaw5Gnis8Ha4eRXirk0s-Cnk2zzN12YDpH__45869.1711457961$1300099861$gmane$org@mail.gmail.com>
@ 2011-03-14 11:03             ` Andreas Schwab
  2011-03-14 11:12               ` Pekka Enberg
  0 siblings, 1 reply; 24+ messages in thread
From: Andreas Schwab @ 2011-03-14 11:03 UTC (permalink / raw)
  To: Pekka Enberg
  Cc: Ingo Molnar, Jan Beulich, H.J. Lu, binutils, linux-kernel,
	H. Peter Anvin, Alan Modra

Pekka Enberg <penberg@kernel.org> 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, schwab@redhat.com
GPG Key fingerprint = D4E8 DBE3 3813 BB5D FA84  5EC7 45C6 250E 6F00 984E
"And now for something completely different."

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: PATCH: Add --size-check=[error|warning]
  2011-03-14 11:03             ` Andreas Schwab
@ 2011-03-14 11:12               ` Pekka Enberg
  2011-03-14 12:02                 ` Andreas Schwab
  2011-03-14 12:11                 ` Ingo Molnar
  0 siblings, 2 replies; 24+ messages in thread
From: Pekka Enberg @ 2011-03-14 11:12 UTC (permalink / raw)
  To: Andreas Schwab
  Cc: Ingo Molnar, Jan Beulich, H.J. Lu, binutils, linux-kernel,
	H. Peter Anvin, Alan Modra

Hi Andreas,

Pekka Enberg <penberg@kernel.org> 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 <schwab@redhat.com> 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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: PATCH: Add --size-check=[error|warning]
  2011-03-14 11:12               ` Pekka Enberg
@ 2011-03-14 12:02                 ` Andreas Schwab
  2011-03-14 12:13                   ` Ingo Molnar
  2011-03-14 12:11                 ` Ingo Molnar
  1 sibling, 1 reply; 24+ messages in thread
From: Andreas Schwab @ 2011-03-14 12:02 UTC (permalink / raw)
  To: Pekka Enberg
  Cc: Ingo Molnar, Jan Beulich, H.J. Lu, binutils, linux-kernel,
	H. Peter Anvin, Alan Modra

Pekka Enberg <penberg@kernel.org> writes:

> Hi Andreas,
>
> Pekka Enberg <penberg@kernel.org> 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 <schwab@redhat.com> 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, schwab@redhat.com
GPG Key fingerprint = D4E8 DBE3 3813 BB5D FA84  5EC7 45C6 250E 6F00 984E
"And now for something completely different."

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: PATCH: Add --size-check=[error|warning]
  2011-03-14 11:12               ` Pekka Enberg
  2011-03-14 12:02                 ` Andreas Schwab
@ 2011-03-14 12:11                 ` Ingo Molnar
  1 sibling, 0 replies; 24+ messages in thread
From: Ingo Molnar @ 2011-03-14 12:11 UTC (permalink / raw)
  To: Pekka Enberg
  Cc: Andreas Schwab, Jan Beulich, H.J. Lu, binutils, linux-kernel,
	H. Peter Anvin, Alan Modra


* Pekka Enberg <penberg@kernel.org> wrote:

> Hi Andreas,
> 
> Pekka Enberg <penberg@kernel.org> 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 <schwab@redhat.com> 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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: PATCH: Add --size-check=[error|warning]
  2011-03-14 12:02                 ` Andreas Schwab
@ 2011-03-14 12:13                   ` Ingo Molnar
  2011-03-14 12:55                     ` Andreas Schwab
  0 siblings, 1 reply; 24+ messages in thread
From: Ingo Molnar @ 2011-03-14 12:13 UTC (permalink / raw)
  To: Andreas Schwab
  Cc: Pekka Enberg, Jan Beulich, H.J. Lu, binutils, linux-kernel,
	H. Peter Anvin, Alan Modra


* Andreas Schwab <schwab@redhat.com> wrote:

> Pekka Enberg <penberg@kernel.org> writes:
> 
> > Hi Andreas,
> >
> > Pekka Enberg <penberg@kernel.org> 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 <schwab@redhat.com> 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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: PATCH: Add --size-check=[error|warning]
  2011-03-14 10:41         ` Alan Modra
                             ` (2 preceding siblings ...)
       [not found]           ` <AANLkTi=3AiLaw5Gnis8Ha4eRXirk0s-Cnk2zzN12YDpH__45869.1711457961$1300099861$gmane$org@mail.gmail.com>
@ 2011-03-14 12:23           ` Ingo Molnar
  2011-03-14 12:26             ` Ingo Molnar
  2011-03-14 13:16             ` git bisect plus fixes (was: PATCH: Add --size-check=[error|warning]) Ralf Wildenhues
  3 siblings, 2 replies; 24+ messages in thread
From: Ingo Molnar @ 2011-03-14 12:23 UTC (permalink / raw)
  To: Jan Beulich, H.J. Lu, binutils, linux-kernel, H. Peter Anvin,
	Andrew Morton, Linus Torvalds, Thomas Gleixner


* Alan Modra <amodra@gmail.com> 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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: PATCH: Add --size-check=[error|warning]
  2011-03-14 12:23           ` Ingo Molnar
@ 2011-03-14 12:26             ` Ingo Molnar
  2011-03-14 13:16             ` git bisect plus fixes (was: PATCH: Add --size-check=[error|warning]) Ralf Wildenhues
  1 sibling, 0 replies; 24+ messages in thread
From: Ingo Molnar @ 2011-03-14 12:26 UTC (permalink / raw)
  To: Alan Modra
  Cc: Jan Beulich, H.J. Lu, binutils, linux-kernel, H. Peter Anvin,
	Andrew Morton, Linus Torvalds, Thomas Gleixner


(resend, fixed the To line)

* Alan Modra <amodra@gmail.com> 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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: PATCH: Add --size-check=[error|warning]
  2011-03-14 12:13                   ` Ingo Molnar
@ 2011-03-14 12:55                     ` Andreas Schwab
  2011-03-14 13:17                       ` Ingo Molnar
  0 siblings, 1 reply; 24+ messages in thread
From: Andreas Schwab @ 2011-03-14 12:55 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Pekka Enberg, Jan Beulich, H.J. Lu, binutils, linux-kernel,
	H. Peter Anvin, Alan Modra

Ingo Molnar <mingo@elte.hu> writes:

> * Andreas Schwab <schwab@redhat.com> wrote:
>
>> Pekka Enberg <penberg@kernel.org> writes:
>> 
>> > Hi Andreas,
>> >
>> > Pekka Enberg <penberg@kernel.org> 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 <schwab@redhat.com> 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

This is just ridiculous.  You are defining away facts just because they
don't fit your view.

> latest Make does not break reasonably old kernel builds such as
> v2.6.30.

$ git tag --contains 3c955b4 | head -1
v2.6.36

Andreas.

-- 
Andreas Schwab, schwab@redhat.com
GPG Key fingerprint = D4E8 DBE3 3813 BB5D FA84  5EC7 45C6 250E 6F00 984E
"And now for something completely different."

^ permalink raw reply	[flat|nested] 24+ messages in thread

* git bisect plus fixes (was: PATCH: Add --size-check=[error|warning])
  2011-03-14 12:23           ` Ingo Molnar
  2011-03-14 12:26             ` Ingo Molnar
@ 2011-03-14 13:16             ` Ralf Wildenhues
  1 sibling, 0 replies; 24+ messages in thread
From: Ralf Wildenhues @ 2011-03-14 13:16 UTC (permalink / raw)
  To: Ingo Molnar, git
  Cc: Jan Beulich, H.J. Lu, binutils, linux-kernel, H. Peter Anvin,
	Andrew Morton, Linus Torvalds, Thomas Gleixner

[ 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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: PATCH: Add --size-check=[error|warning]
  2011-03-14 12:55                     ` Andreas Schwab
@ 2011-03-14 13:17                       ` Ingo Molnar
  2011-03-14 14:11                         ` Andreas Schwab
  0 siblings, 1 reply; 24+ messages in thread
From: Ingo Molnar @ 2011-03-14 13:17 UTC (permalink / raw)
  To: Andreas Schwab
  Cc: Pekka Enberg, Jan Beulich, H.J. Lu, binutils, linux-kernel,
	H. Peter Anvin, Alan Modra


* Andreas Schwab <schwab@redhat.com> wrote:

> Ingo Molnar <mingo@elte.hu> writes:
> 
> > * Andreas Schwab <schwab@redhat.com> wrote:
> >
> >> Pekka Enberg <penberg@kernel.org> writes:
> >> 
> >> > Hi Andreas,
> >> >
> >> > Pekka Enberg <penberg@kernel.org> 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 <schwab@redhat.com> 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.
> 
> This is just ridiculous.
> You are defining away facts just because they don't fit your view.

No, i actually tried out the latest released Make version (3.82) and it still builds 
v2.6.30 fine:

    LD      arch/x86/boot/setup.elf
    OBJCOPY arch/x86/boot/setup.bin
    BUILD   arch/x86/boot/bzImage
  Root device is (8, 1)
  Setup is 12364 bytes (padded to 12800 bytes). 
  System is 3730 kB
  CRC 77bb7f2d
  Kernel: arch/x86/boot/bzImage is ready  (#105830)

The kernel build count is at 105830 because i build and test a lot of kernels on 
this box.

And the resulting kernel boots fine on a testbox:

  mercury:~> uname -a
  Linux mercury 2.6.30 #105830 SMP Mon Mar 14 12:29:06 CET 2011 x86_64 x86_64 x86_64 GNU/Linux

I have built and booted over half a million Linux kernels in the past 3-4 years so i 
generally have quite a bit of experience when it comes to build environment bugs and 
workflow problems. Breaking the build retroactively and unnecessarily like here is 
one of the worst things that can be done to testing quality and efficiency.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: PATCH: Add --size-check=[error|warning]
  2011-03-14  8:44     ` Jan Beulich
  2011-03-14  9:55       ` Ingo Molnar
@ 2011-03-14 13:55       ` H.J. Lu
  1 sibling, 0 replies; 24+ messages in thread
From: H.J. Lu @ 2011-03-14 13:55 UTC (permalink / raw)
  To: Jan Beulich; +Cc: mingo, amodra, binutils

On Mon, Mar 14, 2011 at 1:44 AM, Jan Beulich <JBeulich@novell.com> wrote:
>>>> On 11.03.11 at 18:19, "H.J. Lu" <hjl.tools@gmail.com> wrote:
>> On Fri, Mar 11, 2011 at 9:05 AM, Jan Beulich <JBeulich@novell.com> wrote:
>>>>>> On 11.03.11 at 17:58, "H.J. Lu" <hongjiu.lu@intel.com> 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.

I can provide a wrapper if needed.

> 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.
>

Just put the wrapper in the front of PATH.


-- 
H.J.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: PATCH: Add --size-check=[error|warning]
  2011-03-14 13:17                       ` Ingo Molnar
@ 2011-03-14 14:11                         ` Andreas Schwab
  0 siblings, 0 replies; 24+ messages in thread
From: Andreas Schwab @ 2011-03-14 14:11 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Pekka Enberg, Jan Beulich, H.J. Lu, binutils, linux-kernel,
	H. Peter Anvin, Alan Modra

Ingo Molnar <mingo@elte.hu> writes:

> * Andreas Schwab <schwab@redhat.com> wrote:
>
>> Ingo Molnar <mingo@elte.hu> writes:
>> 
>> > * Andreas Schwab <schwab@redhat.com> wrote:
>> >
>> >> Pekka Enberg <penberg@kernel.org> writes:
>> >> 
>> >> > Hi Andreas,
>> >> >
>> >> > Pekka Enberg <penberg@kernel.org> 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 <schwab@redhat.com> 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.
>> 
>> This is just ridiculous.
>> You are defining away facts just because they don't fit your view.
>
> No, i actually tried out the latest released Make version (3.82) and it still builds 
> v2.6.30 fine:

That's a very convincing argument.

Andreas.

-- 
Andreas Schwab, schwab@redhat.com
GPG Key fingerprint = D4E8 DBE3 3813 BB5D FA84  5EC7 45C6 250E 6F00 984E
"And now for something completely different."

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: PATCH: Add --size-check=[error|warning]
  2011-03-14 10:50           ` Pekka Enberg
@ 2011-03-14 18:04             ` Alan Cox
  0 siblings, 0 replies; 24+ messages in thread
From: Alan Cox @ 2011-03-14 18:04 UTC (permalink / raw)
  To: Pekka Enberg
  Cc: Ingo Molnar, Jan Beulich, H.J. Lu, binutils, linux-kernel,
	H. Peter Anvin, Andrew Morton, Linus Torvalds, Thomas Gleixner,
	Alan Modra

> > 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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: PATCH: Add --size-check=[error|warning]
  2011-03-11 16:58 PATCH: Add --size-check=[error|warning] H.J. Lu
  2011-03-11 17:04 ` Jan Beulich
@ 2011-03-16 23:56 ` Alan Modra
  2011-03-18 11:20   ` Alan Modra
  1 sibling, 1 reply; 24+ messages in thread
From: Alan Modra @ 2011-03-16 23:56 UTC (permalink / raw)
  To: H.J. Lu; +Cc: binutils

On Fri, Mar 11, 2011 at 08:58:02AM -0800, H.J. Lu wrote:
> This patch adds
> --size-check=[error|warning] so that we can issue a warning instead of
> an error.  OK to install?

I guess this had better go in.  Please apply to 2.21 branch also.
Oh, another thing that just occurred to me:  Reporting the .size
expression symbols is likely not as useful as reporting the function
symbol, and it's actually a lot easier to report the function..

-- 
Alan Modra
Australia Development Lab, IBM

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: PATCH: Add --size-check=[error|warning]
  2011-03-16 23:56 ` Alan Modra
@ 2011-03-18 11:20   ` Alan Modra
  2011-03-18 11:59     ` Alan Modra
  0 siblings, 1 reply; 24+ messages in thread
From: Alan Modra @ 2011-03-18 11:20 UTC (permalink / raw)
  To: H.J. Lu, binutils

On Thu, Mar 17, 2011 at 10:26:06AM +1030, Alan Modra wrote:
> Oh, another thing that just occurred to me:  Reporting the .size
> expression symbols is likely not as useful as reporting the function
> symbol, and it's actually a lot easier to report the function..

Like this.

	* config/obj-elf.c (elf_frob_symbol): Report S_SET_SIZE symbol
	on .size expression errors rather than ones in the size expression.

Index: gas/config/obj-elf.c
===================================================================
RCS file: /cvs/src/src/gas/config/obj-elf.c,v
retrieving revision 1.139
diff -u -p -r1.139 obj-elf.c
--- gas/config/obj-elf.c	16 Mar 2011 12:58:26 -0000	1.139
+++ gas/config/obj-elf.c	18 Mar 2011 02:50:30 -0000
@@ -1896,49 +1901,12 @@ elf_frob_symbol (symbolS *symp, int *pun
 	S_SET_SIZE (symp, size->X_add_number);
       else
 	{
-	  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;
+	    as_bad (_(".size expression for %s "
+		      "does not evaluate to a constant"), S_GET_NAME (symp));
 	  else
-	    as_error = as_warn;
-
-	  if (size->X_op == O_subtract)
-	    {
-	      op_name = S_GET_NAME (size->X_op_symbol);
-	      add_name = S_GET_NAME (size->X_add_symbol);
-	      if (strcmp (op_name, FAKE_LABEL_NAME) == 0)
-		op_name = NULL;
-	      if (strcmp (add_name, FAKE_LABEL_NAME) == 0)
-		add_name = NULL;
-
-	      if (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;
-
-		  if (op_name)
-		    name = op_name;
-		  else if (add_name)
-		    name = add_name;
-		  else
-		    name = NULL;
-
-		  if (name)
-		    as_error (_(".size expression with symbol `%s' "
-				"does not evaluate to a constant"),
-			      name);
-		}
-	    }
-	  
-	  if (!op_name && !add_name)
-	    as_error (_(".size expression does not evaluate to a "
-			"constant"));
+	    as_warn (_(".size expression for %s "
+		       "does not evaluate to a constant"), S_GET_NAME (symp));
 	}
       free (sy_obj->size);
       sy_obj->size = NULL;

-- 
Alan Modra
Australia Development Lab, IBM

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: PATCH: Add --size-check=[error|warning]
  2011-03-18 11:20   ` Alan Modra
@ 2011-03-18 11:59     ` Alan Modra
  0 siblings, 0 replies; 24+ messages in thread
From: Alan Modra @ 2011-03-18 11:59 UTC (permalink / raw)
  To: H.J. Lu, binutils

And for 2.21.

	* config/obj-elf.c (elf_frob_symbol): Report S_SET_SIZE symbol
	on .size expression errors rather than symbols in the size expression.

	Backport 2011-03-16  H.J. Lu  <hongjiu.lu@intel.com>
	* 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=.

Index: gas/as.c
===================================================================
RCS file: /cvs/src/src/gas/as.c,v
retrieving revision 1.92.2.1
diff -u -p -r1.92.2.1 as.c
--- gas/as.c	1 Feb 2011 12:25:40 -0000	1.92.2.1
+++ gas/as.c	18 Mar 2011 11:49:05 -0000
@@ -1,6 +1,7 @@
 /* as.c - GAS main program.
    Copyright 1987, 1990, 1991, 1992, 1993, 1994, 1995, 1996, 1997, 1998,
-   1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010
+   1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009,
+   2010, 2011
    Free Software Foundation, Inc.
 
    This file is part of GAS, the GNU Assembler.
@@ -283,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"));
@@ -442,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,
@@ -475,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}
@@ -812,6 +818,15 @@ This program has absolutely no warranty.
 	  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;
Index: gas/as.h
===================================================================
RCS file: /cvs/src/src/gas/as.h,v
retrieving revision 1.68
diff -u -p -r1.68 as.h
--- gas/as.h	3 Jul 2010 20:52:24 -0000	1.68
+++ gas/as.h	18 Mar 2011 11:49:06 -0000
@@ -1,7 +1,7 @@
 /* as.h - global header file
    Copyright 1987, 1990, 1991, 1992, 1993, 1994, 1995, 1996, 1997, 1998,
-   1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009
-   Free Software Foundation, Inc.
+   1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010
+   2011 Free Software Foundation, Inc.
 
    This file is part of GAS, the GNU Assembler.
 
@@ -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
Index: gas/config/obj-elf.c
===================================================================
RCS file: /cvs/src/src/gas/config/obj-elf.c,v
retrieving revision 1.133.2.4
diff -u -p -r1.133.2.4 obj-elf.c
--- gas/config/obj-elf.c	25 Feb 2011 13:46:36 -0000	1.133.2.4
+++ gas/config/obj-elf.c	18 Mar 2011 11:49:08 -0000
@@ -1893,7 +1893,14 @@ elf_frob_symbol (symbolS *symp, int *pun
 	  && sy_obj->size->X_op == O_constant)
 	S_SET_SIZE (symp, sy_obj->size->X_add_number);
       else
-	as_bad (_(".size expression does not evaluate to a constant"));
+	{
+	  if (flag_size_check == size_check_error)
+	    as_bad (_(".size expression for %s "
+		      "does not evaluate to a constant"), S_GET_NAME (symp));
+	  else
+	    as_warn (_(".size expression for %s "
+		       "does not evaluate to a constant"), S_GET_NAME (symp));
+	}
       free (sy_obj->size);
       sy_obj->size = NULL;
     }
Index: gas/doc/as.texinfo
===================================================================
RCS file: /cvs/src/src/gas/doc/as.texinfo,v
retrieving revision 1.225
diff -u -p -r1.225 as.texinfo
--- gas/doc/as.texinfo	2 Nov 2010 14:36:36 -0000	1.225
+++ gas/doc/as.texinfo	18 Mar 2011 11:49:13 -0000
@@ -1,6 +1,6 @@
 \input texinfo @c                               -*-Texinfo-*-
 @c  Copyright 1991, 1992, 1993, 1994, 1995, 1996, 1997, 1998, 1999, 2000,
-@c  2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010
+@c  2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011
 @c  Free Software Foundation, Inc.
 @c UPDATE!!  On future updates--
 @c   (1)   check for new machine-dep cmdline options in
@@ -103,7 +103,8 @@ This file documents the GNU Assembler "@
 
 @c man begin COPYRIGHT
 Copyright @copyright{} 1991, 1992, 1993, 1994, 1995, 1996, 1997, 1998, 1999,
-2000, 2001, 2002, 2006, 2007, 2008, 2009, 2010 Free Software Foundation, Inc.
+2000, 2001, 2002, 2006, 2007, 2008, 2009, 2010, 2011 Free Software Foundation,
+Inc.
 
 Permission is granted to copy, distribute and/or modify this document
 under the terms of the GNU Free Documentation License, Version 1.3
@@ -153,7 +154,8 @@ done.
 
 @vskip 0pt plus 1filll
 Copyright @copyright{} 1991, 1992, 1993, 1994, 1995, 1996, 1997, 1998, 1999,
-2000, 2001, 2002, 2006, 2007, 2008, 2009, 2010 Free Software Foundation, Inc.
+2000, 2001, 2002, 2006, 2007, 2008, 2009, 2010, 2011 Free Software Foundation,
+Inc.
 
       Permission is granted to copy, distribute and/or modify this document
       under the terms of the GNU Free Documentation License, Version 1.3
@@ -241,6 +243,7 @@ gcc(1), ld(1), and the Info entries for 
  @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
@@ -602,6 +610,10 @@ Generate DWARF2 debugging information fo
 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.
 

-- 
Alan Modra
Australia Development Lab, IBM

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2011-03-18 11:59 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-03-11 16:58 PATCH: Add --size-check=[error|warning] H.J. Lu
2011-03-11 17:04 ` Jan Beulich
2011-03-11 17:20   ` H.J. Lu
2011-03-14  8:44     ` Jan Beulich
2011-03-14  9:55       ` Ingo Molnar
2011-03-14 10:41         ` Alan Modra
2011-03-14 10:50           ` Pekka Enberg
2011-03-14 18:04             ` Alan Cox
2011-03-14 10:52           ` Jan Beulich
     [not found]           ` <AANLkTi=3AiLaw5Gnis8Ha4eRXirk0s-Cnk2zzN12YDpH__45869.1711457961$1300099861$gmane$org@mail.gmail.com>
2011-03-14 11:03             ` Andreas Schwab
2011-03-14 11:12               ` Pekka Enberg
2011-03-14 12:02                 ` Andreas Schwab
2011-03-14 12:13                   ` Ingo Molnar
2011-03-14 12:55                     ` Andreas Schwab
2011-03-14 13:17                       ` Ingo Molnar
2011-03-14 14:11                         ` Andreas Schwab
2011-03-14 12:11                 ` Ingo Molnar
2011-03-14 12:23           ` Ingo Molnar
2011-03-14 12:26             ` Ingo Molnar
2011-03-14 13:16             ` git bisect plus fixes (was: PATCH: Add --size-check=[error|warning]) Ralf Wildenhues
2011-03-14 13:55       ` PATCH: Add --size-check=[error|warning] H.J. Lu
2011-03-16 23:56 ` Alan Modra
2011-03-18 11:20   ` Alan Modra
2011-03-18 11:59     ` Alan Modra

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).