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.

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

tsan does not recognize QMutex #460

Closed
ramosian-glider opened this issue Sep 1, 2015 · 12 comments
Closed

tsan does not recognize QMutex #460

ramosian-glider opened this issue Sep 1, 2015 · 12 comments

Comments

@ramosian-glider
Copy link
Member

Originally reported on Google Code with ID 53

What steps will reproduce the problem?

Compile and run the test program below.


What is the expected output? What do you see instead?

Tsan should recognize the QMutex as a synchronizing mechanism and not print a warning.
The example works like expected using ab boost:mutex instead.


What version of the product are you using? On what operating system?

llvm-3.4 on centos-6.5 x86_64


Please provide any additional information below.


    #include <QtConcurrentRun>
    #include <QMutex>
    #include <iostream>
    #include <boost/thread/mutex.hpp>

    class TestClass
    {
    public:
     void fun()
     {
      QMutexLocker autoMutex(&mutex);
      //boost::mutex::scoped_lock bautoMutex(bmutex);
      data = 1;
     }
    private:
     int data;
     QMutex mutex;
     boost::mutex bmutex;
    };

    int main()
    {
     TestClass *tc = new TestClass;

     QFuture<void> t1 = QtConcurrent::run(tc, &TestClass::fun);
     QFuture<void> t2 = QtConcurrent::run(tc, &TestClass::fun);

     t1.waitForFinished();
     t2.waitForFinished();

     return 0;
    }


Reported by benjamin.firl@wincor-nixdorf.com on 2014-03-10 13:10:29

@ramosian-glider
Copy link
Member Author

Hi,

Thanks for the report.

Does QT accept patches from community?

Please also file an issue against QT. Let's see what interest are there.


Reported by dvyukov@google.com on 2014-03-11 06:08:52

  • Labels added: Type-Enhancement
  • Labels removed: Type-Defect

@ramosian-glider
Copy link
Member Author

The Qt bug report was closed because it's "not our issue": https://bugreports.qt.io/browse/QTBUG-37402

However they do accept patches if someone is able to fix the issue in the way they
want you to...

Reported by cbparker on 2015-02-02 08:37:35

@ramosian-glider
Copy link
Member Author

Is there a way to annotate QMutex for ThreadSanitizer to understand it? If so, then
I'd add such code to Qt Base when defined(__has_feature) && __has_feature(thread_sanitizer).


If there is no such thing, and I cannot find any headers with macros I could use for
that purpose, can someone point me to the sources in TSan (in data-race-test, right?)
that need to be patched to add support for QMutex?

Reported by milian.wolff@kdab.com on 2015-07-16 16:29:43

@ramosian-glider
Copy link
Member Author

Hi milian.wolff,

Yes, tsan supports annotations for mutexes:
https://code.google.com/p/data-race-test/source/browse/trunk/dynamic_annotations/dynamic_annotations.h
They are implemented in tsan here:
http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/tsan/rtl/tsan_interface_ann.cc?view=markup

In particular you want:
AnnotateRWLockCreate(char *f, int l, uptr m)
AnnotateRWLockDestroy(char *f, int l, uptr m)
AnnotateRWLockAcquired(char *f, int l, uptr m, uptr is_w)
AnnotateRWLockReleased(char *f, int l, uptr m, uptr is_w)

f is file name (__FILE__)
l is line (__LINE__)
m is mutex address
is_w is whether mutexes is locked in write mode or in read mode

AnnotateRWLockAcquired must be called after the actual acquire.
AnnotateRWLockReleased must be called right before the mutex is actually unlocked.

Reported by dvyukov@google.com on 2015-07-16 20:46:36

@ramosian-glider
Copy link
Member Author

GCC does not seem to support the __has_feature check for TSan - is there an alternative
to detect it there?

Reported by milian.wolff@kdab.com on 2015-07-20 10:31:09

@ramosian-glider
Copy link
Member Author

The attached file works fine with clang++, but in GCC it does not work at all. Note
that I rely instead on a define (QT_SANITIZE_THREAD) to be set, instead of the __has_feature
which is also not supported by GCC.

I thought GCC and clang share the same TSan, but this here shows that apparently this
is it not the case. Is there any way to fix this also for GCC?

Reported by milian.wolff@kdab.com on 2015-07-20 12:53:52

@ramosian-glider
Copy link
Member Author

Internally at Google we define THREAD_SANITIZER macro when tsan is enabled, and check
that macro.

You can file a bug against gcc, but as far as I remember they intentionally did not
implement it.

Reported by dvyukov@google.com on 2015-07-20 18:09:29

@ramosian-glider
Copy link
Member Author

Yuri, do you know any way to detect if a sanitizer is enabled or not in gcc?

Reported by dvyukov@google.com on 2015-07-20 18:10:19

@ramosian-glider
Copy link
Member Author

OK, I can add a similar macro. But the annotations don't work there - should they? I'm
using GCC 5.1.0.

Reported by milian.wolff@kdab.com on 2015-07-21 14:26:32

@ramosian-glider
Copy link
Member Author

gcc should support these annotations as well.
Please show how you annotate, and how exactly annotation don't work (don't compile?
don't link?).

Reported by dvyukov@google.com on 2015-07-22 06:25:25

@ramosian-glider
Copy link
Member Author

Adding Project:ThreadSanitizer as part of GitHub migration.

Reported by glider@google.com on 2015-07-30 09:21:31

  • Labels added: ProjectThreadSanitizer

@dvyukov
Copy link
Contributor

dvyukov commented Dec 1, 2015

not actionable on our side

@dvyukov dvyukov closed this as completed Dec 1, 2015
ghost pushed a commit to wireshark/wireshark that referenced this issue Feb 3, 2018
QStrings are implictly shared as described at
http://doc.qt.io/qt-5/implicit-sharing.html. This is normally useful,
but RecentFileStatus is passed a QString before it does its work in a
separate thread.

Make a deep copy of the filename in order to ensure local ownership and
to avoid having to fool around with a QMutex (which might not be
recognized by ThreadSanitizer[1] or Helgrind[2]).

Remove getFilename since it was unused.

[1] google/sanitizers#460
[2] http://valgrind.org/docs/manual/hg-manual.html

Change-Id: I5b5c329505ed8c02d30043a2a6d1ded625924b9f
Reviewed-on: https://code.wireshark.org/review/25572
Reviewed-by: Gerald Combs <gerald@wireshark.org>
Petri-Dish: Gerald Combs <gerald@wireshark.org>
Tested-by: Petri Dish Buildbot
Reviewed-by: Michael Mann <mmann78@netscape.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants