Fix -Wundef for FEATURE_INDEX_1.
diff mbox

Message ID 535F6BDE.3050801@redhat.com
State New
Headers show

Commit Message

Carlos O'Donell April 29, 2014, 9:07 a.m. UTC
While reviewing some code for Siddhesh and waiting for a build I had
time to do another -Wundef fix.

This is the simplest patch I can come up with to fix FEATURE_INDEX_1
-Wundef warnings. The constant definition is duplicated because we
can't use the enum constant in assembly nor in the other macros.

The bright side is that we end up with:
#ifdef __ASSEMBLER__
... Everything we need for assembly.
#endif
... Everything we need for non-assembly.
#endif

The down side is the slight duplication. I considered adding another
macro e.g.
#define __FEATURE_INDEX_1 0
...
# define FEATURE_INDEX_1 __FEATURE_INDEX_1
...
However, that just seemed ugly, so I left the duplication.

Any objections?

2014-04-29  Carlos O'Donell  <carlos@redhat.com>

	* sysdeps/x86_64/multiarch/init-arch.h [__ASSEMBLER__]:
	Define FEATURE_INDEX_1 to 0.
	[!__ASSEMBLER__]: Likewise.

---

Cheers,
Carlos.

Comments

Roland McGrath April 30, 2014, 5:55 p.m. UTC | #1
> This is the simplest patch I can come up with to fix FEATURE_INDEX_1
> -Wundef warnings. The constant definition is duplicated because we
> can't use the enum constant in assembly nor in the other macros.

What does "nor in the other macros" mean?  Of course a macro can be
based on arithmetic plus use of another constant that is usable in the
contexts where the macro is used.  enum constants cannot be used in
assembly, nor in #if contexts.  Did you mean to say that the other
macros are used in #if contexts and therefore must not use enum
constants?

> The down side is the slight duplication. I considered adding another
> macro e.g.
> #define __FEATURE_INDEX_1 0
> ...
> # define FEATURE_INDEX_1 __FEATURE_INDEX_1
> ...
> However, that just seemed ugly, so I left the duplication.

When "duplication" concretely means two instances of one macro whose
value is zero, then it seems less ugly than many other things.  But in
the general case, duplication is worse than most other things.

In this situation, the question is, what are we getting from
FEATURE_INDEX_* being enum constants rather than macros at all?

This is internal-only code, so ease of maintenance is really the only
issue that we care much about.  The general reason to have enum
constants for things is so that you can use them in the debugger rather
than either looking up values in the source by hand or relying on -g3
macro info.  That is far less useful than the general case when its an
anonymous enum, so there is no type to which you can cast an integer
value to get the symbolic name trivially in gdb.  Hence, literally the
only benefit is typing FEATURE_INDEX_1 in gdb.  If we cared about that,
we'd make all the macros in init-arch.h be enum constants.  So I think
we just aren't getting anything worthwhile from having it be an enum.
Let's eliminate the duplication by just having one unconditional #define.


Thanks,
Roland

Patch
diff mbox

diff --git a/sysdeps/x86_64/multiarch/init-arch.h b/sysdeps/x86_64/multiarch/init-arch.h
index 813b6de..6449109 100644
--- a/sysdeps/x86_64/multiarch/init-arch.h
+++ b/sysdeps/x86_64/multiarch/init-arch.h
@@ -49,6 +49,11 @@ 
 
 #ifdef __ASSEMBLER__
 
+/* The feature index constants must match those used in the non-assembly
+   below.  We can't use a unified definition because the assembly won't
+   accept enum constants, nor will other macros.  */
+# define FEATURE_INDEX_1 0
+
 # include <ifunc-defines.h>
 
 # define index_SSE2    COMMON_CPUID_INDEX_1*CPUID_SIZE+CPUID_EDX_OFFSET
@@ -85,6 +90,7 @@  enum
 enum
   {
     FEATURE_INDEX_1 = 0,
+# define FEATURE_INDEX_1 0
     /* Keep the following line at the end.  */
     FEATURE_INDEX_MAX
   };