This is an archive of the discontinued LLVM Phabricator instance.

[sanitizer][MIPS] Implement clone for MIPS
ClosedPublic

Authored by slthakur on Mar 13 2015, 5:53 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

slthakur updated this revision to Diff 21922.Mar 13 2015, 5:53 AM
slthakur retitled this revision from to [LSan][MIPS] Implement clone for MIPS.
slthakur updated this object.
slthakur edited the test plan for this revision. (Show Details)
slthakur added reviewers: earthdok, kcc, samsonov.
slthakur set the repository for this revision to rL LLVM.
slthakur retitled this revision from [LSan][MIPS] Implement clone for MIPS to [sanitizer][MIPS] Implement clone for MIPS.Mar 13 2015, 5:59 AM
slthakur updated this revision to Diff 22249.Mar 19 2015, 3:59 AM

Changed comment "/* Call _exit(%rax). */" to "/* Call _exit($v0). */"

Hi all,

This patch is important for LSan to work on MIPS64. Without this patch child task in LSan is not able to return control to parent task and puts LSan in infinite wait state.
Are there any comments or any changes required to this patch ?

earthdok edited edge metadata.Apr 2 2015, 9:25 AM

Hi, sorry for the delay.

It may be worth adding a note that we don't have proper CFI info here either (same as in x86_64 version). Clang probably supports all the necessary directives now, but it's a lot of code (at least in the mips case) for very marginal benefits, so probably not worth doing.

Note also that the glibc implementation appears to have a few differences between 32 and 64 bit mips, so please double check those to make sure they don't affect us.

Otherwise lgtm.

slthakur updated this revision to Diff 23324.Apr 7 2015, 6:08 AM
slthakur edited edge metadata.
  • Added note about CFI directives as suggested by @earthdok.
  • Added .cprestore directive in assembly which saves gp register across function call.

Hi @earthdok,

Note also that the glibc implementation appears to have a few differences between 32 and 64 bit mips, so please double check those to make sure they don't affect us.

The only difference between 32-bit and 64-bit in glibc implementation I can see is argument passing to clone syscall. This patch was written according to n64 abi of mips. In case of n64 abi we have argument registers a0 to a5 for syscalls. Therefore this patch will work fine for LSan 64-bit on Mips.

Hi all,

May I go ahead and commit this patch now?

In D8318#154546, @Sagar wrote:

Hi @earthdok,

Note also that the glibc implementation appears to have a few differences between 32 and 64 bit mips, so please double check those to make sure they don't affect us.

The only difference between 32-bit and 64-bit in glibc implementation I can see is argument passing to clone syscall. This patch was written according to n64 abi of mips. In case of n64 abi we have argument registers a0 to a5 for syscalls. Therefore this patch will work fine for LSan 64-bit on Mips.

If this code is correct only on MIPS64, then please put it under an appropriate #ifdef such as __mips64.

slthakur updated this revision to Diff 24000.Apr 19 2015, 10:31 PM

The code is correct only on MIPS64, therefore we put it under #ifdef(__mips64) as suggested by @earthdok.

earthdok accepted this revision.Apr 21 2015, 7:08 AM
earthdok edited edge metadata.

lgtm

This revision is now accepted and ready to land.Apr 21 2015, 7:08 AM
slthakur updated this revision to Diff 24463.Apr 27 2015, 3:10 AM
slthakur edited edge metadata.

Added code to make internal_clone work on 32-bit also.
ASan 32-bit on mips requires internal_clone (). Therefore added in this patch only to avoid breaking ASan 32-bit on mips.

Stoptheworld isn't actually used on 32-bit platforms, and that's the only thing you need clone for. But since you already have the implementation, let's land it. rslgtm

This revision was automatically updated to reflect the committed changes.