Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

macos: fix missing pthread mutex init after calloc #21

Open
wants to merge 1 commit into
base: mariadb-4.x
Choose a base branch
from
Open

macos: fix missing pthread mutex init after calloc #21

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions gcs/src/gcs_group.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ gcs_group_init (gcs_group_t* group, gu::Config* const cnf, gcache_t* const cache
int const appl_proto_ver)
{
// here we also create default node instance.
new (&group->memb_mtx_) gu::Mutex(NULL);
Copy link

Choose a reason for hiding this comment

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

This reminds me of the recent MDEV-34625 and MariaDB/server#3408. I understood that macOS would use a clang based compiler by default.

I think that use of placement new after a call to a calloc like operation may lead to surprises when using GCC 6 or later, if the constructor is expecting that some fields were already initialized by a previous write. GCC 6 and later could optimize away such pre-constructor writes, thanks to -flifetime-dse.

That macOS (as well as AIX) are not happy with zero initialization for pthread_mutex_t bit me in MariaDB/server#3433.

Copy link
Author

Choose a reason for hiding this comment

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

This reminds me of the recent MDEV-34625 and MariaDB/server#3408. I understood that macOS would use a clang based compiler by default.

I think that use of placement new after a call to a calloc like operation may lead to surprises when using GCC 6 or later, if the constructor is expecting that some fields were already initialized by a previous write. GCC 6 and later could optimize away such pre-constructor writes, thanks to -flifetime-dse.

MacOS uses Clang yes. But MDEV-34625 IMHO is different. Moreover, the Godbolt example does not look correct - it is fine that the memset in https://gcc.godbolt.org/z/5n87z1raG is optimized out because it is expected (I suppose) that after a class constructor is called all class variables are initialized. Thus, the compiler may conclude that if we have void *buf = malloc(size S); s = new (buf) buf; is equivalent to S *s = new S;.

But it is different if you have malloc() buf size bigger than the memory that the constructor() is expected to touch:

https://gcc.godbolt.org/z/Y43YW7vKj

struct S {
  int i;     // uninitialized in consturctor
  S() {};
};

struct A {
    char a[256];
    S s;
};

int bar() {
  A *buf = (A *)malloc(sizeof(A));
  memset(buf, 0, sizeof(A));
  S* s = new(&buf->s) S;
  return s->i;
}

became call calloc in assembly or rep stosq. So memset is Not eliminated in this case and will not be eliminated in the case of gcs_core_t* core = GU_CALLOC (1, gcs_core_t); as far as sizeof(gcs_core_t) >> gcs_group_t.

it does not eliminate calloc() even if -flifetime-dse=0 (try saving *buf to external volatile var) (default is 2) (https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html).

if you still think it is not safe, I can offer that we could replace the mutex there instead of gu::Mutex memb_mtx_ we could write gu_mutex_t mutable value_ and init without calling class constructors. WDYT?

Copy link
Author

Choose a reason for hiding this comment

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

if the constructor is expecting that some fields were already initialized by a previous write.

I think its forbidden by the spec (speculating) - constructor must init all class fields

Copy link

Choose a reason for hiding this comment

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

Yes, it is UB, with no doubt. If all fields of gu::Mutex are initialized by the constructor, then there should be no issue with the GCC -flifetime-dse on any platform.

Copy link
Author

Choose a reason for hiding this comment

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

from what we can see in gu_mutex.hpp they all are init-ed

namespace gu
{
    class Mutex
    {
    public:

        Mutex (const wsrep_mutex_key_t* key) : value_()
#ifdef GU_MUTEX_DEBUG
                 , owned_()
                 , locked_()
#endif /* GU_MUTEX_DEBUG */
        {
            if (gu_mutex_init (key, &value_))
                gu_throw_fatal;
        }

    protected:

        gu_mutex_t  mutable value_;
#ifdef GU_MUTEX_DEBUG
        gu_thread_t mutable owned_;
        bool        mutable locked_;
#endif /* GU_MUTEX_DEBUG */
    };
}

group->cache = cache;
group->act_id_ = GCS_SEQNO_ILL;
group->conf_id = GCS_SEQNO_ILL;
Expand Down Expand Up @@ -185,8 +186,13 @@ gcs_group_free (gcs_group_t* group)
if (group->my_address) free ((char*)group->my_address);
delete group->vote_history;

gu::Lock lock(group->memb_mtx_);
group_nodes_free (group);
{
gu::Lock lock(group->memb_mtx_);
group_nodes_free (group);
}

// manually release memb_mtx_ after placement-new.
group->memb_mtx_.~Mutex();
}

/* Reset nodes array without breaking the statistics */
Expand Down