From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 98441 invoked by alias); 2 Feb 2017 15:46:04 -0000 Mailing-List: contact newlib-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: newlib-owner@sourceware.org Received: (qmail 98427 invoked by uid 89); 2 Feb 2017 15:46:03 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.2 required=5.0 tests=AWL,BAYES_20,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=bare-metal, baremetal, induce, H*f:sk:1485938 X-HELO: mail02.lgsinnovations.com Received: from mail02.lgsinnovations.com (HELO mail02.lgsinnovations.com) (63.149.110.42) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 02 Feb 2017 15:46:01 +0000 Subject: Re: [PATCH 2/3, newlib] Only define static locks in multithreaded mode To: References: <7a943313-48e7-2c8f-7250-ce47aeecda13@foss.arm.com> <1485938923.1269.3.camel@op.pl> From: Craig Howland Message-ID: <93d8a5bc-3ebf-2247-e98f-732d92232e43@LGSInnovations.com> Date: Thu, 02 Feb 2017 15:46:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-ClientProxiedBy: LGS-EX02.lgsdirect.com (135.22.77.165) To LGS-EX01.lgsdirect.com (135.22.77.164) X-IsSubscribed: yes X-SW-Source: 2017/txt/msg00119.txt.bz2 On 02/02/2017 09:32 AM, Thomas Preudhomme wrote: > On 01/02/17 08:48, Freddie Chopin wrote: >> On Tue, 2017-01-31 at 17:10 +0000, Thomas Preudhomme wrote: >>> It also makes sure locking macros in lock.h are noop in single thread >>> mode. >> >> Doesn't it make all the guards in the code superfluous? If the lock >> initialization macros are defined to be ";" and lock functions are >> defined to be "(void)0", then the guards in the code actually guard >> nothing, as things like: >> >> #if !defined(__SINGLE_THREAD__) && defined(HAVE_DD_LOCK) >> __LOCK_INIT(static, __dd_hash_mutex); >> #endif >> >> #ifndef __SINGLE_THREAD__ >> __lock_acquire(__dd_hash_mutex); >> #endif >> >> will then be equivalent to: >> >> #if !defined(__SINGLE_THREAD__) && defined(HAVE_DD_LOCK) >> ; >> #endif >> >> #ifndef __SINGLE_THREAD__ >> (void)0; >> #endif > > Yes it does and I started to remove them but noticed that removing the guard > made code more inconsistent. The example you give is a nice one because then > we would still have a condition with HAVE_DD_LOCK which might induce people to > think that's the only case where this line does nothing. > > In some other case __SINGLE_THREAD__ guards more than just the lock. For > instance in newlib/libc/stdio/findfp.c __SINGLE_THREAD__ is used to guard > whether some function are defined. This would need to be kept so the only > change that could be done is take the lock out of the __SINGLE_THREAD__ > guarded block which would look weird again. > > I then wondered whether making the code noop in lock.h was the right approach > but decided that provided more safety that the code would not be compiled in > in case of missing guard (as you discovered for some of the locks). It also > goes with what is currently being done when targeting bare-metal since the > lock routines themselves are noop in that case. Only the declarations are not. > The one thing that could be done is to make the declaration noop in both > cases, thus reducing the complexity. > > Any thoughts on that? > > Best regards, > > Thomas Yes, a thought on the general approach. At this moment in time the locks define to being degenerate so that the guards are superfluous. However, the guards make it very clear on reading the "application" code that the locks are not used in those cases, without needing to chase down what happens with the underlying functions. In addition, the locks becoming degenerate in the guarded cases could possibly change in the future. So the thought is that while the guards could be removed, don't. This not only keeps the main code more clear in intent, it saves the time to take them out, the time to check it, and could also possibly save the time putting them back later on if the present degeneration method were to be altered. Craig