From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 45A493858404 for ; Thu, 13 Oct 2022 10:35:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 45A493858404 Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com [209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-255-_YV7o3kDMfmh6UCZSVMZOQ-1; Thu, 13 Oct 2022 06:35:05 -0400 X-MC-Unique: _YV7o3kDMfmh6UCZSVMZOQ-1 Received: by mail-wr1-f71.google.com with SMTP id g27-20020adfa49b000000b0022cd5476cc7so379956wrb.17 for ; Thu, 13 Oct 2022 03:35:04 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=hqO2dqBlp0CFGL7MTqxLJzWRDD/IcPk2Xmjd4Kp5s+4=; b=eb+ZH9PEZTq+RKcGXJF/h/b+MCp6ftVYU8Xj4Pib7F2f39rOk5jVS2GXc44MqRo9oe Fi4fzFoJlwwhzVLyrEf1gX2314+zTlPVNoWqGqI0JceJZcEyQ+3IVo73TCu74CGJRtiy t/T+1nh2cxT8ElXvzT2L2LRO3bUxmGJPl8js+PEQRCy2ILv2j9yDKoi1iNjK7W9lntTd +feZZj785o30r4Zi3WgV1zzi8wd+9k/twH5jukihtDL1CavgEqacImyNqbyvD2H1WI4u 4/np+j2F9eTX+wxyw3NZI6Hjidam5Rf6RqDXWotydjiplJ9PTTAVYF4i485weMjh2mBM z4zg== X-Gm-Message-State: ACrzQf0AFsgPV+evADsHeYZhd+KxSAgnHNNwsqRQNiKlYFqlX/a5cye9 YpBoyopa3C9F4OV83Ve7dvijwqGHUypYt8N226rC4QYVNr2XzI4h9Q6Hy7nTX6MMw1bJJBxbaZv 40W6sEs7Oq45s+xG7LROEcQ== X-Received: by 2002:a5d:4f09:0:b0:22e:3f04:2820 with SMTP id c9-20020a5d4f09000000b0022e3f042820mr20758265wru.219.1665657303471; Thu, 13 Oct 2022 03:35:03 -0700 (PDT) X-Google-Smtp-Source: AMsMyM7iwF7sGVvTno6DULFB9WzdvC/eCIdbOhIs3L13EEwLLH+7WAmjFIlxE5uKAZ0Nnpul9k4KTA== X-Received: by 2002:a5d:4f09:0:b0:22e:3f04:2820 with SMTP id c9-20020a5d4f09000000b0022e3f042820mr20758242wru.219.1665657303115; Thu, 13 Oct 2022 03:35:03 -0700 (PDT) Received: from localhost (52.72.115.87.dyn.plus.net. [87.115.72.52]) by smtp.gmail.com with ESMTPSA id y16-20020a5d6210000000b0022cce7689d3sm2111559wru.36.2022.10.13.03.35.02 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 13 Oct 2022 03:35:02 -0700 (PDT) From: Andrew Burgess To: Lancelot SIX , Pedro Alves Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 4/5] sim/erc32: avoid dereferencing type-punned pointer warnings In-Reply-To: <20221012170215.imifj66p6dndtf6p@octopus> References: <170dc056-7e74-6c15-7131-31943c17be3a@palves.net> <20221012170215.imifj66p6dndtf6p@octopus> Date: Thu, 13 Oct 2022 11:35:01 +0100 Message-ID: <87czawau5m.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-10.3 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 13 Oct 2022 10:35:07 -0000 Lancelot SIX writes: > On Wed, Oct 12, 2022 at 03:11:27PM +0100, Pedro Alves wrote: >> On 2022-10-12 1:38 p.m., Andrew Burgess via Gdb-patches wrote: >> > When building the erc32 simulator I get a few warnings like this: >> > >> > /tmp/build/sim/../../src/sim/erc32/exec.c:1377:21: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing] >> > 1377 | sregs->fs[rd] = *((float32 *) & ddata[0]); >> > | ~^~~~~~~~~~~~~~~~~~~~~~~ >> > >> > The type of '& ddata[0]' will be 'uint32_t *', which is what triggers >> > the warning. >> > >> > This commit uses an intermediate pointer of type 'char *' when >> > performing the type-punning, which is well-defined behaviour, and will >> > silence the above warning. >> >> I'm afraid that's not correct. That's still undefined behavior, it's just silencing >> the warning. The end result is still aliasing float32 and uint32_t objects, and risks >> generating bogus code. The char escape hatch only works if you access the object >> representation via a character type. Here, the patch is still accessing the object >> representation of a uint32_t object via a floa32 type. >> >> Here's an old article explaining strict aliasing (just one that I found via a quick google): >> >> https://cellperformance.beyond3d.com/articles/2006/06/understanding-strict-aliasing.html >> >> This scenario is the "CASTING TO CHAR*" one in that article. >> >> > @@ -1345,7 +1345,8 @@ dispatch_instruction(struct pstate *sregs) >> > if (mexc) { >> > sregs->trap = TRAP_DEXC; >> > } else { >> > - sregs->fs[rd] = *((float32 *) & data); >> > + char *ptr = (char *) &data; >> > + sregs->fs[rd] = *((float32 *) ptr); >> >> The best IMHO is to do the type punning via a union, like e.g.: >> >> union { float32 f; uint32_t i; } u; >> u.i = data; >> sregs->fs[rd] = u.f; >> >> See here in the C11 standard: >> >> https://port70.net/~nsz/c/c11/n1570.html#note95 >> >> and also the documentation of -fstrict-aliasing at: >> >> https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html >> > > Hi, > > Another well defined (at least to my knowledge) solution to this problem > is memcpy. You could do something like: > > memcpy (&sregt->fs[rd], ddata, sizeof (float32)); > > I tend to find this more straightforward than the type punning version, > but I would be happy with either. > Pedro, Lancelot, thanks for taking the time to give really useful feedback. In the end I went with the memcpy approach. I ran a few tests with GCC, Clang, and ICC, and in each case the code generated at -O0 was either identical, or pretty much identical when using memcpy vs using a union. When switching to -O2 the code was identical in all cases I checked. Thoughts? Thanks, Andrew --- commit d04acbda1f2a191193772fc9416cf5b29f0702ce Author: Andrew Burgess Date: Wed Oct 12 11:45:53 2022 +0100 sim/erc32: avoid dereferencing type-punned pointer warnings When building the erc32 simulator I get a few warnings like this: /tmp/build/sim/../../src/sim/erc32/exec.c:1377:21: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing] 1377 | sregs->fs[rd] = *((float32 *) & ddata[0]); | ~^~~~~~~~~~~~~~~~~~~~~~~ The type of '& ddata[0]' will be 'uint32_t *', which is what triggers the warning. This commit makes use of memcpy when performing the type-punning, which resolves the above warnings. With this change, I now see no warnings when compiling exec.c, which means that the line in Makefile.in that disables -Werror can be removed. There should be no change in behaviour after this commit. diff --git a/sim/erc32/Makefile.in b/sim/erc32/Makefile.in index 786ae1dcc7b..41830aab726 100644 --- a/sim/erc32/Makefile.in +++ b/sim/erc32/Makefile.in @@ -32,9 +32,6 @@ SIM_EXTRA_CLEAN = clean-sis # behaviour of UART interrupt routines ... SIM_EXTRA_CFLAGS += -DFAST_UART -I$(srcroot) -# Some modules don't build cleanly yet. -exec.o: SIM_WERROR_CFLAGS = - ## COMMON_POST_CONFIG_FRAG # `sis' doesn't need interf.o. diff --git a/sim/erc32/exec.c b/sim/erc32/exec.c index ef93692e7a2..26d48c0e46e 100644 --- a/sim/erc32/exec.c +++ b/sim/erc32/exec.c @@ -1345,7 +1345,7 @@ dispatch_instruction(struct pstate *sregs) if (mexc) { sregs->trap = TRAP_DEXC; } else { - sregs->fs[rd] = *((float32 *) & data); + memcpy (&sregs->fs[rd], &data, sizeof (sregs->fs[rd])); } break; case LDDF: @@ -1373,11 +1373,12 @@ dispatch_instruction(struct pstate *sregs) } else { rd &= 0x1E; sregs->flrd = rd; - sregs->fs[rd] = *((float32 *) & ddata[0]); + memcpy (&sregs->fs[rd], &ddata[0], sizeof (sregs->fs[rd])); #ifdef STAT sregs->nload++; /* Double load counts twice */ #endif - sregs->fs[rd + 1] = *((float32 *) & ddata[1]); + memcpy (&sregs->fs[rd + 1], &ddata[1], + sizeof (sregs->fs[rd + 1])); sregs->ltime = ebase.simtime + sregs->icnt + FLSTHOLD + sregs->hold + sregs->fhold; }