From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-x244.google.com (mail-oi1-x244.google.com [IPv6:2607:f8b0:4864:20::244]) by sourceware.org (Postfix) with ESMTPS id 13E173858034 for ; Tue, 17 Nov 2020 15:52:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 13E173858034 Received: by mail-oi1-x244.google.com with SMTP id k26so23080836oiw.0 for ; Tue, 17 Nov 2020 07:52:06 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=KJXgnebvnp0KdDklD6DYJa2RiRS3i6Q2Cxcaukq/t+c=; b=N9WwUPszIhXtn8wWHHFZmEiTl99805pfe25Qe9ZLPvGcQvmOrSjJRSctcvUp6xQ1gm Fx1+vFZPPphwzQZpPC0llFv7L326VQv+fMjiy+hSB88B2V0a33iHnTk3zvYeuWpOiFXa 8u6xD/3KcJ4NEQQrCtqBpXSbh6YoWJVJsVJhDT2Z/muu3zJlbSfeGPHPAzJBmRpjIRPp 9GcV1B6tLXWBCqUShfqjfzJdVCFTA0oz2elxF29nefO6SYL4s6qtfL2ylr94cMAwsPKB roLW0K1tmSwS4dfOjph2PDV4wMq6P8SFvmUgvKSLepiqH7LkPTf8Kpu0X5IT9hBgUJ0o g1Ig== X-Gm-Message-State: AOAM531PA1yin2Zkhuefxmqmqnvc+svYLMb49S5IFBFQcnIFaHblZF8A dWLI7b/D+AkMWc69hwRyMDrs8tbZpjR11mtxV/4iag== X-Google-Smtp-Source: ABdhPJw4ooyf6y80Xqje2aEz2+s4n8E2pelsy8pAweyAxH+IInp8Melj3blM1OtNp1ta1Wd2R/Fvy9PzqzI+010NtR0= X-Received: by 2002:aca:4287:: with SMTP id p129mr1077924oia.48.1605628325306; Tue, 17 Nov 2020 07:52:05 -0800 (PST) MIME-Version: 1.0 References: <8383d817-8622-4d1f-9564-8c10131db664@arm.com> In-Reply-To: From: Christophe Lyon Date: Tue, 17 Nov 2020 16:51:54 +0100 Message-ID: Subject: Re: [PATCH] libgcc: Add a weak stub for __sync_synchronize To: "Richard Earnshaw (lists)" Cc: Bernd Edlinger , "gcc-patches@gcc.gnu.org" , Ramana Radhakrishnan Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-4.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 Nov 2020 15:52:08 -0000 On Tue, 17 Nov 2020 at 16:41, Richard Earnshaw (lists) via Gcc-patches wrote: > > On 17/11/2020 15:18, Bernd Edlinger wrote: > > On 11/17/20 1:44 PM, Richard Earnshaw (lists) wrote: > >> On 03/11/2020 15:08, Bernd Edlinger wrote: > >>> Hi, > >>> > >>> this fixes a problem with a missing symbol __sync_synchronize > >>> which happens when newlib is used together with libstdc++ for > >>> the non-threaded simulator target arm-none-eabi. > >>> > >>> There are several questions on stackoverflow about this issue. > >>> > >>> I would like to add a weak symbol for this target, since this > >>> is only a default implementation and not meant to override a > >>> possibly more sophisticated synchronization function from the > >>> c-runtime. > >>> > >>> > >>> Regression tested successfully on arm-none-eabi with newlib-3.3.0. > >>> > >>> Is it OK for trunk? > >>> > >>> > >>> Thanks > >>> Bernd. > >>> > >> > >> I seem to recall that this was a deliberate decision - you can't guess > >> this correctly, at least when trying to build portable code - you just > >> have to know which runtime you will be using. > >> > > > > Therefore I suggest to use the weak attribute. It is on purpose not > > implementing all of the atomics. > > > > The use case, is a C++ program which initializes a local static variable. > > > > $ cat test.cc > > #include > > main(int argc, char **argv) > > { > > static std::string x = "test"; > > return 0; > > } > > > > compiles to this: > > sub sp, sp, #20 > > str r0, [fp, #-24] > > str r1, [fp, #-28] > > ldr r3, .L14 > > ldrb r4, [r3] > > bl __sync_synchronize > > and r3, r4, #255 > > and r3, r3, #1 > > cmp r3, #0 > > moveq r3, #1 > > movne r3, #0 > > and r3, r3, #255 > > cmp r3, #0 > > beq .L8 > > ldr r0, .L14 > > bl __cxa_guard_acquire > > mov r3, r0 > > > > so __sync_synchronize is not defined in newlib since the target (arm-sim) > > is known to be not multi-threaded, > > but __cxa_guard_acquire is also not a thread safe function, > > because __GTHREADS is not defined by libgcc, since it is known > > at configure time, that the target does not support threads. > > So libstdc++ does not try to use a mutex or any atomics either, > > because it is not compiled with __GTHREADS. > > > > I can further narrow down the patch by only defining this function when > > __GTHREADS is not defined, to make it more clear. > > > > > >> I think Ramana had some changes in the works at one point to address > >> (some) of this, but I'm not sure what happened to them. Ramana? > >> > >> > >> +#if defined (__ARM_ARCH_6__) || defined (__ARM_ARCH_6J__) \ > >> + || defined (__ARM_ARCH_6K__) || defined (__ARM_ARCH_6T2__) \ > >> + || defined (__ARM_ARCH_6Z__) || defined (__ARM_ARCH_6ZK__) \ > >> + || defined (__ARM_ARCH_7__) || defined (__ARM_ARCH_7A__) > >> +#if defined (__ARM_ARCH_7__) || defined (__ARM_ARCH_7A__) > >> > >> Ug, no! Use the ACLE macros to avoid this sort of mess. > >> > > > > Ah, thanks, copy-paste from freebsd-atomic.c :) > > > > > > I've attached the updated patch. > > Is it OK? > > > > > > Thanks > > Bernd. > > > > libgcc is *still* the wrong place for this. It belongs in the system > library (eg newlib, or glibc, or whatever), which knows about the system > it's running on. (Sorry, I should have said this before, but I've > context-switched this out since it's been a long time since it came up). > > This hack will just lead to silent code failure of the worst kind > (non-reproducable, racy) at runtime. > I haven't fully re-read the thread, but I think this is related to an old discussion, not very well archived in: https://gcc.gnu.org/pipermail/gcc-patches/2016-November/462299.html There's a pointer to a newlib patch from Ramana. > R.