Page MenuHomePhabricator

[msan] Refactor memory layout specification and setup
ClosedPublic

Authored by eugenis on Jan 23 2015, 7:32 AM.

Details

Summary

A flexible way of describing MSan memory layout details on various platforms. No significant functional changes, but the memory layout description that you get at verbosity=1 looks slightly different. This change includes stronger sanity checks than before.

The goal of this change is to allow more than 2 application memory ranges for https://code.google.com/p/memory-sanitizer/issues/detail?id=76.

I don't have an easy way to verify that I did not break freebsd or mips, though I tried not too. I'd appreciate if you verified that layout descriptions for those platforms are accurate.

Diff Detail

Event Timeline

eugenis updated this revision to Diff 18677.Jan 23 2015, 7:32 AM
eugenis retitled this revision from to [msan] Refactor memory layout specification and setup.
eugenis updated this object.
eugenis edited the test plan for this revision. (Show Details)
eugenis set the repository for this revision to rL LLVM.
eugenis added a subscriber: Unknown Object (MLST).
eugenis updated this revision to Diff 18745.Jan 26 2015, 1:12 AM

Added #pragma unroll to addr_is_type. LLVM stops unrolling this function when the loop size reaches ~ 8, and ends up with dozens of memory loads instead of 3 or 4 constant comparisons.

Layout descriptions are correct, no issues for mips.

glider added a subscriber: glider.Jan 27 2015, 3:05 AM

Looks mostly good.

lib/msan/CMakeLists.txt
18 ↗(On Diff #18745)

Please add a comment listing the concrete pragmae you're suppressing.

lib/msan/msan.h
72–79

Please add a comment about the restrictions that led to these particular memory layouts on each platform (like it's done for FreeBSD)

93

Please add a comment describing the necessity of unrolling this loop.
You may want to add something GCC-specific under an #ifdef (e.g. attribute((optimize("unroll-loops"))) or "pragma GCC unroll-loops")
(see http://stackoverflow.com/questions/4071690/tell-gcc-to-specifically-unroll-a-loop)

lib/msan/msan_linux.cc
111

Maybe also assert that "end - start" is sane (e.g. not less than a page).
I can imagine accidental zero-sized ranges.

113–125

Can this loop be joined with the loop above?

eugenis updated this revision to Diff 18808.Jan 27 2015, 4:56 AM
eugenis edited edge metadata.
eugenis removed rL LLVM as the repository for this revision.
eugenis added inline comments.
lib/msan/CMakeLists.txt
18 ↗(On Diff #18745)

Now that I've added gcc pragmas, this is not needed.

lib/msan/msan.h
72–79

done for linux/x86_64

93

done

lib/msan/msan_linux.cc
111

done

113–125

I'd prefer not to. They do different things, even though they iterate over the same array.
I've even moved the first loop to a separate function now.

glider accepted this revision.Jan 27 2015, 5:13 AM
glider added a reviewer: glider.

LGTM with a nit.

lib/msan/msan.h
39

Please add a "TODO(someone): add description" or ask Mohit for the description.

This revision is now accepted and ready to land.Jan 27 2015, 5:13 AM

Thanks, r227192.

lib/msan/msan.h
39

Committed version include a very short comment in the mips section.

eugenis closed this revision.Feb 16 2015, 12:37 AM