On Mon, Feb 15, 2016 at 12:29 PM, Bernd Edlinger <bernd.edlin...@hotmail.de> wrote: > On 15/02/16 08:18, Dmitry Vyukov wrote: >> llvm tsan tests contain test.h file (probably what's called >> test_barrier.h in gcc), you can put the macro there. test.h should >> already be included into all tests. > > Hmm.. as the person who introduced test_barrer.h (well before llvm had a > test.h ;) > I must say, that although if gcc was first here, we will probably change > that to > match llvm's implementation for gcc-7. > > I would not like to add more differences here without a very good reason. > I'd say, if Dmitry sees a reason to improve the error handling in test.h, that > is a good thing, and should go into llvm's tree first. > > And independently of that I am looking at using llvm's test.h framework > instead > of gcc's test_barrier.h for gcc-7 soon. > > On 15/02/16 11:56, Tom de Vries wrote: >> On Mon, Feb 15, 2016 at 11:45 AM, Tom de Vries <tom_devr...@mentor.com> >> wrote: >>> >>> I've tried to be as clear as possible in the RFC submission that I'm not >>> certain about the cause of the failure, and that the patch is proposing a >>> fix that would make that guessed failure cause explicit. >>> >>>> Sure pthread_create can fail, as malloc and mmap. >>>> But if that is the reason for the failure it would happen >>>> just randomly, everywhere. >>>> >>>> Why do you think that only this test case shows the problem? >>>> >>> >>> As I explained in the RFC submission, my reasoning there was that the test >>> is one of the very few test cases that tests the result of pthread_create >>> and then returns 0, which causes the failure in combination with >>> dg-shouldfail. >>> >>> But thinking about it some more, even if pthread_create would fail, causing >>> the testcase to fail in execution, allowing the execution test to pass due >>> to dg-shouldfail, presumably the dg-output test would still fail in that >>> case, so my reasoning was not sound. >>> >>> So I suppose you're right, indeed the pthread_create fail hypothesis is not >>> the most logical one. >>> >>> Still, the patch is an improvement irrespective of the PR that inspired it, >>> and perhaps a lot more library calls should be checked for errors that just >>> pthread_create. >>> >>>> I think Dmitry's comment may be right on the point. >>> >>> >>> If someone proposes that as a patch for the testcase, great. I'm more that >>> willing to test that in my setup to be able to claim 'bootstrapped and >>> reg-tested on x86_64' in the submission. >>> > > No problem. PR65400 was a GCC wrong code bug, so it makes no > sense to have the same test in llvm's tree, thus we are free to fix it on > our own, as we like. > > Here is a patch that puts each value on it's own 8-byte aligned memory > location. From my experience with tsan tests, sharing shadow memory > slots between v and q or o is the most likely explanation for the occasional > inability to spot the race condition on v, thus the test case fails, because > the return code is 0, and the expected output is not found. > > > Boot-strapped/regression tested on x86_64-linux-gnu. > > OK for trunk?
I don't know whether it will fire or not, but 4-byte variables that are 8-byte aligned can still be collocated with something else. Making vars 8-byte should be safer.
