From patchwork Thu May 1 23:35:32 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Carlos O'Donell X-Patchwork-Id: 788 Return-Path: X-Original-To: siddhesh@wilcox.dreamhost.com Delivered-To: siddhesh@wilcox.dreamhost.com Received: from homiemail-mx23.g.dreamhost.com (mx2.sub5.homie.mail.dreamhost.com [208.113.200.128]) by wilcox.dreamhost.com (Postfix) with ESMTP id 22CB636007D for ; Thu, 1 May 2014 16:35:53 -0700 (PDT) Received: by homiemail-mx23.g.dreamhost.com (Postfix, from userid 14307373) id BD55463245C21; Thu, 1 May 2014 16:35:52 -0700 (PDT) X-Original-To: glibc@patchwork.siddhesh.in Delivered-To: x14307373@homiemail-mx23.g.dreamhost.com Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by homiemail-mx23.g.dreamhost.com (Postfix) with ESMTPS id 8774B6324E518 for ; Thu, 1 May 2014 16:35:52 -0700 (PDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:message-id:date:from:mime-version:to:cc :subject:references:in-reply-to:content-type :content-transfer-encoding; q=dns; s=default; b=sqLNXJsW6tKRF2Tu WE16g03dvjn/PazeKI5MtHIJDxl3UVRdxelA+8OlY8LCgQzyiWJxzTLVjdx1HFA7 edGMo5ndKRbxW3WybZDN4zzLImSJ7m2s4PlvTxRBJxF3VCY8Yli1ABza4WelR+SZ ejah8vWKRO3rRvytvQq93EO/HTQ= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:message-id:date:from:mime-version:to:cc :subject:references:in-reply-to:content-type :content-transfer-encoding; s=default; bh=7xXq0yhEp3TUkysKo9OlAh cA3CI=; b=S+dvsLt6MLrRLaU+VLmphzH8OmnQT+PcaRIh4ZQ2Je4M46JBRYhNUf FfN6j8bmLXWzvRityB/1N+A6F/ebQQ6IWEAk17DgchjxrbzJxMz2ed+SxIKMJW6A xHR/wi0a2YHAdIgcgQmCM34PaVZ7+AcEJZsRi76ylW4/fmuPdFUOU= Received: (qmail 28052 invoked by alias); 1 May 2014 23:35:40 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 27996 invoked by uid 89); 1 May 2014 23:35:39 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.0 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Message-ID: <5362DA44.3090507@redhat.com> Date: Thu, 01 May 2014 19:35:32 -0400 From: "Carlos O'Donell" User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.3.0 MIME-Version: 1.0 To: Roland McGrath CC: GNU C Library Subject: Re: [PATCH] Fix -Wundef for FEATURE_INDEX_1. References: <535F6BDE.3050801@redhat.com> <20140430175513.3DF042C39DE@topped-with-meat.com> In-Reply-To: <20140430175513.3DF042C39DE@topped-with-meat.com> X-DH-Original-To: glibc@patchwork.siddhesh.in On 04/30/2014 01:55 PM, Roland McGrath wrote: >> 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? Correct. >> 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? Good point. Nothing really. The enum constant use was likely a first step at making this typo-proof, but you need to convert everything and use sym files for assembly. Thus I find this easier and equally typo-proof. > 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. Done. I've removed only the FEATURE_INDEX enum which is the only problematic case for -Wundef. This fixes all FEATURE_INDEX* warnings. No regressions on x86-64 (and you'd notice, I messed this up once and the whole testsuite falls over because it breaks ifuncs). 2014-05-01 Carlos O'Donell * sysdeps/x86_64/multiarch/init-arch.h: Define FEATURE_INDEX_1 to 0. [!__ASSEMBLER__]: Remove anonymous enum for FEATURE_INDEX_*. --- Cheers, Carlos. diff --git a/sysdeps/x86_64/multiarch/init-arch.h b/sysdeps/x86_64/multiarch/init-arch.h index 813b6de..e4d265d 100644 --- a/sysdeps/x86_64/multiarch/init-arch.h +++ b/sysdeps/x86_64/multiarch/init-arch.h @@ -47,6 +47,12 @@ #define bit_XMM_state (1 << 1) #define bit_YMM_state (2 << 1) +/* The integer bit array index for the first set of internal feature bits. */ +# define FEATURE_INDEX_1 0 + +/* The current maximum size of the feature integer bit array. */ +# define FEATURE_INDEX_MAX 1 + #ifdef __ASSEMBLER__ # include @@ -82,13 +88,6 @@ enum COMMON_CPUID_INDEX_MAX }; -enum - { - FEATURE_INDEX_1 = 0, - /* Keep the following line at the end. */ - FEATURE_INDEX_MAX - }; - extern struct cpu_features { enum cpu_features_kind