Bug 66457

Summary: testatomic: Line 908: Unexpected value
Product: APR Reporter: Lubos Uhliarik <luhliari>
Component: APR testAssignee: Apache Portable Runtime bugs mailinglist <bugs>
Status: NEW ---    
Severity: normal    
Priority: P2    
Version: 1.7.2   
Target Milestone: ---   
Hardware: PC   
OS: Linux   
Attachments: build.log from i686 machine
Config.log
apr_private.h

Description Lubos Uhliarik 2023-02-02 01:55:33 UTC
Created attachment 38492 [details]
build.log from i686 machine

After downloading apr-1.7.2 and trying to build it on i686 (on all other platforms it was OK), I'm getting following error:

make[1]: Leaving directory '/builddir/build/BUILD/apr-1.7.2/test'
+ ./testall -v -q
testatomic          :  Line 908: Unexpected value
FAILED 1 of 33
testdir             :  SUCCESS
testdso             :  SUCCESS
testdup             :  SUCCESS
testencode          :  SUCCESS
testenv             :  SUCCESS
testescape          :  SUCCESS
testfile            :  abcdefSUCCESS
testfilecopy        :  SUCCESS
testfileinfo        :  SUCCESS
testflock           :  SUCCESS
testfmt             :  SUCCESS
testfnmatch         :  SUCCESS
testargs            :  SUCCESS
testhash            :  SUCCESS
testipsub           :  SUCCESS
testlock            :  SUCCESS
testcond            :  SUCCESS
testlfs             :  SUCCESS
testmmap            :  SUCCESS
testnames           :  SUCCESS
testoc              :  SUCCESS
testpath            :  SUCCESS
testpipe            :  SUCCESS
testpoll            :  SUCCESS
testpools           :  SUCCESS
testproc            :  SUCCESS
testprocmutex       :  SUCCESS
testrand            :  SUCCESS
testsleep           :  SUCCESS
testshm             :  SUCCESS
testsock            :  SUCCESS
testsockets         :  SUCCESS
testsockopt         :  SUCCESS
teststr             :  SUCCESS
teststrnatcmp       :  SUCCESS
testtable           :  SUCCESS
testtemp            :  SUCCESS
testthread          :  SUCCESS
testtime            :  SUCCESS
testud              :  SUCCESS
testuser            :  SUCCESS
testvsn             :  SUCCESS
testskiplist        :  SUCCESS
Failed Tests   		Total	Fail	Failed %
===================================================
testatomic     		   33	   1	  3.03%


I'm also attaching whole build log from the i686 machine
Comment 1 Yann Ylavic 2023-02-02 09:48:23 UTC
Could you please attach the config.log file?
Comment 2 Yann Ylavic 2023-02-02 10:01:47 UTC
And "include/arch/unix/apr_private.h" too if possible.
Comment 3 Lubos Uhliarik 2023-02-02 11:44:02 UTC
Created attachment 38495 [details]
Config.log

Hi Yann, I'm attaching config.log and apr_private.h
Comment 4 Lubos Uhliarik 2023-02-02 11:44:36 UTC
Created attachment 38496 [details]
apr_private.h
Comment 5 Yann Ylavic 2023-02-09 13:52:47 UTC
Fixed in r1907541, can you give it a try please?
Comment 6 Lubos Uhliarik 2023-02-10 01:32:28 UTC
Hi Yann,

I can confirm that your patch fixes test failure on i686. Thanks!
Comment 7 Yann Ylavic 2023-03-02 17:42:17 UTC
Lubos, I made some changes in r1907988 to possibly enable atomic builtins on i586 and i686 (gcc for instance provides them for these cpus), and since you are using -march=i686 I wonder if it's a suitable change for you or if -march=i686 should mean --enable-nonportable-atomics=no like before for your (and/or redhat's) case.
Since a real i686 cpu is quite unlikely to be used/found these days, what's the point of -march=i686 (wouldn't only -m32 be enough)?
Some thoughts/opinions about this change?
Comment 8 Lubos Uhliarik 2023-03-02 18:22:53 UTC
Hello Yann,

We don't ship i686 packages in Fedora/RHEL, we only build them if it is not explicitly turned off. I only wanted to report this test failure, since I thought it might be broken on i686 and I didn't want to turn off i686 builds.

If I understand to the change correctly, usage of -march=i686 will set --enable-nonportable-atomics=no and therefore atomics buildins will be disabled. If so I think it is fine by me and as I mentioned before, we only build it on i686 to test if it is build-able on this arch.
Comment 9 Yann Ylavic 2023-03-02 21:44:58 UTC
(In reply to Lubos Uhliarik from comment #8)
> If I understand to the change correctly, usage of -march=i686 will set
> --enable-nonportable-atomics=no and therefore atomics buildins will be
> disabled.

The other way around, -march=i686 was implicitely forcing --enable-nonportable-atomics=no unconditionally before this change (thus always using the generic/mutex implementation), now if the compiler supports atomic builtins for i686 they will be used instead (they are more efficient after all).
I don't think it matters much because i686 atomic builtins will also work on newer Intel CPUs if APR finally runs on a >i686, but if -march=i686 was used (for whatever reasons) to disable builtins it will not be the case anymore (an explicit --enable-nonportable-atomics=no will still do that of course).
My reasoning is that if -march=i686 is usually set as a large Intel 32bit CPUs compatibility toggle then there is no reason to forcibly disable builtins thus existing optimizations (should performance matter on 32bit systems..).

PS: I'm talking about 32bit atomics only here, 64bit ones will always use the generic/mutex implementation on real or -march/forced 32bit CPUs.
Comment 10 Lubos Uhliarik 2023-03-03 18:59:36 UTC
Hello Yann,

OK, it makes sense to me. As I previously mention, we don't use/ship i686 packages, so it won't have any impact on our packages performance. 

I have created a new build including your new config change which forces to use atomic buildins and I can confirm that it builds and passes all tests.

Thanks a lot for your time and looking into this!