Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Message sends to nil return structs return garbage values. #1365

Open
Tracked by #2838
aballway opened this issue Nov 11, 2016 · 6 comments
Open
Tracked by #2838

Message sends to nil return structs return garbage values. #1365

aballway opened this issue Nov 11, 2016 · 6 comments

Comments

@aballway
Copy link
Contributor

No description provided.

@DHowett-MSFT
Copy link

DHowett-MSFT commented Nov 14, 2016

Specifically: message sends to nil which return floats or uniform float aggregates will return garbage values on ARM.

Root Cause
objc_msgSend zeroes out r0 and r1 if r0 is nil, but it does nothing for d0-d3.

Fix

diff --git a/msvc/objc_msgSend.arm.asm b/msvc/objc_msgSend.arm.asm
index 3b29c96..90e95ed 100644
--- a/msvc/objc_msgSend.arm.asm
+++ b/msvc/objc_msgSend.arm.asm
@@ -89,6 +89,10 @@
 #line DBG_SKIP
     movs  r1, #0
 #line DBG_SKIP
+    ldr r2, =LZeroVFPRegisters
+#line DBG_SKIP
+    vldm r2, {d0-d3}
+#line DBG_SKIP
     mov   ip, lr
 #line DBG_SKIP
     b     %5f
@@ -172,5 +176,7 @@
     EXTERN SmallObjectClasses
 LSmallIntClass DCD SmallObjectClasses

+LZeroVFPRegisters DCD 0, 0, 0, 0
+
 #line DBG_SKIP
     END

However, I have not had time to validate this fix.

@DHowett-MSFT
Copy link

On the reference platform, Clang emits an if(unlikely(!receiver)) { memset(&dest, 0, sizeof(dest)) } before every stret-possible message send.

@DHowett-MSFT
Copy link

Without a compiler fix, we cannot address the general case here.
We can, however, address the <=24-byte VFP aggregate case and the single floating point value cases.

Since getting a memset(0)'d struct from a nil message send is relatively new (compared to the rest of Objective-C), the real bug is that a single float or double does not get cleared on ARM. Fixing this should be sufficient for most cases.

@rajsesh
Copy link
Contributor

rajsesh commented Jan 9, 2017

@DHowett-MSFT I have a repro of this on x86 as well.

@rajsesh rajsesh changed the title Message sends to nil return structs return garbage values on ARM Message sends to nil return structs return garbage values. Jan 9, 2017
@DHowett-MSFT
Copy link

DHowett-MSFT commented Jan 9, 2017

It turns out that we can't fix this in libobjc2.

Assuming the following:

@interface X
- (T)message;
@end
...
T value = [x message];

For non-pointer-sized aggregate returns, the Mac Objective-C codegen shipped with clang emits code like the following:

T value;
static T _global_T_zeroValue_1; // zero-initialized by virtue of being static; C++ ctors called.
if (likely(nil != receiver)) {
    objc_msgSend_stret(&value, x, @selector(message));
} else {
    memcpy(&value, _global_T_zeroValue_1, sizeof(T));
}

Whereas our code generator--and, indeed, clang's Mac codegen before iOS 5.0 or 6.0--emits code like this:

T value;
if (likely(nil != receiver)) {
    objc_msgSend_stret(&value, x, @selector(message));
} else {
    T *temporary = alloca(sizeof(T)); // <- never initialized
    memcpy(&value, temporary, sizeof(T));
}
// else: nothing?

Since we never end up in objc_msgSend*, we don't get a chance to stomp the return value with nil.

For pointer-sized returns, the check is never made.

This may be a bug in CGObjCGNU. The comment here indicates that it should "fill in the 0 value".

However, it follows up by saying

  // The language spec says the result of this kind of message send is
  // undefined, but lots of people seem to have forgotten to read that
  // paragraph and insist on sending messages to nil that have structure
  // returns.  With GCC, this generates a random return value (whatever happens
  // to be on the stack / in those registers at the time) on most platforms,
  // and generates an illegal instruction trap on SPARC.  With LLVM it corrupts
  // the stack.  

As such, I'm bumping this to Area-Dev Tools and SubArea-Compiler.

@rajsesh rajsesh modified the milestones: 1703, 1702 Mar 8, 2017
@weswmsft weswmsft self-assigned this Mar 23, 2017
@rajsesh rajsesh modified the milestones: 1703, 1704 Apr 19, 2017
@davidchisnall
Copy link
Contributor

This looks like it's specific to the MS version of clang: the initialisation is correct here (which hasn't been modified upstream for 2 years). With upstream clang, this code:

struct X { int x,y,z,a,b,c; };

@interface X
- (struct X)y;
@end

int ex(X*x)
{
	[x y];
}

Generates this IR:

define i32 @ex(%0* %x) #0 {
entry:
  %retval = alloca i32, align 4
  %x.addr = alloca %0*, align 8
  %tmp = alloca %struct.X, align 4
  %null = alloca %struct.X, align 4
  ; struct X value initialised to 0 here.
  store %struct.X zeroinitializer, %struct.X* %null, align 4 
  store %0* %x, %0** %x.addr, align 8
  %0 = load %0*, %0** %x.addr, align 8
  %1 = icmp eq %0* %0, null
  br i1 %1, label %continue, label %msgSend

msgSend:                                          ; preds = %entry
  %2 = bitcast %0* %0 to i8*
  call void bitcast (i8* (i8*, ...)* @objc_msgSend_stret to void (%struct.X*, i8*, i8*)*)(%struct.X* sret %tmp, i8* %2, i8* bitcast ([2 x { i8*, i8* }]* @.objc_selector_list to i8*)), !GNUObjCMessageSend !2
  br label %continue

continue:                                         ; preds = %msgSend, %entry
  ; Using the zero-initialised version if we didn't do the message send.
  %3 = phi %struct.X* [ %tmp, %msgSend ], [ %null, %entry ] 
  %4 = load i32, i32* %retval, align 4
  ret i32 %4
}

Note, however, that the ObjC language ref makes this explicitly undefined behaviour, because NeXT considered this approach and decided it was too expensive (back in the days of 25MHz CPUs and 8MB of RAM). I added this code path to clang 6-7 years ago because it makes portable code quite tricky to write, because whether objc_msgSend or objc_msgSend_stret is called depends on the ABI and, in particular, a lot of code broke on SPARC as a result of it treating things as stret that weren't on most other platforms (and people are far more likely to complain if code works on i386 OS X and not on SPARC OpenBSD with GNUstep than vice versa).

Apple adopted it a little bit later, but I think it wasn't until OS X 10.7 or 10.8 that this behaviour became reliable, and code that expects to run with older versions of Apple's toolchain shouldn't rely on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants