Message ID | e6d220e3-a7a2-44ba-841f-d0345c15b290@BAMAIL02.ba.imgtec.org |
---|---|
State | Rejected |
Delegated to: | Carlos O'Donell |
Headers |
Return-Path: <x14307373@homiemail-mx20.g.dreamhost.com> X-Original-To: siddhesh@wilcox.dreamhost.com Delivered-To: siddhesh@wilcox.dreamhost.com Received: from homiemail-mx20.g.dreamhost.com (mx2.sub5.homie.mail.dreamhost.com [208.113.200.128]) by wilcox.dreamhost.com (Postfix) with ESMTP id 2F37B360078 for <siddhesh@wilcox.dreamhost.com>; Tue, 29 Apr 2014 11:07:33 -0700 (PDT) Received: by homiemail-mx20.g.dreamhost.com (Postfix, from userid 14307373) id E293641431801; Tue, 29 Apr 2014 11:07:32 -0700 (PDT) X-Original-To: glibc@patchwork.siddhesh.in Delivered-To: x14307373@homiemail-mx20.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-mx20.g.dreamhost.com (Postfix) with ESMTPS id B99A84136934E for <glibc@patchwork.siddhesh.in>; Tue, 29 Apr 2014 11:07:32 -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:from:date:to:cc:subject:mime-version :content-type:content-transfer-encoding:message-id; q=dns; s= default; b=r8bXov/t5M/mf5OM5v3MG9fbc6fdQhke8dqYy3IRM4c+4zWHOEvFO oZCIp5jXAlTz8suvTmIOAt4pxLFT4uOgpEdOleyKoCWGpB83H4bTtEqAT6NtxElZ X05CS4aMRbQwqDK1aA59IHcqakFgPGKF63gbfMBTjzLqDAlan3g4cs= 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:from:date:to:cc:subject:mime-version :content-type:content-transfer-encoding:message-id; s=default; bh=KWA0A0I+bBOewyojT8XAc7t2D1M=; b=hqN0H5g7cJXmXo4wLe7w6jbwbsLZ 4WaNlaPPrNqewsTlX6EsFCKnXsOmIWUY8+9w6BIfGCUcP6+NinGg4Om5SehHk903 TaAR9l+CxCT9+WjIzrla9J+WUiExSgRrv6psP6OAGi0UqomOlm/GqtBO8wqj0seD npaGXWtbZePLJIg= Received: (qmail 18964 invoked by alias); 29 Apr 2014 18:07:30 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-glibc=patchwork.siddhesh.in@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 18952 invoked by uid 89); 29 Apr 2014 18:07:30 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mailapp01.imgtec.com From: "Steve Ellcey " <sellcey@mips.com> Date: Tue, 29 Apr 2014 11:07:15 -0700 To: <libc-alpha@sourceware.org> CC: <yufeng.zhang@arm.com> Subject: [Patch] Fix __mips16 undef macro warnings. User-Agent: Heirloom mailx 12.5 6/20/10 MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Message-ID: <e6d220e3-a7a2-44ba-841f-d0345c15b290@BAMAIL02.ba.imgtec.org> X-DH-Original-To: glibc@patchwork.siddhesh.in |
Commit Message
Steve Ellcey
April 29, 2014, 6:07 p.m. UTC
This is a change to stdlib/longlong.h to fix the undefined macro warning for __mips16. The GCC compiler defines __mips16 when compiling in mips16 mode and does not define it othewise. So this patch just changes the #if check to a #if defined(). stdlib/longlong.h is also in the GCC and binutils trees as include/longlong.h. If this patch is approved I will submit it to those groups for checkin as well. Right now it looks like the GCC and glibc files are in sync but the last change for clz on ARM did not make it into binutils for some reason. Tested with mips-mti-linux-gnu to verify that no object files were changed due to this patch. OK to checkin? Steve Ellcey sellcey@mips.com 2014-04-29 Steve Ellcey <sellcey@mips.com> * stdlib/longlong.h: Use 'defined()' to check __mips16.
Comments
On 04/29/2014 02:07 PM, Steve Ellcey wrote: > > This is a change to stdlib/longlong.h to fix the undefined macro > warning for __mips16. The GCC compiler defines __mips16 when > compiling in mips16 mode and does not define it othewise. So > this patch just changes the #if check to a #if defined(). This is not correct. We should never add `defined' or `ifdef' to fix this problem since it puts us back in the same position. We want to remove all ifdef or defined where possible. A better use is like this: * In a central mips header: /* Long description about this coming from the compiler and indicating that it will not be defined when not compiling MIPS16. */ #ifdef __mips16 #define __glibc_mips16 1 #else #define __glibc_mips16 0 #endif * Then all uses are always `if __glibc_mips16' to use when we want mips16 code, and that macro is always either 0 or 1 exactly. Thus you have a single check for the compiler define, and good comments explaining when it's defined and why, and then a macro that is either 1 or 0 for that value to use internally. However, if this is the *only* use of __mips16, then you can get away with using defined, but you should add a big and descriptive comment about when and how __mips16 is defined and set by the compiler. > stdlib/longlong.h is also in the GCC and binutils trees as > include/longlong.h. If this patch is approved I will submit > it to those groups for checkin as well. Right now it looks > like the GCC and glibc files are in sync but the last change > for clz on ARM did not make it into binutils for some reason. That's the wrong way around IIRC. The canonical source is gcc, and binutils and glibc get updated from that. > Tested with mips-mti-linux-gnu to verify that no object > files were changed due to this patch. > > OK to checkin? No. > Steve Ellcey > sellcey@mips.com > > > 2014-04-29 Steve Ellcey <sellcey@mips.com> > > * stdlib/longlong.h: Use 'defined()' to check __mips16. > > > diff --git a/stdlib/longlong.h b/stdlib/longlong.h > index d45dbe2..070b40c 100644 > --- a/stdlib/longlong.h > +++ b/stdlib/longlong.h > @@ -848,7 +848,7 @@ extern UDItype __umulsidi3 (USItype, USItype); > #define UMUL_TIME 10 > #define UDIV_TIME 100 > > -#if (__mips == 32 || __mips == 64) && ! __mips16 > +#if (__mips == 32 || __mips == 64) && ! defined (__mips16) > #define count_leading_zeros(COUNT,X) ((COUNT) = __builtin_clz (X)) > #define COUNT_LEADING_ZEROS_0 32 > #endif > Cheers, Carlos.
On 04/29/2014 02:47 PM, Carlos O'Donell wrote: >> OK to checkin? > > No. > >> Steve Ellcey >> sellcey@mips.com >> >> >> 2014-04-29 Steve Ellcey <sellcey@mips.com> >> >> * stdlib/longlong.h: Use 'defined()' to check __mips16. >> >> >> diff --git a/stdlib/longlong.h b/stdlib/longlong.h >> index d45dbe2..070b40c 100644 >> --- a/stdlib/longlong.h >> +++ b/stdlib/longlong.h >> @@ -848,7 +848,7 @@ extern UDItype __umulsidi3 (USItype, USItype); >> #define UMUL_TIME 10 >> #define UDIV_TIME 100 >> >> -#if (__mips == 32 || __mips == 64) && ! __mips16 >> +#if (__mips == 32 || __mips == 64) && ! defined (__mips16) >> #define count_leading_zeros(COUNT,X) ((COUNT) = __builtin_clz (X)) >> #define COUNT_LEADING_ZEROS_0 32 >> #endif In case it wasn't clear my recommendation is: Add a large descriptive comment about the use of __mips16 here and perhaps even talk about __mips32 and __mips64. Given the fact that this header is shared we can't enforce the same kind of source rules we enforce in glibc, and therefore the documentation of the define is enough to meet the idea of the goal for glibc right now. My other rant was just to set you straight if you decide to fix more of these [-Wundef] warnings. Eventually we're going to make this an error and then we get a lot of benefits from this process and remove a whole class of bugs dealing with undefined macros (which is why we want documentation in detail for macros that are checked with `defined' or `ifdef'.). Cheers, Carlos.
On Tue, 29 Apr 2014, Carlos O'Donell wrote: > * In a central mips header: > > /* Long description about this coming from the compiler and > indicating that it will not be defined when not compiling > MIPS16. */ > #ifdef __mips16 > #define __glibc_mips16 1 > #else > #define __glibc_mips16 0 > #endif longlong.h is shared, so a central header defining __glibc_* macros makes no sense whatsoever. There may be cases for converting compiler-defined macros to glibc-defined macros with the 0/1 convention (e.g. replacing tests of __ARM_ARCH_* with tests of __ARM_ARCH and defining that if the compiler is too old to define it itself), but this isn't one of them. And generally, if a simple defined/undefined test is sufficient for the relevant condition, I think defining a 0/1 macro is overkill.
On Tue, 29 Apr 2014, Carlos O'Donell wrote: > Add a large descriptive comment about the use of __mips16 > here and perhaps even talk about __mips32 and __mips64. I think a large comment about a compiler-defined macro would be completely out-of-place in this header - in MIPS-specific code, commenting on a test of __mips16 is just like i++; /* Add 1 to i. */ in generic C code. You can comment about *why* testing MIPS16 is relevant, but *what* the test does and *how* it does it are complete plain and don't need commenting.
On 04/29/2014 05:00 PM, Joseph S. Myers wrote: > On Tue, 29 Apr 2014, Carlos O'Donell wrote: > >> * In a central mips header: >> >> /* Long description about this coming from the compiler and >> indicating that it will not be defined when not compiling >> MIPS16. */ >> #ifdef __mips16 >> #define __glibc_mips16 1 >> #else >> #define __glibc_mips16 0 >> #endif > > longlong.h is shared, so a central header defining __glibc_* macros makes > no sense whatsoever. > > There may be cases for converting compiler-defined macros to glibc-defined > macros with the 0/1 convention (e.g. replacing tests of __ARM_ARCH_* with > tests of __ARM_ARCH and defining that if the compiler is too old to define > it itself), but this isn't one of them. And generally, if a simple > defined/undefined test is sufficient for the relevant condition, I think > defining a 0/1 macro is overkill. Agreed, this was just an example. For the record I like the idea of converting compiler macros to 0/1 conventions. Cheers, Carlos.
On 04/29/2014 05:03 PM, Joseph S. Myers wrote: > On Tue, 29 Apr 2014, Carlos O'Donell wrote: > >> Add a large descriptive comment about the use of __mips16 >> here and perhaps even talk about __mips32 and __mips64. > > I think a large comment about a compiler-defined macro would be completely > out-of-place in this header - in MIPS-specific code, commenting on a test > of __mips16 is just like > > i++; /* Add 1 to i. */ > > in generic C code. You can comment about *why* testing MIPS16 is > relevant, but *what* the test does and *how* it does it are complete plain > and don't need commenting. > My personal threshold for adding comments is set pretty low :-) c.
diff --git a/stdlib/longlong.h b/stdlib/longlong.h index d45dbe2..070b40c 100644 --- a/stdlib/longlong.h +++ b/stdlib/longlong.h @@ -848,7 +848,7 @@ extern UDItype __umulsidi3 (USItype, USItype); #define UMUL_TIME 10 #define UDIV_TIME 100 -#if (__mips == 32 || __mips == 64) && ! __mips16 +#if (__mips == 32 || __mips == 64) && ! defined (__mips16) #define count_leading_zeros(COUNT,X) ((COUNT) = __builtin_clz (X)) #define COUNT_LEADING_ZEROS_0 32 #endif