From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl1-x62d.google.com (mail-pl1-x62d.google.com [IPv6:2607:f8b0:4864:20::62d]) by sourceware.org (Postfix) with ESMTPS id 801F33858D3C for ; Fri, 24 Nov 2023 12:28:49 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 801F33858D3C Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 801F33858D3C Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::62d ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1700828932; cv=none; b=UOb0JYXtIdzHsIBKs6dKGYvgNyS/WigeSHovNIlxSwtEeXRtgFTNBZsWB8bPWalqx8n1ESVw/3jKvgi55Lp/Cv/IU1xad96ZvxGQ2okm30fcqGB74G75p4Fs6grP9vmoSjgygjXN1uUcUksxjYVnEat10twk8GGFC2qHmGZHeVQ= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1700828932; c=relaxed/simple; bh=AHILcgsS5WygwhJtIOwQg+4yTP2j57HLf/IM2EFj2vk=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=xHKziqw6son91cNKr41JVgReLDAiMev+syMqkuLMdcCdxnXc01s+dE4DYzIo8I3YB8ZsSKqMx8xlUoEXPiuveH7XWwKK6g+Eh+EfocntG/jvnszPUZ5tSH9kSWN+8Qxi/UpmW3szdRj9PrdVc4CP9frHenLMH9YNzeQ4PhxRUDQ= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-pl1-x62d.google.com with SMTP id d9443c01a7336-1cf6373ce31so13573485ad.0 for ; Fri, 24 Nov 2023 04:28:49 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1700828928; x=1701433728; darn=sourceware.org; h=content-transfer-encoding:in-reply-to:organization:from:references :to:content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=VkEg/5mW+BTXwQ317+kDwc12O1r9nnpL81h0WgjBV2A=; b=Kb1+2Nqgig8KazvjUFIxb2IHF0NEHxofhZVvYYZW8kClsSHjQxx1KXwGNYhyHqIkHJ HFR7RPIt8KGp9qRm4Vs6FNoJjRrLNIlLBHRSqXAj8LFw+1Jc4/LqSIqqUbPW3utCoeVu z+C7sQoGCkx7wirCldJ4HRwNlawRRvgAgwk8EQBrQjSlozIfAvm1OaYqVz+QN617zHlm YAPllRY6CJ62RyFsDkNl0+1ZBGpwbJ4DdVe4fAbjk5U0ElNMoO8ohZp5nW074lOf7w1e JZVpawpVGUeiTqbpU04IZBp54mjhGoDh/6N706we29dol70L9XI+3iCCfGxBlf/7n2fc uocA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1700828928; x=1701433728; h=content-transfer-encoding:in-reply-to:organization:from:references :to:content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=VkEg/5mW+BTXwQ317+kDwc12O1r9nnpL81h0WgjBV2A=; b=dMgRhwRb2vXwC9xoibMdW6+ip5jc8t8DET3VIdRE0tl2CUVAJEGokrhuAMMAD0r1Q5 rGlilJWLwMvtLuiDSfviwLX/B3FU3kmWfju4DOPwB7ubJosAKyX41rsS6644qt96TAAZ l99QruKBuyuk26h8hFn2vp5UbFASuKnbVAf/Y1CA+zP8+MuPrX4sAx0PDcrrqWr+nvzO csDgqk0vWjAD4VQf7PdYWwOFHhTtYLwh+FIG5pSjfuM04uL02FJmBxM4GFjGHzZgcLBp IccHq68SOoW6hnQLtotSY4Q/OryGHWk72Osxd9hJtV3GtJeq+k8vej3vRHzpnfDT8etg ixeA== X-Gm-Message-State: AOJu0Yy3vigvz+gxSpTOweEOxX/oV0oudj71Bc7pCB6gn0HBAcoM2q4h rYrugL4hEmjQDOOF3oLU7wlLyBq43vJRHvaPqmKUsg== X-Google-Smtp-Source: AGHT+IGwdp9CrGUodg7INq29LQNFXUJ1gtD8YR4ghTxqwN8AmcDOA2kKRI0u4H2vdAF56GOS3vq+MA== X-Received: by 2002:a17:903:41cb:b0:1cf:ad9b:b97a with SMTP id u11-20020a17090341cb00b001cfad9bb97amr239149ple.33.1700828928357; Fri, 24 Nov 2023 04:28:48 -0800 (PST) Received: from ?IPV6:2804:1b3:a7c2:94e:6d59:cb73:1a0b:30ad? ([2804:1b3:a7c2:94e:6d59:cb73:1a0b:30ad]) by smtp.gmail.com with ESMTPSA id s17-20020a170902ea1100b001c9c47d6cb9sm3065061plg.99.2023.11.24.04.28.45 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 24 Nov 2023 04:28:47 -0800 (PST) Message-ID: Date: Fri, 24 Nov 2023 09:28:44 -0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 1/7] powerpc: Do not raise exception traps for fesetexcept/fesetexceptflag (BZ 30988) Content-Language: en-US To: Carlos O'Donell , libc-alpha@sourceware.org, Bruno Haible References: <20231106132713.953501-1-adhemerval.zanella@linaro.org> <20231106132713.953501-2-adhemerval.zanella@linaro.org> <6130f4c9-dab2-6f8e-5bc5-902b5a48e2dc@redhat.com> <5e031d35-5d3e-49a7-b354-809bb4a1dc8f@linaro.org> <6e4ff3c5-8504-79a4-8865-0239b0cd7185@redhat.com> <8aaf2565-5310-44aa-a331-6d12b26d2274@linaro.org> <3052e518-dac2-4b23-a070-787db5e13bf2@linaro.org> <6e52fa2f-9198-4a0a-9c77-80a1eccbd347@linaro.org> <0f892926-e336-f08d-c96e-0ad38d4ce75a@redhat.com> From: Adhemerval Zanella Netto Organization: Linaro In-Reply-To: <0f892926-e336-f08d-c96e-0ad38d4ce75a@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 23/11/23 18:47, Carlos O'Donell wrote: > On 11/6/23 15:46, Adhemerval Zanella Netto wrote: >> >> >> On 06/11/23 14:56, Adhemerval Zanella Netto wrote: >>> >>> >>> On 06/11/23 14:38, Carlos O'Donell wrote: >>>> On 11/6/23 12:11, Adhemerval Zanella Netto wrote: >>>>> >>>>> >>>>> On 06/11/23 14:02, Carlos O'Donell wrote: >>>>>> On 11/6/23 11:50, Adhemerval Zanella Netto wrote: >>>>>>> >>>>>>> >>>>>>> On 06/11/23 13:08, Carlos O'Donell wrote: >>>>>>>> On 11/6/23 08:27, Adhemerval Zanella wrote: >>>>>>>>> According to ISO C23 (7.6.4.4), fesetexcept is supposed to set >>>>>>>>> floating-point exception flags without raising a trap (unlike >>>>>>>>> feraiseexcept, which is supposed to raise a trap if feenableexcept was >>>>>>>>> called with the appropriate argument). >>>>>>>>> >>>>>>>>> This is a side-effect of how we implement the GNU extension >>>>>>>>> feenableexcept, where feenableexcept/fesetenv/fesetmode/feupdateenv >>>>>>>>> might issue prctl (PR_SET_FPEXC, PR_FP_EXC_PRECISE) depending of the >>>>>>>>> argument. And on PR_FP_EXC_PRECISE, setting a floating-point exception >>>>>>>>> flag triggers a trap. >>>>>>>>> >>>>>>>>> To make the both functions follow the C23, fesetexcept and >>>>>>>>> fesetexceptflag now fail if the argument may trigger a trap. >>>>>>>> >>>>>>>> OK. I reviewed ISO C 2x (n3096), and I agree this is permissible and preferable. >>>>>>>> >>>>>>>>> >>>>>>>>> The math tests now check for an value different than 0, instead >>>>>>>>> of bail out as unsupported for EXCEPTION_SET_FORCES_TRAP. >>>>>>>>> >>>>>>>>> Checked on powerpc64le-linux-gnu. >>>>>>>> >>>>>>>> Changes test from UNSUPPORTED to PASS when we should test more now that with >>>>>>>> C2x we're saying the behaviour will result in a non-zero return... then we >>>>>>>> should test for that. >>>>>>>> >>>>>>>>> --- >>>>>>>>> math/test-fesetexcept-traps.c | 11 ++++------- >>>>>>>>> math/test-fexcept-traps.c | 11 ++++------- >>>>>>>>> sysdeps/powerpc/fpu/fesetexcept.c | 5 +++++ >>>>>>>>> sysdeps/powerpc/fpu/fsetexcptflg.c | 9 ++++++++- >>>>>>>>> 4 files changed, 21 insertions(+), 15 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/math/test-fesetexcept-traps.c b/math/test-fesetexcept-traps.c >>>>>>>>> index 71b6e45b33..96f6c4752f 100644 >>>>>>>>> --- a/math/test-fesetexcept-traps.c >>>>>>>>> +++ b/math/test-fesetexcept-traps.c >>>>>>>>> @@ -39,16 +39,13 @@ do_test (void) >>>>>>>>> return result; >>>>>>>>> } >>>>>>>>> >>>>>>>>> - if (EXCEPTION_SET_FORCES_TRAP) >>>>>>>>> - { >>>>>>>>> - puts ("setting exceptions traps, cannot test on this architecture"); >>>>>>>>> - return 77; >>>>>>>>> - } >>>>>>>>> - /* Verify fesetexcept does not cause exception traps. */ >>>>>>>>> + /* Verify fesetexcept does not cause exception traps. For architectures >>>>>>>>> + where setting the exception might result in traps the function should >>>>>>>>> + return a nonzero value. */ >>>>>>>>> ret = fesetexcept (FE_ALL_EXCEPT); >>>>>>>>> if (ret == 0) >>>>>>>> >>>>>>>> We can check for a non-zero return if EXCEPTION_SET_FORCES_TRAP? >>>>>>>> >>>>>>>> e.g. >>>>>>>> >>>>>>>> if (!EXCEPTION_SET_FORCES_TRAP) >>>>>>>> { >>>>>>>> if (ret == 0) >>>>>>>> puts ("fesetexcept (FE_ALL_EXCEPT) succeeded"); >>>>>>>> else >>>>>>>> /* fail */ >>>>>>>> } >>>>>>>> else >>>>>>>> { >>>>>>>> if (ret == 0) >>>>>>>> /* fail */ >>>>>>>> else >>>>>>>> /* pass */ >>>>>>>> } >>>>>>> >>>>>>> The '!EXCEPTION_SET_FORCES_TRAP && ret == 0' or 'EXCEPTION_SET_FORCES_TRAP && ret == 1' >>>>>>> checks are not really meaningful: either the function succeeds and return 0, or it fails >>>>>>> for some reason. And for failure, EXCEPTION_SET_FORCES_TRAP really means an expected >>>>>>> failure. >>>>>> >>>>>> Sure. >>>>>> >>>>>>> So if the function succeeds and no trap is generated (which terminates the process >>>>>>> as default on Linux) we are fine. Otherwise, it check if the failure is expected >>>>>>> (EXCEPTION_SET_FORCES_TRAP). >>>>>>> >>>>>> >>>>>> So we go from UNSUPPORTED to... ? >>>>>> >>>>> >>>>> I though about that, but the test also checks fegetexceptflag (a better option would >>>>> to split the test in two, so only the fesetexceptflag is unsupported on ppc32). >>>>> >>>> >>>> Perhaps the best option is to just keep the UNSUPPORTED status? >>>> >>> >>> Fair enough. >> >> Revising the patch, I recalled that I explicitly removed the UNSUPPORTED >> so the test can now check if the fesetexcept does fails with -1 for >> !EXCEPTION_SET_FORCES_TRAP. I am not sure if adding it back is an improvement, >> it means that it won't actually check if BZ#30988 is really fixed. > > My apologies that we have gone around in a circle. > > Let me start again. > > And for the public record and your review I'll write down my assumptions. > > (a) Previously calling fesetexcept() (ISO/IEC 60559) or fesetexceptflag() (ISO C) > on POWER would raise a trap because the hardware can only raise the flag if > it *also* forces a trap. > > (b) In Bug 30988 (a) is raised as an ISO/IEC 60559 and ISO C conformance issue. > And the fix is to return an error from fesetexcept() or fesetexceptflag() if > the hardware cannot raise a flag without *also* forcing a trap (which fails > to comply with the standard definition). > > (c) In your patch 1/7 you want to remove the "return 77;" for the > EXCEPTION_SET_FORCES_TRAP path because it can now be tested. > > Given (c) my expectation is that we *actively* test for the failure. > > Your test changes look they will cause POWER to now fail the test, particularly > since 'EXCEPTION_TESTS (float)' for POWER will always be true because we want > to test exceptions (it's just that our expectations are different). It won't fail on powerpc (I actually tested using the gcc compile farm), because EXCEPTION_TESTS (float) won't be checked: volatile double a = 1.0; volatile double b = a + a; math_force_eval (b); // It will trigger the exception volatile long double al = 1.0L; volatile long double bl = al + al; math_force_eval (bl); if (ret == 0) // ret will -1 here (this very fix) puts ("fesetexcept (FE_ALL_EXCEPT) succeeded"); else if (!EXCEPTION_SET_FORCES_TRAP) // EXCEPTION_SET_FORCES_TRAP is set to 1 { puts ("fesetexcept (FE_ALL_EXCEPT) failed"); if (EXCEPTION_TESTS (float)) { puts ("failure of fesetexcept was unexpected"); result = 1; } else puts ("failure of fesetexcept OK"); } > > Let me sketch out what I was expecting for both test cases: > > diff --git a/math/test-fesetexcept-traps.c b/math/test-fesetexcept-traps.c > index 71b6e45b33..5ea295a5b8 100644 > --- a/math/test-fesetexcept-traps.c > +++ b/math/test-fesetexcept-traps.c > @@ -23,46 +23,97 @@ > static int > do_test (void) > { > - int result = 0; > + int errors = 0; > + int ret; > > fedisableexcept (FE_ALL_EXCEPT); > - int ret = feenableexcept (FE_ALL_EXCEPT); > + ret = feenableexcept (FE_ALL_EXCEPT); > if (!EXCEPTION_ENABLE_SUPPORTED (FE_ALL_EXCEPT) && (ret == -1)) > { > - puts ("feenableexcept (FE_ALL_EXCEPT) not supported, cannot test"); > + puts ("UNSUPPORTED: feenableexcept (FE_ALL_EXCEPT) not supported, cannot test"); > return 77; > } > else if (ret != 0) > { > - puts ("feenableexcept (FE_ALL_EXCEPT) failed"); > - result = 1; > - return result; > + puts ("FAIL: feenableexcept (FE_ALL_EXCEPT)"); > + errors++; > + return errors; > } > > - if (EXCEPTION_SET_FORCES_TRAP) > + if (!EXCEPTION_SET_FORCES_TRAP) > { > - puts ("setting exceptions traps, cannot test on this architecture"); > - return 77; > + /* Verify fesetexcept does not cause exception traps. */ > + ret = fesetexcept (FE_ALL_EXCEPT); > + if (ret == 0) > + puts ("PASS: fesetexcept (FE_ALL_EXCEPT)"); > + else > + { > + /* Some architectures are expected to fail. */ > + if (EXCEPTION_TESTS (float)) > + puts ("PASS: fesetexcept (FE_ALL_EXCEPT) " > + "failed as expected because testing is disabled"); > + else > + { > + puts ("FAIL: fesetexcept (FE_ALL_EXCEPT)"); > + errors++; > + } > + } > + ret = feclearexcept (FE_ALL_EXCEPT); > + if (ret == 0) > + puts ("PASS: feclearexcept (FE_ALL_EXCEPT)"); > + else > + { > + /* Some architectures are expected to fail. */ > + if (EXCEPTION_TESTS (float)) > + { > + puts ("PASS: feclearexcept (FE_ALL_EXCEPT) " > + "failed as expected because testing is disabled"); > + } > + else > + { > + puts ("FAIL: feclearexcept (FE_ALL_EXCEPT) failed"); > + errors++; > + } > + } > } > - /* Verify fesetexcept does not cause exception traps. */ > - ret = fesetexcept (FE_ALL_EXCEPT); > - if (ret == 0) > - puts ("fesetexcept (FE_ALL_EXCEPT) succeeded"); > else > { > - puts ("fesetexcept (FE_ALL_EXCEPT) failed"); > - if (EXCEPTION_TESTS (float)) > + /* Verify fesetexcept fails because the hardware cannot set the > + exceptions without also raising them. */ > + ret = fesetexcept (FE_ALL_EXCEPT); > + if (ret == 0) > { > - puts ("failure of fesetexcept was unexpected"); > - result = 1; > + puts ("FAIL: fesetexcept (FE_ALL_EXCEPT) succeeded unexpectedly"); > + errors++; > } I think this is essentially what you think my proposed change is incomplete, I assume that EXCEPTION_SET_FORCES_TRAP is a hit since I think it might be possible that either kernel might paper over this limitation (by some instruction emulation to hide the exception signal) or a new chip revision might eventually fix it (as i686 did with SSE2). Maybe it would be better to assume that EXCEPTION_SET_FORCES_TRAP is a failure expectation and trigger a regression is function succeeds. > else > - puts ("failure of fesetexcept OK"); > + { > + if (EXCEPTION_TESTS (float)) > + puts ("PASS: fesetexcept (FE_ALL_EXCEPT) " > + "failed as expected because testing is disabled"); > + else > + puts ("PASS: fesetexcept (FE_ALL_EXCEPT) failed as expected"); > + } > + ret = feclearexcept (FE_ALL_EXCEPT); > + if (ret == 0) > + puts ("PASS: feclearexcept (FE_ALL_EXCEPT)"); > + else > + { > + /* Some architectures are expected to fail. */ > + if (EXCEPTION_TESTS (float)) > + { > + puts ("PASS: feclearexcept (FE_ALL_EXCEPT) " > + "failed as expected because testing is disabled"); > + } > + else > + { > + puts ("FAIL: feclearexcept (FE_ALL_EXCEPT) failed"); > + errors++; > + } > + } > } > - feclearexcept (FE_ALL_EXCEPT); > > - return result; > + return errors; > } > > -#define TEST_FUNCTION do_test () > -#include "../test-skeleton.c" > +#include > --- > > My point is that by changing the implementation we need to test a whole > different set of conditions now and the test needs expanding, likewise > with test-fexcept-traps.c. > > We need two testing paths with different expectations? No really, the whole point of the test is to check: int exc_before = fegetexcept (); ret = fesetexcept (FE_ALL_EXCEPT); int exc_after = fegetexcept (); Will not change the exception mask (exc_before == exc_after) *and* not generate any trap (which you abort the process). Also, for i686 we need to trigger some math operations after the fesetexcept to check no exception will be triggered. Now, if ret is 0 everything works as expected. If ret is -1, it would depend whether the architecture has EXCEPTION_SET_FORCES_TRAP: * if is not set, it will depend whether the architectures allows setting the exception for the specific float type (EXCEPTION_TESTS (float), which is expanded to the constants defined by math-tests-exceptions.h). Some architectures does not support exceptions at all (riscv), or it depends of the ABI (arc, arm, loongarch, and ork1 in soft-fp mode). * if it is set (powerpc and i386/x87) it means that there is no extra checks requires, since the failure for these architectures *is* expected. So assuming EXCEPTION_SET_FORCES_TRAP is a hard indication, I think this below would be suffice: if (ret == 0) puts ("fesetexcept (FE_ALL_EXCEPT) succeeded"); else if (!EXCEPTION_SET_FORCES_TRAP) { puts ("fesetexcept (FE_ALL_EXCEPT) failed"); if (EXCEPTION_TESTS (float)) { puts ("failure of fesetexcept was unexpected"); result = 1; } else puts ("failure of fesetexcept OK"); } else { if (ret == 0) puts ("unexpected fesetexcept success"); result = ret != -1; }