This is an archive of the discontinued LLVM Phabricator instance.

[libcxx testing] Fix UB in tests for std::lock_guard
ClosedPublic

Authored by ikudrin on Jan 14 2021, 12:16 AM.

Details

Summary

If mutex::try_lock() is called in a thread that already owns the mutex, the behavior is undefined. The patch fixes the issue by creating another thread, where the call is allowed.

Diff Detail

Event Timeline

ikudrin requested review of this revision.Jan 14 2021, 12:16 AM
ikudrin created this revision.
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptJan 14 2021, 12:16 AM

I appreciate that you're trying to make this be defined behavior, but I honestly don't know why this is UB. I can't think of or find a platform where the underlying OS "trylock" routine would have UB when called on the same thread that owns the lock already. Can somebody with access to the C++ standard provide any hint to why the standard wants this to be UB?

The current code violates the C++ standard, which says (in different sections, depending on the version):

The expression m.try_lock() shall be well-formed and have the following semantics:
Requires: If m is of type mutex, timed_mutex, shared_mutex, or shared_timed_mutex, the calling thread does not own the mutex.

The particular UB depends on the implementation and is clearly triggered on our private platform, which is why we have noticed it.

ldionne requested changes to this revision.Jan 14 2021, 7:34 AM

That seems correct to me: http://eel.is/c++draft/thread.mutex#requirements.mutex.general-14

Please fix the tests in C++03 mode as suggested, though.

libcxx/test/std/thread/thread.mutex/thread.lock/thread.lock.guard/adopt_lock.pass.cpp
30–32

With do_try_lock defined after the global mutex:

void do_try_lock() {
    assert(m.try_lock() == false);
}

This is annoying, but those tests need to run in C++03 mode.

This revision now requires changes to proceed.Jan 14 2021, 7:34 AM
ikudrin updated this revision to Diff 316665.Jan 14 2021, 8:04 AM
ikudrin marked an inline comment as done.
  • Updated as suggested. Thanks, @ldionne!
ldionne accepted this revision.Jan 14 2021, 12:40 PM
ldionne set the repository for this revision to rG LLVM Github Monorepo.

LGTM if CI passes. I think you need to re-upload the diff for it to trigger, since the LLVM Monorepo tag was somehow removed.

This revision is now accepted and ready to land.Jan 14 2021, 12:41 PM
ikudrin added a project: Restricted Project.Jan 14 2021, 5:23 PM
ikudrin updated this revision to Diff 316828.Jan 14 2021, 6:47 PM
ikudrin removed a project: Restricted Project.

Trying to force CI to run.

This revision was automatically updated to reflect the committed changes.