Fix issue 21578 - core.atomic.atomicFetchSub for pointers incorrectly… #3343

rymrg · 2021-01-25T09:18:21Z

… calls wrong function from core.internal.atomic

I managed to break the previous pull request [/pull/3342] instead of fixing it. Sorry!
Updated the unit tests and targeted stable.

@PetarKirov Thanks for the input. See updates.

@kinke @ibuclaw

dlang-bot · 2021-01-25T09:18:24Z

Thanks for your pull request and interest in making D better, @rymrg! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	21578	major	core.atomic.atomicFetchSub for pointers incorrectly calls wrong function from core.internal.atomic

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "stable + druntime#3343"

PetarKirov · 2021-01-25T09:29:53Z

Updated the unit tests and targeted stable.

@rymrg thanks for following through, the commit message and target branch all look good now!

PetarKirov

Looks good to me overall, nice work!

I have one final suggestion regarding the array variables:

PetarKirov · 2021-01-25T09:35:49Z

src/core/atomic.d

+    @betterC pure nothrow @nogc unittest
+    {
+        byte[] byteArray = [1, 3, 5, 7, 9, 11, 13, 15, 17, 19];
+        ulong[] ulongArray = [2, 4, 6, 8, 10, 12, 14, 16, 19, 20];


core.atomic is meant to work with shared types. I think the functions that take non-shared arguments are for backwards compatibility with old code. Can you make these array variables shared, in order to be consistent with the general concept of this module?

Suggested change

ulong[] ulongArray = [2, 4, 6, 8, 10, 12, 14, 16, 19, 20];

shared(ulong)[] ulongArray = [2, 4, 6, 8, 10, 12, 14, 16, 19, 20];

I'm not sure. shared is still considered broken by some people. It also gets in the way of creating proper atomic variable like in C++.

I'm not sure. shared is still considered broken by some people.

The latest state of shared is that it's not broken, just incomplete. core.atomic and core.sync.mutex [1] work perfectly with shared as far as I know.

It also gets in the way of creating proper atomic variable like in C++.

Do you have a bug report? As far as I can tell it works without issues based on a quick implementation I just wrote [2]

[1]: After my PR some time ago: #1728
[2]: https://gist.github.com/PetarKirov/8590707f5a21347dc4684b77d092cf60 /
https://run.dlang.io/gist/PetarKirov/8590707f5a21347dc4684b77d092cf60?compiler=dmd&args=-main%20-unittest%20-checkaction%3Dcontext / https://run.dlang.io/is/MsuNVh

Interesting. I encountered these atomic issues when trying to implement my full version. It seems my issue stemmed from not having the shared attribute over the functions. I cannot find documentation stating that shared should be used as function attribute.
So this is a documentation issue. Does shared have any documentation other than this?

It seems my issue stemmed from not having the shared attribute over the functions.

Yes, analogous to C++ const member functions, in D a member function func must have the shared qualifier, if you want to call it on a shared object - shared(Obj) obj; obj.func().

I cannot find documentation stating that shared should be used as function attribute.

You can overload member functions on the type qualifier of this, whether it is shared, just like const, immutable and inout. After searching, the only mention of that language feature in the spec that I could find is this one: https://dlang.org/spec/class.html#member-functions. In other words, you can treat shared just like the other transitive D type qualifiers and as long as you match them on the function signatures things should work well.

core.atomic is meant to work with shared types

No, core.atomic is supposed to support shared types. Unshared has nothing to do with backwards compatibility, it's just normal testing.
I think tests should not have shared unless they are testing shared specifically. If tests wanted to be comprehensive, it could be implemented as a test matrix or something.

So should I just remove the shared versions of the test?

I wouldn't make it a primary test, because it's obscuring the thing that's being tested. But it doesn't hurt to make sure shared is also covered in tests because it's one of the combinatorial things that is often overlooked in testing, definitely should to be supported, and shared is tricky enough that it can not be assumed to work naturally without testing.

OK. I removed the shared test in the recent commit. Seeing as testing both at the same time like earlier submission caused too much confusion. How do you suggest to add the shared test?

src/core/atomic.d

PetarKirov · 2021-01-25T11:22:05Z

The error on CircleCI is:

Testing assert_fail
./generated/linux/debug/64/assert_fail  2>&1 1>/dev/null | grep -qF "success."
sed "s/\[0x[0-9a-f]*\]/\[ADDR\]/g; s/scope //g; s/Nl//g" ./generated/linux/debug/64/line_trace.output | diff line_trace.exp -
if [ ! -z "src/rt_trap_exceptions.d:8 main" ] ; then \
	 ./generated/linux/debug/64/rt_trap_exceptions  2>&1 1>/dev/null | grep -qF "src/rt_trap_exceptions.d:8 main"; \
fi
if [ ! -z  ] ; then \
	 ./generated/linux/debug/64/assert_fail  2>&1 1>/dev/null | grep -qF ; \
fi
# Use sed to canonicalize long_backtrace_trunc.output and compare against expected output in long_backtrace_trunc.exp
sed "s/\[0x[0-9a-f]*\]/\[ADDR\]/g; s/scope //g; s/Nl//g" ./generated/linux/debug/64/long_backtrace_trunc.output | diff long_backtrace_trunc.exp -
Aborted (core dumped)
rm generated/linux/debug/64/unknown_gc generated/linux/debug/64/assert_fail generated/linux/debug/64/stderr_msg generated/linux/debug/64/rt_trap_exceptions generated/linux/debug/64/static_dtor generated/linux/debug/64/future_message generated/linux/debug/64/invalid_memory_operation generated/linux/debug/64/catch_in_finally
make[2]: Leaving directory '/home/circleci/druntime/test/exceptions'
make[1]: Leaving directory '/home/circleci/druntime'
posix.mak:282: recipe for target 'unittest-debug' failed
make: *** [unittest-debug] Error 2
make: Leaving directory '/home/circleci/druntime'

Exited with code exit status 2
CircleCI received exit code 2

On CirrusCI and BuildKite we have this log output:


src/core/internal/traits.d(330): Error: static assert:  `hasElaborateCopyConstructor!(S)` is false
--
  | posix.mak:312: recipe for target 'generated/linux/debug/64/unittest/libdruntime-ut.so' failed
  | make[1]: *** [generated/linux/debug/64/unittest/libdruntime-ut.so] Error 1
  | make[1]: *** Waiting for unfinished jobs....

which doesn't make much sense to me.

Geod24 · 2021-01-25T11:31:41Z

No CircleCI has the same error:

make[2]: Entering directory '/home/circleci/druntime/test/profile'
/home/circleci/dmd/generated/linux/release/64/dmd -m64 -fPIC -w -I../../src -I../../import -Isrc -defaultlib= -debuglib= -dip1000 -L-lpthread -L-lm -L/home/circleci/druntime/generated/linux/debug/64/libdruntime.a -g -debug -profile -of./generated/linux/debug/64/profile src/profile.d
src/core/internal/traits.d(330): Error: static assert:  `hasElaborateCopyConstructor!(S)` is false
/home/circleci/dmd/generated/linux/release/64/dmd -m64 -fPIC -w -I../../src -I../../import -Isrc -defaultlib= -debuglib= -dip1000 -L-lpthread -L-lm -L/home/circleci/druntime/generated/linux/debug/64/libdruntime.a -g -debug -ofgenerated/linux/debug/64/catch_in_finally src/catch_in_finally.d
posix.mak:312: recipe for target 'generated/linux/debug/64/unittest/libdruntime-ut.so' failed
make[1]: *** [generated/linux/debug/64/unittest/libdruntime-ut.so] Error 1
make[1]: *** Waiting for unfinished jobs....

Protip: Grep for ***

TurkeyMan · 2021-01-25T13:00:35Z

src/core/atomic.d

@@ -196,7 +196,7 @@ T atomicFetchSub(MemoryOrder ms = MemoryOrder.seq, T)(ref T val, size_t mod) pur
 in (atomicValueIsProperlyAligned(val))
 {
    static if (is(T == U*, U))
-        return cast(T)core.internal.atomic.atomicFetchAdd!ms(cast(size_t*)&val, mod * U.sizeof);
+        return cast(T)core.internal.atomic.atomicFetchSub!ms(cast(size_t*)&val, mod * U.sizeof);


Oh wow... I'm sorry! That's an epic fail!

src/core/atomic.d

RazvanN7 · 2021-01-25T13:30:09Z

@rymrg Are you sure you have rebased on top of the latest stable branch? You could try:

git checkout master
git pull
git checkout $Issue_21578
git rebase --onto origin/stable origin/master

Most likely the failures are due to some mismatches between your local repo and upstream.

rymrg · 2021-01-25T13:35:34Z

@rymrg Are you sure you have rebased on top of the latest stable branch? You could try:
git checkout master
git pull
git checkout $Issue_21578
git rebase --onto origin/stable origin/master
Most likely the failures are due to some mismatches between your local repo and upstream.

Problem was forgetting tests are inside a nogc block

… calls wrong function from core.internal.atomic

rymrg requested review from ibuclaw, TurkeyMan and WalterBright as code owners January 25, 2021 09:18

dlang-bot added the Bug Fix Include reference to corresponding bugzilla issue label Jan 25, 2021

PetarKirov reviewed Jan 25, 2021

View reviewed changes

src/core/atomic.d Show resolved Hide resolved

TurkeyMan reviewed Jan 25, 2021

View reviewed changes

src/core/atomic.d Show resolved Hide resolved

Fix issue 21578 - core.atomic.atomicFetchSub for pointers incorrectly…

fbf5f52

… calls wrong function from core.internal.atomic

rymrg requested a review from TurkeyMan January 25, 2021 14:54

thewilsonator approved these changes Jan 27, 2021

View reviewed changes

thewilsonator added the auto-merge label Jan 27, 2021

dlang-bot merged commit 2d1f8ef into dlang:stable Jan 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue 21578 - core.atomic.atomicFetchSub for pointers incorrectly… #3343

Fix issue 21578 - core.atomic.atomicFetchSub for pointers incorrectly… #3343

rymrg commented Jan 25, 2021 •

edited

Loading

dlang-bot commented Jan 25, 2021

PetarKirov commented Jan 25, 2021

PetarKirov left a comment

PetarKirov Jan 25, 2021

rymrg Jan 25, 2021

PetarKirov Jan 25, 2021

rymrg Jan 25, 2021

PetarKirov Jan 25, 2021 •

edited

Loading

TurkeyMan Jan 25, 2021

rymrg Jan 25, 2021

TurkeyMan Jan 25, 2021

rymrg Jan 25, 2021

PetarKirov commented Jan 25, 2021

Geod24 commented Jan 25, 2021

TurkeyMan Jan 25, 2021

RazvanN7 commented Jan 25, 2021

rymrg commented Jan 25, 2021 •

edited

Loading

	ulong[] ulongArray = [2, 4, 6, 8, 10, 12, 14, 16, 19, 20];
	shared(ulong)[] ulongArray = [2, 4, 6, 8, 10, 12, 14, 16, 19, 20];

Fix issue 21578 - core.atomic.atomicFetchSub for pointers incorrectly… #3343

Fix issue 21578 - core.atomic.atomicFetchSub for pointers incorrectly… #3343

Conversation

rymrg commented Jan 25, 2021 • edited Loading

dlang-bot commented Jan 25, 2021

Bugzilla references

Testing this PR locally

PetarKirov commented Jan 25, 2021

PetarKirov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PetarKirov Jan 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PetarKirov commented Jan 25, 2021

Geod24 commented Jan 25, 2021

Choose a reason for hiding this comment

RazvanN7 commented Jan 25, 2021

rymrg commented Jan 25, 2021 • edited Loading

rymrg commented Jan 25, 2021 •

edited

Loading

PetarKirov Jan 25, 2021 •

edited

Loading

rymrg commented Jan 25, 2021 •

edited

Loading