[commit] Get rid of build warnings on xtensa-tdep.c

[commit] Get rid of build warnings on xtensa-tdep.c

[Maxim Grigoriev]

[-- Attachment #1: Type: text/plain, Size: 152 bytes --]

My recent updates made FSF GDB build unhappy, see
New ARI warning Fri Mar 11 01:54:56 UTC 2011.
This updates removes inline attributes from the code.



[-- Attachment #2: IW.diff --]
[-- Type: text/plain, Size: 2173 bytes --]

2011-03-10  Maxim Grigoriev  <maxim2405@gmail.com>

	* xtensa-tdep.c (windowing_enabled): Remove inline attribute.
	(xtensa_write_register, xtensa_read_register): Likewise.
	(xtensa_hextochar): Removed.
	(xtensa_init_reggroups): Replace xtensa_hextochar () by explicit code.

Index: gdb/xtensa-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/xtensa-tdep.c,v
retrieving revision 1.53
diff -u -r1.53 xtensa-tdep.c
--- gdb/xtensa-tdep.c	11 Mar 2011 00:21:42 -0000	1.53
+++ gdb/xtensa-tdep.c	11 Mar 2011 02:23:03 -0000
@@ -121,7 +121,7 @@
 #define PS_WOE			(1<<18)
 #define PS_EXC			(1<<4)
 
-static inline int
+static int
 windowing_enabled (struct gdbarch *gdbarch, unsigned int ps)
 {
   /* If we know CALL0 ABI is set explicitly,  say it is Call0.  */
@@ -162,7 +162,7 @@
 }
 
 /* Read Xtensa register directly from the hardware.  */ 
-static inline unsigned long
+static unsigned long
 xtensa_read_register (int regnum)
 {
   ULONGEST value;
@@ -172,7 +172,7 @@
 }
 
 /* Write Xtensa register directly to the hardware.  */ 
-static inline void
+static void
 xtensa_write_register (int regnum, ULONGEST value)
 {
   regcache_raw_write_unsigned (get_current_regcache (), regnum, value);
@@ -734,15 +734,6 @@
 		    _("invalid register number %d"), regnum);
 }
 
-/* Return a character representation of a hex-decimal digit.
-   The value of "xdigit" is assumed to be in a range [0..15].  */
-
-static inline
-char xtensa_hextochar (int xdigit)
-{
-  return '0' + xdigit;
-}
-
 static struct reggroup *xtensa_ar_reggroup;
 static struct reggroup *xtensa_user_reggroup;
 static struct reggroup *xtensa_vectra_reggroup;
@@ -760,7 +751,7 @@
 
   for (i = 0; i < XTENSA_MAX_COPROCESSOR; i++)
     {
-      cpname[2] = xtensa_hextochar (i);
+      cpname[2] = '0' + i;
       xtensa_cp[i] = reggroup_new (cpname, USER_REGGROUP);
     }
 }
@@ -1430,7 +1421,7 @@
 /* Report a problem with prologue analysis while doing backtracing.
    But, do it only once to avoid annoyng repeated messages.  */
 
-static inline void warning_once ()
+static void warning_once ()
 {
   if (xtensa_session_once_reported == 0)
     warning (_("\

Re: [commit] Get rid of build warnings on xtensa-tdep.c

[Joel Brobecker]

> 2011-03-10  Maxim Grigoriev  <maxim2405@gmail.com>
> 
> 	* xtensa-tdep.c (windowing_enabled): Remove inline attribute.
> 	(xtensa_write_register, xtensa_read_register): Likewise.
> 	(xtensa_hextochar): Removed.
> 	(xtensa_init_reggroups): Replace xtensa_hextochar () by explicit code.

Small comments, seen while scanning your patch...

> -static inline void warning_once ()
> +static void warning_once ()

The function name should be on the first column of the line.
This is to facilitate the search of the function implementation
(we can use: <<grep "^warning_once">> for instance). Also,
functions with no arguments should be declared with a "void"
parameter. Thus:

static void
warning_once (void)

-- 
Joel

[RFA] New rules for ARI (was [commit] Get rid of build warnings on xtensa-tdep.c)

[Pierre Muller]

  Reading Joel's comment, I thought
that the missing 'void' inside braces
should be a gdb_ari.sh output,
so I tried to write a new rule
to find out if functions that have no parameters
do use () instead of (void).

  While at it, I also tried to figure out if
there were calls to functions in first lines
(or prototype declarations with the function name at first line).

  I got 13 "function call in first line"
and 16 "no parameter function"

  I think that most are real errors in the sources:
$ grep -f list-no-param *lines
gdb/breakpoint.c:11955:all_tracepoints ()
gdb/f-exp.y:922:match_string_literal ()
gdb/f-exp.y:954:yylex ()
gdb/m2-exp.y:811:yylex ()
gdb/mep-tdep.c:848:current_me_module ()
gdb/mep-tdep.c:871:current_options ()
gdb/mep-tdep.c:888:current_cop_data_bus_width ()
gdb/mep-tdep.c:897:current_cr_names ()
gdb/mep-tdep.c:909:current_cr_is_float ()
gdb/mep-tdep.c:921:current_ccr_names ()
gdb/objc-exp.y:1221:yylex ()
gdb/p-exp.y:1123:yylex ()
gdb/python/py-evts.c:45:gdbpy_initialize_py_events ()
gdb/python/py-prettyprint.c:397:push_dummy_python_frame ()
gdb/remote.c:3115:send_interrupt_sequence ()
gdb/tracepoint.c:221:current_trace_status ()

$ grep -f list-first *lines
gdb/hppa-tdep.h:244:hppa_init_objfile_priv_data (struct objfile *objfile);
gdb/inferior.h:370:get_displaced_step_closure_by_addr (CORE_ADDR addr);
gdb/objc-exp.y:111:yyparse (void);
gdb/objc-exp.y:114:yylex (void);
gdb/objc-exp.y:117:yyerror (char *);
gdb/objc-exp.y:154:parse_number (char *, int, int, YYSTYPE *);
gdb/p-exp.y:121:yyerror (char *);
gdb/p-exp.y:158:parse_number (char *, int, int, YYSTYPE *);
gdb/python/py-stopevent.h:32:create_breakpoint_event_object (PyObject
*breakpoint);
gdb/python/py-stopevent.h:35:create_signal_event_object (enum target_signal
stop_signal);
gdb/solib-pa64.c:80:read_dynamic_info (asection *dyninfo_sect, dld_cache_t
*dld_cache_p);
gdb/varobj.c:300:varobj_add_child (struct varobj *var, const char *name,
struct value *value);
gdb/varobj.h:171:varobj_set_visualizer (struct varobj *var, const char
*visualizer);
  

OK to add these two new rules?

Pierre Muller, as ARI maintainer...

PS: Most of these ARI warnings should be easy to fix and
this would allow to promote these rule rapidly to ari_regression,
giving them a higher visibility if someone introduces
such code again later.


  
$ cvs diff -u -p gdb_ari.sh
Index: gdb_ari.sh
===================================================================
RCS file: /cvs/gdbadmin/ss/gdb_ari.sh,v
retrieving revision 1.105
diff -u -p -r1.105 gdb_ari.sh
--- gdb_ari.sh  29 Sep 2010 18:56:10 -0000      1.105
+++ gdb_ari.sh  11 Mar 2011 09:43:11 -0000
@@ -561,7 +561,27 @@ Function name starts lower case but has
     possible_FNR = FNR
     editCase_full_line = $0
 }
-
+
+# Only function implementation should be on first line
+BEGIN { doc["function call in first line"] = "\
+Function name in first line should be restricted to function
implementation"
+    category["function call in first line"] = ari_code
+}
+/^[a-z][a-z0-9_]*[[:space:]]*\((|[^*][^()]*)\)[[:space:]]*[^ \t]+/ {
+    fail("function call in first line")
+}
+
+
+# Functions without any parameter should have (void)
+# after their name not simply ().
+BEGIN { doc["no parameter function"] = "\
+Function having no parameter should be declared with funcname (void)."
+    category["no parameter function"] = ari_code
+}
+/^[a-zA-Z][a-z0-9A-Z_]*[[:space:]]*\(\)/ {
+    fail("no parameter function")
+}
+
 BEGIN { doc["hash"] = "\
 Do not use ` #...'\'', instead use `#...'\''(some compilers only correctly
\
 parse a C preprocessor directive when `#'\'' is the first character on \

Re: [RFA] New rules for ARI (was [commit] Get rid of build warnings on xtensa-tdep.c)

[Joel Brobecker]

> so I tried to write a new rule
> to find out if functions that have no parameters
> do use () instead of (void).

Thanks!

>   While at it, I also tried to figure out if
> there were calls to functions in first lines
> (or prototype declarations with the function name at first line).

I think you meant first column?

> OK to add these two new rules?

I think they would be useful - but be sure to s/line/column/ in
the code as well:

> +# Only function implementation should be on first line
> +BEGIN { doc["function call in first line"] = "\
> +Function name in first line should be restricted to function
> implementation"
> +    category["function call in first line"] = ari_code
> +}
> +/^[a-z][a-z0-9_]*[[:space:]]*\((|[^*][^()]*)\)[[:space:]]*[^ \t]+/ {
> +    fail("function call in first line")
> +}

-- 
Joel

RE: [RFA] New rules for ARI (was [commit] Get rid of build warnings on xtensa-tdep.c)

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Joel Brobecker
> Envoyé : vendredi 11 mars 2011 12:40
> À : Pierre Muller
> Cc : 'Maxim Grigoriev'; gdb-patches@sourceware.org
> Objet : Re: [RFA] New rules for ARI (was [commit] Get rid of build
> warnings on xtensa-tdep.c)
> 
> > so I tried to write a new rule
> > to find out if functions that have no parameters
> > do use () instead of (void).
> 
> Thanks!
> 
> >   While at it, I also tried to figure out if
> > there were calls to functions in first lines
> > (or prototype declarations with the function name at first line).
> 
> I think you meant first column?

  Whoops, of course, yes,
thanks for catching this...

 
> > OK to add these two new rules?
> 
> I think they would be useful - but be sure to s/line/column/ in
> the code as well:

  OK, I committed this to ss directory after
the substitution.

  New corresponding ARI warnings should appear
overnight.

Pierre

Re: [RFA] New rules for ARI

[Tom Tromey]

>>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr> writes:

Pierre>   Reading Joel's comment, I thought
Pierre> that the missing 'void' inside braces
Pierre> should be a gdb_ari.sh output,
Pierre> so I tried to write a new rule
Pierre> to find out if functions that have no parameters
Pierre> do use () instead of (void).

I wonder if -Wstrict-prototypes would catch this.

Tom

Re: [RFA] New rules for ARI

[Joel Brobecker]

> Pierre>   Reading Joel's comment, I thought
> Pierre> that the missing 'void' inside braces
> Pierre> should be a gdb_ari.sh output,
> Pierre> so I tried to write a new rule
> Pierre> to find out if functions that have no parameters
> Pierre> do use () instead of (void).
> 
> I wonder if -Wstrict-prototypes would catch this.

Actually, it would.  But when I tried to use it in our build,
I quickly hit a problem, because the headers we include might not
have strict prototypes.  For instance, readline defines:

    typedef int Function ();
    typedef void VFunction ();
    typedef char *CPFunction ();
    typedef char **CPPFunction ();

So, unless there is a way to say check strict-prototyping certain
files only, it's going to be tough to use :-(.

-- 
Joel

Re: [RFA] New rules for ARI

[Pedro Alves]

On Wednesday 16 March 2011 14:02:40, Joel Brobecker wrote:
> > Pierre>   Reading Joel's comment, I thought
> > Pierre> that the missing 'void' inside braces
> > Pierre> should be a gdb_ari.sh output,
> > Pierre> so I tried to write a new rule
> > Pierre> to find out if functions that have no parameters
> > Pierre> do use () instead of (void).
> > 
> > I wonder if -Wstrict-prototypes would catch this.
> 
> Actually, it would.  But when I tried to use it in our build,
> I quickly hit a problem, because the headers we include might not
> have strict prototypes.  For instance, readline defines:
> 
>     typedef int Function ();
>     typedef void VFunction ();
>     typedef char *CPFunction ();
>     typedef char **CPPFunction ();
> 
> So, unless there is a way to say check strict-prototyping certain
> files only, it's going to be tough to use :-(.

Maybe by including the in-tree readline headers with -isystem
rather than -I.  I sense that other cases in other libraries
will appear though, and that it'd bring in more pain than benefit.

-- 
Pedro Alves

Re: [RFA] New rules for ARI

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> Maybe by including the in-tree readline headers with -isystem
Pedro> rather than -I.  I sense that other cases in other libraries
Pedro> will appear though, and that it'd bring in more pain than benefit.

For free software libraries, we can at least report bugs and assume that
they will be fixed some day.

The bigger danger would be system libraries; but it would be good enough
if we could use this option on free operating systems.

Tom

Re: [RFA] New rules for ARI

[Pedro Alves]

On Wednesday 16 March 2011 15:03:15, Tom Tromey wrote:
> >>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:
> 
> Pedro> Maybe by including the in-tree readline headers with -isystem
> Pedro> rather than -I.  I sense that other cases in other libraries
> Pedro> will appear though, and that it'd bring in more pain than benefit.
> 
> For free software libraries, we can at least report bugs and assume that
> they will be fixed some day.
> 
> The bigger danger would be system libraries; but it would be good enough
> if we could use this option on free operating systems.

gcc is much more permissive with system libraries than with regular
headers.  -isystem PATHFOO makes gcc treat headers found in PATHFOO
as system headers.  Doesn't gcc ignore this warning on declarations
coming from system headers?  It's actually the opposite that happens:
bugs in system headers are more masked than non-system headers'.

-- 
Pedro Alves

Re: [RFA] New rules for ARI

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> gcc is much more permissive with system libraries than with regular
Pedro> headers.  -isystem PATHFOO makes gcc treat headers found in PATHFOO
Pedro> as system headers.  Doesn't gcc ignore this warning on declarations
Pedro> coming from system headers?  It's actually the opposite that happens:
Pedro> bugs in system headers are more masked than non-system headers'.

Yeah... I know this and yet somehow managed to write that email.
Sigh.

Anyway, I reported a bug to the readline maintainers.

I didn't run across any other problems with this flag, and I generally
do "full" (--enable-targets=all + expat + python) builds.  So aside from
some TUI things, that depend on readline anyway, we are in pretty good
shape.

Tom

Re: [RFA] New rules for ARI

[Pedro Alves]

On Wednesday 16 March 2011 15:43:11, Tom Tromey wrote:
> I didn't run across any other problems with this flag, and I generally
> do "full" (--enable-targets=all + expat + python) builds.  So aside from
> some TUI things, that depend on readline anyway, we are in pretty good
> shape.

But that was system expat and python, right?  Anyway, a grep
for "();" in expat and python headers in my system didn't show
anything.  (python2.6 showed a hit, but it's #if 0'ed out).

-- 
Pedro Alves

Re: [RFA] New rules for ARI

[Tom Tromey]

Pedro> But that was system expat and python, right?

Yes.

If readline is fixed, I'll enable it.  And if it causes problems, we can
deal with it then.  We can maybe make it conditional on using system
libraries or the like.

Tom

Re: [RFA] New rules for ARI

[Pedro Alves]

On Wednesday 16 March 2011 16:45:40, Tom Tromey wrote:
> If readline is fixed, I'll enable it.  And if it causes problems, we can
> deal with it then.  

Sure.  BTW, this is PR9878, which, apparently, I filed.  :-D

PR9877 is about -Wmissing-prototypes.  I wonder about turning
that on.

-- 
Pedro Alves

Re: [RFA] New rules for ARI

[Tom Tromey]

Pedro> PR9877 is about -Wmissing-prototypes.  I wonder about turning
Pedro> that on.

I gave it a try.  Some of the errors surprised me, so I think enabling
it would be a good idea.

I'm somewhat reluctant to enable it now, just before 7.3 branches.
Maybe after the branch?

Tom

Re: [RFA] New rules for ARI

[Pedro Alves]

On Wednesday 16 March 2011 17:10:55, Tom Tromey wrote:
> Pedro> PR9877 is about -Wmissing-prototypes.  I wonder about turning
> Pedro> that on.
> 
> I gave it a try.  Some of the errors surprised me, so I think enabling
> it would be a good idea.
> 
> I'm somewhat reluctant to enable it now, just before 7.3 branches.
> Maybe after the branch?

Yeah, agreed.

-- 
Pedro Alves

Re: [RFA] New rules for ARI

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> Actually, it would.  But when I tried to use it in our build,
Joel> I quickly hit a problem, because the headers we include might not
Joel> have strict prototypes.  For instance, readline defines:

Joel>     typedef int Function ();
Joel>     typedef void VFunction ();
Joel>     typedef char *CPFunction ();
Joel>     typedef char **CPPFunction ();

Joel> So, unless there is a way to say check strict-prototyping certain
Joel> files only, it's going to be tough to use :-(.

We can work around that by defining _FUNCTION_DEF and also including
readline/config.h.  But, it is a mess :-(.  For one thing, the new
standalone readline doesn't seem to install readline/config.h, and
nothing else defines PREFER_STDARG, so the distro case would be broken.

I did this as a hack though and it worked ok-ish in tree.  It found some
mildly questionable TUI code and also a few more minor cases that I will
fix.

Tom

[commit] Correct style issues in xtensa-tdep.c

[Maxim Grigoriev]

[-- Attachment #1: Type: text/plain, Size: 918 bytes --]

I am surprised I missed it. Thanks for noticing this.
This patch corrects the style violation mentioned by Joel.

On 03/10/2011 10:45 PM, Joel Brobecker wrote:
>> 2011-03-10  Maxim Grigoriev<maxim2405@gmail.com>
>>
>> 	* xtensa-tdep.c (windowing_enabled): Remove inline attribute.
>> 	(xtensa_write_register, xtensa_read_register): Likewise.
>> 	(xtensa_hextochar): Removed.
>> 	(xtensa_init_reggroups): Replace xtensa_hextochar () by explicit code.
>>      
> Small comments, seen while scanning your patch...
>
>    
>> -static inline void warning_once ()
>> +static void warning_once ()
>>      
> The function name should be on the first column of the line.
> This is to facilitate the search of the function implementation
> (we can use:<<grep "^warning_once">>  for instance). Also,
> functions with no arguments should be declared with a "void"
> parameter. Thus:
>
> static void
> warning_once (void)
>
>    


[-- Attachment #2: Style.diff --]
[-- Type: text/plain, Size: 686 bytes --]

2011-03-11  Maxim Grigoriev  <maxim2405@gmail.com>

	* xtensa-tdep.c (warning_once): Correct style issues.

Index: gdb/xtensa-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/xtensa-tdep.c,v
retrieving revision 1.54
diff -u -r1.54 xtensa-tdep.c
--- gdb/xtensa-tdep.c	11 Mar 2011 02:32:31 -0000	1.54
+++ gdb/xtensa-tdep.c	11 Mar 2011 18:25:19 -0000
@@ -1421,7 +1421,8 @@
 /* Report a problem with prologue analysis while doing backtracing.
    But, do it only once to avoid annoyng repeated messages.  */
 
-static void warning_once ()
+static void
+warning_once (void)
 {
   if (xtensa_session_once_reported == 0)
     warning (_("\