Page MenuHomePhabricator

[UBSan] Adding support of MIPS32
ClosedPublic

Authored by slthakur on Aug 13 2014, 4:06 AM.

Details

Summary

Changed files:
config-ix.cmake: Enabled UBSan for MIPS32
sanitizer_stacktrace.cc: Program counter for MIPS32 is four byte aligned and a delay slot so subtracted PC by 8 for getting call site address.
cast-overflow.cpp: Added big endian support for this test case.

Diff Detail

Repository
rL LLVM

Event Timeline

slthakur updated this revision to Diff 12441.Aug 13 2014, 4:06 AM
slthakur retitled this revision from to [UBSan] Adding support of MIPS32.
slthakur updated this object.
slthakur edited the test plan for this revision. (Show Details)
slthakur added reviewers: kcc, rsmith, dsanders, petarj.
slthakur set the repository for this revision to rL LLVM.
slthakur added subscribers: farazs, sdkie.
slthakur added a subscriber: Unknown Object (MLST).Aug 13 2014, 9:58 PM
petarj added inline comments.Aug 25 2014, 7:50 AM
lib/sanitizer_common/sanitizer_stacktrace.cc
26 ↗(On Diff #12441)

While not the subject of this code review, I have to say that the name of this function seems to be wrong. (otherwise, it needs some explanation in a form of a code comment)
From what I read in the rest of the code, this is used to convert a return-address value into a call-site address, right?
If that is correct, I am puzzled with the ARM part which does nothing for non-thumb addresses.
Further, (if my understanding is still correct,) MIPS should follow SPARC case for MIPS32r1/r2, as there is a delay slot to be taken into account.
Last, just a comment that this code is likely to become more complex with compact branches in MIPS32r6, microMIPS, etc.

kcc accepted this revision.Aug 25 2014, 11:35 AM
kcc edited edge metadata.

LGTM

This revision is now accepted and ready to land.Aug 25 2014, 11:35 AM
slthakur updated this revision to Diff 13195.Sep 3 2014, 4:19 AM
slthakur edited the test plan for this revision. (Show Details)
slthakur edited edge metadata.

Changed files:
sanitizer_stacktrace.cc: Considering the delay slot in MIPS32 PC should be subtracted by 8.
ubsan_handlers.cc: __ubsan_handle_float_cast_overflow should give callers PC to handleFloatCastOverflow to print the correct location. Else it prints the location of call site to be in ubsan_handlers.cc itself.
cast-overflow.cpp: Added support for big endian machines to cast-overflow test case.

slthakur updated this revision to Diff 13198.Sep 3 2014, 4:38 AM
kcc added inline comments.Sep 4 2014, 11:20 AM
lib/ubsan/ubsan_handlers.cc
273 ↗(On Diff #13198)

please add spaces around '='

kcc added a comment.Sep 4 2014, 11:21 AM

Also, I'd prefer samsonov@ to review ubsan-related changes. (Somehow I can't CC him here...)

samsonov added inline comments.
lib/ubsan/ubsan_handlers.cc
265 ↗(On Diff #13198)

Just use Opts.pc instead of Loc here.

273 ↗(On Diff #13198)

You don't need this line, caller's PC is already collected in GET_REPORT_OPTIONS() macro.

slthakur updated this revision to Diff 14036.Sep 24 2014, 6:04 AM
slthakur updated this object.
slthakur set the repository for this revision to rL LLVM.

Updated patch as per suggestion by samsonov.

petarj added inline comments.Sep 24 2014, 6:48 AM
test/ubsan/TestCases/Float/cast-overflow.cpp
57 ↗(On Diff #14036)

Why is this value different than the one for little endian?

samsonov accepted this revision.Sep 24 2014, 3:44 PM
samsonov added a reviewer: samsonov.

LGTM. Thanks!

Note that we don't have a MIPS bot, so we can't guarantee we won't accidentally break this arch.

slthakur updated this revision to Diff 14064.Sep 24 2014, 11:52 PM
slthakur edited edge metadata.
slthakur set the repository for this revision to rL LLVM.

Updated patch as there was a mistake in the value of NaN for big endian.

petarj accepted this revision.Sep 25 2014, 2:28 PM
petarj edited edge metadata.

LGTM

petarj added inline comments.Sep 25 2014, 2:30 PM
test/ubsan/TestCases/Float/cast-overflow.cpp
20 ↗(On Diff #14064)

nit: put this #include before #include <stdint.h>

slthakur updated this revision to Diff 14095.Sep 25 2014, 11:03 PM
slthakur edited edge metadata.
slthakur set the repository for this revision to rL LLVM.

Moved #include <endian.h> before #include <stdint.h>

Great. Do you need someone to commit this for you?

dsanders edited edge metadata.Sep 26 2014, 6:19 AM

Note that we don't have a MIPS bot, so we can't guarantee we won't accidentally break this arch.

I'm in the process of sorting out some kit for new buildbots. I'll add a sanitizer buildslave to the todo list.

This comment was removed by slthakur.
lib/sanitizer_common/sanitizer_stacktrace.cc
26 ↗(On Diff #12441)

I had not considered the delay slot. I have updated it in the new revision.

In D4881#36, @petarj wrote:

Great. Do you need someone to commit this for you?

Yes, I don't have commit access.

petarj closed this revision.Sep 26 2014, 7:26 AM
petarj updated this revision to Diff 14119.

Closed by commit rL218519 (authored by @petarj).