Fix nested returns.

[luqmana]

Fixes #15763

[brson]

Nice fix.

[apoelstra]

I've been running this commit locally and it appears that it actually does break Tcp connections. The simple program

use std::io::net::tcp;

fn main() {
  println!("{}", tcp::TcpStream::connect("google.ca", 80).err());
}

outputs Some(Network is unreachable).

Here is an strace: https://gist.github.com/apoelstra/ea7f2e8f0cddb14ccfc1

I'll recompile without the patch and post the difference.
Edit: without the patch it succeeds: https://gist.github.com/apoelstra/b505c712acfbe75037ba

[apoelstra]

Here is an isolated test case that demonstrates the issue:

// 16 bytes
struct RawData {
    pub data: [u8, ..16]
}

// 16 bytes
struct StructuredData {
    pub data: u16,
    pub _align: i64,
}

fn addr_to_sockaddr() -> StructuredData {
    let mut storage = StructuredData { data: 0, _align: 0 };
    unsafe {
        // Try to set the fields by directly setting memory
        let raw = &mut storage as *mut _ as *mut RawData;
        for i in range(0, 16) {
            (*raw).data[i] = i as u8;
        }
    }
    // Then return it as a struct which is largely padding
    return storage;
}

fn main() {
    // Get it back
    let addr = addr_to_sockaddr();
    // Convert to raw data
    unsafe {
        let addrp = &addr as *const _ as *const RawData;
        // The padding is now zeroes!
        println!("raw data {}", (*addrp).data.as_slice());
    }
}

This should print the numbers 0 through 15, but instead the numbers 2-7 (which correspond to bytes which were padding in the StructuredData struct) are zeroed out. Before this patch, all 16 bytes were returned successfully.

This is a problem because this sort of cast lunacy is used all over the place in the glibc/BSD sockets code, and libnative wraps this pretty faithfully. This causes the test failures since the sockaddr, sockaddr_in, sockaddr_storage structs are casted and created with this type of code, and once that is fixed (by avoiding casting), there are later failures which cause packet checksums to be wrong.

Edit: replaced the test code to eliminate all the socket-related cruft.

[apoelstra]

Modifying StructuredData to have three more u16's (so that no padding is needed to get the following i64 to be aligned correctly) fixes the problem. So the bug is definitely a change in how struct padding of return values is (not) handled.

[lilyball]

FWIW the struct probably needs #[repr(C)] if it's depending on the actual layout of the fields.

[sfackler]

@kballard I don't think undefined struct layout has been implemented yet.

[lilyball]

Oh sure, I'm not saying that's the cause here, just pointing out that it needs to be done. The RFC has been accepted so it's technically incorrect today even though the compiler support isn't written yet.

[apoelstra]

Here's a neat observation:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

typedef unsigned char byte;

// 16 bytes
struct RawData {
    byte data[16];
};

// 16 bytes
struct StructuredData {
    short data;
    long _align;
};

struct StructuredData addr_to_sockaddr() {
    struct StructuredData storage = { 0 };

    // Try to set the fields by directly setting memory
    struct RawData *raw = (struct RawData *) &storage;
    memset(raw, 99, 16);

    // Then return it as a struct which is largely padding
    return storage;
}

int main(void) {
    // Get it back
    struct StructuredData addr = addr_to_sockaddr();

    // Convert to raw data
    struct RawData *addrp = (struct RawData *) &addr;

    int i;
    for(i = 0; i < 16; ++i) {
      // The padding is now zeroes!
      printf("%d, ", addrp->data[i]);
    }
    puts("");
}

when compiled with gcc or clang, this exhibits exactly the same "bug". The difference is that it's non-idiomatic to return structures by value in C, so this is not a problem I've ever encountered (even when writing low-level socket code). IIRC for this sort of stuff out-pointers are the usual C idiom.

I noticed this when trying to chase down the cause of the "no-padding copies" and finding myself surrounded by the faceless stares of LLVM intrinsics...so this gives weight to the "change the TCP code" strategy for fixing this :/.

[apoelstra]

An idea: every struct for which we do this kind of thing should have all padding explicitly named, to ensure that LLVM load instructions will copy them over. They should be repr(C) and we should have a new lint repr_c_explicit_padding set on everything(?) in liblibc. This lint would verify that structs marked repr(C) do not have any implicit padding in them.

[apoelstra]

Fixed in #16208 --- once we merge that PR this one should pass.

[lilyball]

@apoelstra Why do we need to be explicit about padding? Actually caring about the contents of the padding seems like a weird exception, rather than the normal behavior for C datatypes.

[apoelstra]

@kballard because within the socket code, struct pointers are casted into each other in such a careless way that data winds up in padding fields, depending which pointer type is used to access a region of memory.

This is definitely a "weird exceptional" case, but sadly I doubt it is unheard of in other pointer-cast-polymorphism C code, hence my solution of adding a lint for it.

[Aatch]

This change unfortunately caused a regression in stack usage (#19684), due to the significant increase in alloca's for code that wouldn't have encountered the problem this change fixes.

From the looks of it, "nested returns" aren't the problem. The problem is really that we eagerly assume that the return expression we see will actually be executed, which isn't the case.