Page MenuHomePhabricator

[Compiler-rt][MSan]Fix shmat testcase: Pass SHMLBA-alligned address to shmat
ClosedPublic

Authored by mohit.bhakkad on Feb 15 2016, 3:40 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

mohit.bhakkad retitled this revision from to [Compiler-rt][MSan][MIPS] Fix shmat testcase for MIPS.
mohit.bhakkad updated this object.
mohit.bhakkad added reviewers: eugenis, kcc, samsonov.
mohit.bhakkad set the repository for this revision to rL LLVM.
mohit.bhakkad added subscribers: jaydeep, slthakur.

@eugenis could you please review this.

samsonov edited edge metadata.Feb 22 2016, 10:29 AM

Instead of writing different code for x86 and MIPS, can you just make sure that you get a SHMLBA-aligned address?

const int kShmSize = 4096;
void *mapping_start = mmap(NULL, kShmSize + SHMLBA, ...);
uptr p = (uptr)mapping_start;
if (p % SHMLBA != 0) {
  p = (p + SHMLBA - 1) / SHMLBA * SHMLBA;
}
// p is now SHMLBA-aligned;
mohit.bhakkad retitled this revision from [Compiler-rt][MSan][MIPS] Fix shmat testcase for MIPS to [Compiler-rt][MSan]Fix shmat testcase: Pass SHMLBA-alligned address to shmat.
mohit.bhakkad updated this object.
mohit.bhakkad edited edge metadata.

Updated with suggested change, thanks!

samsonov added inline comments.Feb 23 2016, 9:44 AM
lib/msan/tests/msan_test.cc
1223 ↗(On Diff #48789)

Better cast the right-hand side to void*

1228 ↗(On Diff #48789)

s/4095/kShmSize - 1

1230 ↗(On Diff #48789)

Hm? Shouldn't you unmap the original mmaped region of size kShmSize + SHMLBA?

1241 ↗(On Diff #48789)

kShmSize - 1?

Updated with suggested changes.

samsonov accepted this revision.Feb 24 2016, 2:42 PM
samsonov edited edge metadata.

LGTM after fixing a bug below.

lib/msan/tests/msan_test.cc
1222 ↗(On Diff #48920)

Shouldn't this be

void *p = mapping_start;

?

Also, you can just unconditionally round up:

void *p = (void*)(((unsigned long)mapping_start + SHMLBA - 1) / SHMLBA * SHMLBA);
This revision is now accepted and ready to land.Feb 24 2016, 2:42 PM
mohit.bhakkad added inline comments.Feb 24 2016, 10:13 PM
lib/msan/tests/msan_test.cc
1222 ↗(On Diff #48920)

Thanks for pointing this out, I will just keep this assignment unconditional.

This revision was automatically updated to reflect the committed changes.