This is an archive of the discontinued LLVM Phabricator instance.

[openmp] Allow all possible argument separators in TSAN_OPTIONS

Authored by upsj on Sep 4 2020, 7:37 AM.



Currently, the parser used to tokenize the TSAN_OPTIONS in libomp uses
only spaces as separators, even though TSAN in compiler-rt supports
other separators like ':' or ','. This causes TSAN used with OpenMP to
spuriously display the following warning:

Warning: please export TSAN_OPTIONS='ignore_noninstrumented_modules=1'
to avoid false positive reports from the OpenMP runtime!

when TSAN_OPTIONS contains multiple arguments and
ignore_noninstrumented_modules follows a non-space separator.

CTest uses ':' to separate sanitizer options by default, which is
where I encountered this issue first in practice.
The documentation for other sanitizers mentions ':' as separator,
but TSAN only lists spaces, which is probably where this mismatch originated.

I wasn't sure where to best add tests for this bugfix, so I wanted to leave it at this stage until I can get some pointers in the right direction :)

Diff Detail

Event Timeline

upsj created this revision.Sep 4 2020, 7:37 AM
upsj requested review of this revision.Sep 4 2020, 7:37 AM
upsj retitled this revision from Allow all possible argument separators in TSAN_OPTIONS to [openmp] Allow all possible argument separators in TSAN_OPTIONS.Sep 4 2020, 7:39 AM
upsj edited the summary of this revision. (Show Details)Sep 4 2020, 7:41 AM
protze.joachim added a subscriber: protze.joachim.

Thanks for submitting this patch!

The archer tool has it's own test sub-directory. I think, it's best to add the test there.

protze.joachim requested changes to this revision.Sep 21 2020, 7:18 AM

Can you add a test?

This revision now requires changes to proceed.Sep 21 2020, 7:18 AM
upsj updated this revision to Diff 293411.Sep 22 2020, 4:39 AM

Sure, I just had to figure out how the LLVM test system works :) (and fix an off-by-one error in the tokenization)
I added a new test with disabled suppression and modified the default suppression to see that the tokenization works.

upsj updated this revision to Diff 293415.Sep 22 2020, 4:56 AM

Shorten test checks for clang-format

protze.joachim accepted this revision.Sep 24 2020, 4:19 AM


Can you push the patch or should I?

This revision is now accepted and ready to land.Sep 24 2020, 4:19 AM
upsj added a comment.Sep 24 2020, 4:47 AM

I don't have commit access, so you will have to do it.