This is an archive of the discontinued LLVM Phabricator instance.

[lld] fix data race in ICF.cpp
ClosedPublic

Authored by inglorion on Mar 20 2018, 4:06 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

inglorion created this revision.Mar 20 2018, 4:06 PM

ruiu informs me similar code exists in the other ports. Let's get this reviewed first and then I'll update the others.

ruiu added a comment.Mar 20 2018, 4:12 PM

Generally looking good.

lld/COFF/ICF.cpp
205–208 ↗(On Diff #139218)

This doubles the total number of computation because you call findBoundary calls twice for each iteration. I wonder if we need to parallelize this in the first place. Maybe it is better to do this with a regular for loop?

210–213 ↗(On Diff #139218)

I would allocate NumShards+1 elements for Boundaries and set Boundaries[0] to 0, so that we don't need to special-case I==0.

inglorion updated this revision to Diff 139226.Mar 20 2018, 5:03 PM

Removed double computation of boundaries and special case for I == 0.

inglorion marked 2 inline comments as done.Mar 20 2018, 5:03 PM
inglorion added inline comments.
lld/COFF/ICF.cpp
205–208 ↗(On Diff #139218)

Ah, yeah. I meant to make this call findBoundary once per shard. Fixed in new upload.

210–213 ↗(On Diff #139218)

Good idea! Done.

inglorion updated this revision to Diff 139228.Mar 20 2018, 5:04 PM
inglorion marked 2 inline comments as done.

removed log message

This passes tests and links all of chromium without tsan failures, but the resulting component_unittests.exe crashes.

ruiu accepted this revision.Mar 22 2018, 2:39 PM

LGTM

This revision is now accepted and ready to land.Mar 22 2018, 2:39 PM
inglorion updated this revision to Diff 139874.Mar 26 2018, 6:17 PM

Fixed off-by-one error: LLVM's for_each_n takes an end index, not a count

This now generates a components_unittests.exe that is the same size as without this change that works correctly. I also verified that the data race is gone on COFF. I will land this to give the bots a chance to chew on it, and then fix the same data race in the ELF linker.

This revision was automatically updated to reflect the committed changes.