This is an archive of the discontinued LLVM Phabricator instance.

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

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

Download Raw Diff

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 added subscribers: jaydeep, mohit.bhakkad, llvm-commits.

slthakur updated this object.Jul 22 2015, 11:45 PM

+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?

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

In D11448#211119, @samsonov wrote:

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

Ping?

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?

In D11448#213361, @dsanders wrote:

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?

In D11448#216608, @dsanders wrote:

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?

fhahn mentioned this in rGe9cced27390b: Recommit "[LAA] Initial support for runtime checks with pointer selects.".Jun 17 2022, 12:07 PM

Revision Contents

Path

Size

lib/

ubsan/

ubsan_value.cc

2 lines

Diff 30445

lib/ubsan/ubsan_value.cc

Show First 20 Lines • Show All 77 Lines • ▼ Show 20 Lines	#if 0
case 16: {		case 16: {
__fp16 Value;		__fp16 Value;
internal_memcpy(&Value, &Val, 4);		internal_memcpy(&Value, &Val, 4);
return Value;		return Value;
}		}
#endif		#endif
case 32: {		case 32: {
float Value;		float Value;
#if defined(__BIG_ENDIAN__)		#if defined(__BIG_ENDIAN__) && !defined(__mips__)
		rsmithUnsubmitted Not Done Reply Inline Actions 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.) rsmith:* This fix is incorrect, as is the original code. The problem is that the code is assuming that…
		dsandersUnsubmitted Not Done Reply Inline Actions 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? dsanders: That makes sense to me. I'll sort out a patch but I won't be able to test it until the morning…
// For big endian the float value is in the second 4 bytes		// For big endian the float value is in the second 4 bytes
// instead of the first 4 bytes.		// instead of the first 4 bytes.
internal_memcpy(&Value, ((const char*)&Val)+4, 4);		internal_memcpy(&Value, ((const char*)&Val)+4, 4);
#else		#else
internal_memcpy(&Value, &Val, 4);		internal_memcpy(&Value, &Val, 4);
#endif		#endif
return Value;		return Value;
}		}
Show All 18 Lines