Page MenuHomePhabricator

[UBSan][MIPS] Fix cast-overflow tests for mips big endian.
ClosedPublic

Authored by slthakur on Jul 22 2015, 8:58 PM.

Details

Reviewers
dsanders
samsonov
Summary

This patch fixes the bug https://llvm.org/bugs/show_bug.cgi?id=24152
The float value resides in the first 4 bytes of ValueHandle for both mips and mipsel.

Diff Detail

Repository
rL LLVM

Event Timeline

slthakur updated this revision to Diff 30445.Jul 22 2015, 8:58 PM
slthakur retitled this revision from to [UBSan][MIPS] Fix cast-overflow tests for mips big endian..
slthakur updated this object.
slthakur added reviewers: samsonov, dsanders.
slthakur set the repository for this revision to rL LLVM.
slthakur updated this object.Jul 22 2015, 11:45 PM
dsanders edited edge metadata.Jul 23 2015, 2:54 AM
dsanders added a subscriber: hans.

+release manager. The code owner (Richard Smith) doesn't seem to have a phabricator account so I'll add him to to the thread shortly.

I don't know the ubsan implementation so I'm unable to review it. However, I can confirm that this fixes both reported ubsan failures in my patched 3.7.0 release build.

+code owner (Richard Smith)

Hi Richard,

Once this (or a revised version) is approved and committed, I'd like to merge this to the LLVM 3.7.0 branch. Is that ok with you?

samsonov accepted this revision.Jul 23 2015, 3:27 PM
samsonov edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Jul 23 2015, 3:27 PM

+rsmith. FWIW I'm ok with merging this patch to 3.7

hans added a comment.Jul 27 2015, 1:21 PM

+rsmith. FWIW I'm ok with merging this patch to 3.7

Ping?

slthakur closed this revision.Jul 28 2015, 1:38 AM

Committed in revision 243384

Thanks Sagar.

Richard: Sorry to ping again so soon after Hans's ping but rc2 is very soon: Ok to merge to 3.7.0?

hans added a comment.Jul 30 2015, 10:14 AM

Thanks Sagar.

Richard: Sorry to ping again so soon after Hans's ping but rc2 is very soon: Ok to merge to 3.7.0?

Richard: ping?

Hans: Is this still a candidate for merging to the release branch? I'm not sure if you count it as a bug fix or finishing a feature.

Assuming it is still a candidate:
Richard: Ping. Is it ok to merge to 3.7.0?

hans added a comment.Aug 3 2015, 9:11 AM

Hans: Is this still a candidate for merging to the release branch? I'm not sure if you count it as a bug fix or finishing a feature.

Yes, this is still fine by me for merging.

rsmith added inline comments.Aug 10 2015, 11:37 AM
lib/ubsan/ubsan_value.cc
86

This fix is incorrect, as is the original code. The problem is that the code is assuming that all big-endian targets have a 64-bit uptr type. The right fix would be to revert this change and change the big-endian code to something like:

internal_memcpy(&Value, (const char*)(&Val + 1) - 4, 4);

(That is, on BE systems, the value is in the last four bytes of Val, not the first four, no matter how wide or narrow the uptr type actually is.)

dsanders added inline comments.Aug 10 2015, 2:08 PM
lib/ubsan/ubsan_value.cc
86

That makes sense to me. I'll sort out a patch but I won't be able to test it until the morning (UK time). My big-endian machine is refusing to respond to anything more than ping and I'm going to have to physically reset it when I get to work tomorrow.

One question though: Given that we only do 32-bit MIPS releases at the moment, is the current patch 'correct enough' for this release?