Race in hb_ot_tags_from_language

[bungeman]

While rolling HarfBuzz in Chromium at https://chromium-review.googlesource.com/c/chromium/src/+/3668368 ThreadSanitizer detected a race which appears to be around hb_ot_tags_from_language. Still investigating.

[bungeman]

The race appears to be on last_tag_idx introduced in dfca47f [ot-tag] Cache last bsearch result

[bungeman]

I see that the intent here is to read from last_tag_idx exactly once to get a guess (and that guess may be anything, even uninitialized is fine) and write to it exactly once to store the next guess. However, in the absence of any barriers or volatile, there is nothing currently preventing the compiler from optimizing away last_tag_idx completely, or worse read from it multiple times, since in the absence of any annotation the compiler may assume single threaded execution and optimize as it wishes.

[behdad]

Thanks. I'll make it an atomic and use get/set_relaxed on it, which should satisfy thread-sanitizer.

[behdad]

That said, that optimization was a bit misguided. I'll revert it.

[behdad]

Let me measure and do one or the other.

[behdad]

I see that the intent here is to read from last_tag_idx exactly once to get a guess (and that guess may be anything, even uninitialized is fine) and write to it exactly once to store the next guess. However, in the absence of any barriers or volatile, there is nothing currently preventing the compiler from optimizing away last_tag_idx completely, or worse read from it multiple times, since in the absence of any annotation the compiler may assume single threaded execution and optimize as it wishes.

It's a static variable. It doesn't make any assumption on multiple threaded execution for effectiveness.

[behdad]

@bungeman Can you see if the commit I just pished fixes the sanitizer issue? Thanks.

[behdad]

Weird, I cannot reproduce this locally with our multithreaded test no matter how I try.

[bungeman]

At https://chromium-review.googlesource.com/c/chromium/src/+/3668368/3 attempting to roll HarfBuzz in Chromium to the commit to use atomic ints (before the change was reverted) to see if this alleviates the issue. If this lands it will be interesting to attempt to roll to the revert and see if it re-introduces the issue.

[behdad]

The revert was on a PR just to test.

[behdad]

The revert was on a PR just to test.

Actually I didn't mean the revert to go to main. But I see it did. That was not intentional. I'll revert the revert. Ouch.

[behdad]

I've confirmed locally using tsan that the atomic ops fix the issue.

[bungeman]

Thanks for updating the tests and reproducing. Can confirm that the roll of HarfBuzz into Chromium went well when rolling to the initial commit with the fix. After attempting a further roll ran into different issues. Will investigate and potentially open a different issue for that.