Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

Fix atomicFetchSub for pointers #3342

Closed
wants to merge 1 commit into from
Closed

Fix atomicFetchSub for pointers #3342

wants to merge 1 commit into from

Conversation

rymrg
Copy link
Contributor

@rymrg rymrg commented Jan 24, 2021

No description provided.

@dlang-bot
Copy link
Contributor

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

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

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 "master + druntime#3342"

@kinke
Copy link
Contributor

kinke commented Jan 24, 2021

Thanks! Please target stable (=> v2.095.1) and add some unittests (for both atomicFetch{Add,Sub} and testing both pointer and integer branches).

Copy link
Member

@ibuclaw ibuclaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a test to validate the change.

@PetarKirov PetarKirov added the Bug Fix Include reference to corresponding bugzilla issue label Jan 24, 2021
@rymrg
Copy link
Contributor Author

rymrg commented Jan 24, 2021

I've added the tests. I'm not familiar with git and github. How do I push this change to both stable and master?

@rymrg rymrg requested a review from ibuclaw January 24, 2021 19:30
@PetarKirov
Copy link
Member

I'm not familiar with git and github.

The D release process is as follows:

  • A dmd minor release (2.XXX.0) occurs every 2 months. It includes all changes that were merged in master until the branch-off point (2 weeks before the release).
  • Safe bug fixes can be merged to the stable branch at any time.
  • Every month, between 2 minor releases, a point release is published (2.XXX.1). It includes everything merged in stable until that moment. Usually, ldc releases are based on dmd patch releases - e.g. ldc 1.25.0 will be based off dmd 2.095.1⁺.
  • stable can be merged to master at any time.
  • During release, stable is merged to master and then master becomes the new stable.

How do I push this change to both stable and master?

You don't. You only need to make sure that your pull request gets merged into stable. After that @kinke will include it in LDC and since anyone can merge stable to master at any time, it will get included in master at some point, so you don't need to anything after your PR is merged.


To allow your PRs to be merged to stable you need to:

  1. Rebase your branch to stable
  2. Change the target branch of your pull request via the GitHub UI by clicking on the "Edit" button on the top of the pull request (near the title:
  • image
  • image

Rebasing a branch from master to stable

# 1. Make sure your git remotes are set up correctly:
# `upstream` should point to either `https://github.com/dlang/druntime.git` or `git@github.com:dlang/druntime.git`
# `origin` should point to your fork of druntime - `git@github.com:rymrg/druntime.git`
# Note that `upstream` and `origin` is just a convention - you can use whatever names you want.
# I'm just making sure we're talking about the same thing ;)
git remote -v
# The command above should print something like this:
# origin	git@github.com:rymrg/druntime.git (fetch)
# origin	git@github.com:rymrg/druntime.git (push)
# upstream	https://github.com/dlang/druntime (fetch)
# upstream	https://github.com/dlang/druntime (push)

# 2. Checkout and update your local copy of the `stable` branch:
git checkout stable
git pull --ff-only upstream stable

# 3. Update your branch to the latest master and then rebase onto stable

# Checkout the branch of this pull request:
git checkout atomic

# Make sure it's up-to-date with upstream/master
git rebase upstream/master

# Finally rebase it on stable:
git rebase --onto upstream/stable upstream/master

Once you know how your remotes are set, just the following should suffice:

git checkout atomic
git fetch upstream master stable
git rebase upstream/master
git rebase --onto upstream/stable upstream/master

Copy link
Member

@PetarKirov PetarKirov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created an issue on Bugzilla so this bugfix can be included automatically in the changelog. Please update the commit message to:

Fix issue 21578 - core.atomic.atomicFetchSub for pointers incorrectly calls wrong function from core.internal.atomic

For more info, check: https://github.com/dlang/dlang-bot#automated-references

Comment on lines +1146 to +1147
@betterC pure nothrow @nogc unittest{
shared byte* sptr = cast(byte*) 8;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@betterC pure nothrow @nogc unittest{
shared byte* sptr = cast(byte*) 8;
@betterC pure nothrow @nogc unittest
{
shared byte* sptr = cast(byte*) 8;

assert(atomicFetchAdd(sptr, 1) == cast(shared byte*) 8);
assert(atomicFetchAdd(ptr, 1) == cast(byte*) 9);
assert(sptr == cast(shared byte*)9);
assert(ptr == cast(byte*)10);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also add a test for pointers to non single byte objects. You can do it without using casts like this:

shared(ulong)[] array = [2, 4, 6, 8, 10, 12, 14, 16, 19, 20];
{
    shared ulong* ptr = &array[0];
    shared(ulong)* prevPtr = atomicFetchAdd(ptr, 3); 
    assert(prevPtr == &array[0]);
    assert(*prevPtr == 2);
    assert(*ptr == 8);
}
{
    shared ulong* ptr = &array[5];
    shared(ulong)* prevPtr = atomicFetchSub(ptr, 4); 
    assert(prevPtr == &array[5]);
    assert(*prevPtr == 12);
    assert(*ptr == 4); // https://issues.dlang.org/show_bug.cgi?id=21578
}

@rymrg rymrg closed this Jan 25, 2021
@rymrg rymrg deleted the dlang/atomic branch January 25, 2021 09:11
skoppe added a commit to symmetryinvestments/concurrency that referenced this pull request Jun 2, 2021
even on dmd there was a little copy/paste error where atomicFetchAdd
was calling atomicFetchSub...
Has been fixed in dlang/druntime#3342 but that is
very recent.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug Fix Include reference to corresponding bugzilla issue
Projects
None yet
5 participants