Stream: t-compiler

Topic: reviewer wanted for abi aggregates PR


nikomatsakis (Jan 18 2019 at 19:12, on Zulip):

So I put @Ariel Ben-Yehuda as reviewer for https://github.com/rust-lang/rust/pull/57645, but I'm not sure if they're super busy or what. I was wondering if @nagisa you might want to review it?

nagisa (Jan 18 2019 at 20:04, on Zulip):

while I do have a fairly extensive knowledge of ARM/AARCH ABIs, I have no idea how they ought to behave in presence of ZSTs and I haven’t been following the issue too closely.

nikomatsakis (Jan 18 2019 at 20:09, on Zulip):

Hmm. I wonder who might be good. @rkruppe or @Nikita Popov, perhaps? =)

nikomatsakis (Jan 18 2019 at 20:09, on Zulip):

tbh I don't claim a deep knowledge, I'm kind of going on what arielb1 wrote + a dash of "instinct"

nikomatsakis (Jan 18 2019 at 20:09, on Zulip):

though the rules seem to make logical sense, which might be a red flag :triangular_flag:

nikomatsakis (Jan 18 2019 at 20:10, on Zulip):

i.e., it seems like variable-length arrays ought to disable the homogeneous aggregate check, since they indicate data that you can't "see"

nikomatsakis (Jan 18 2019 at 20:11, on Zulip):

tbh I don't claim a deep knowledge, I'm kind of going on what arielb1 wrote + a dash of "instinct"

that said, I did add explicit tests for a variety of cases where we are matching the (experimentally detected) behavior on clang

nikomatsakis (Jan 18 2019 at 20:11, on Zulip):

so at least from a certain "black box" perspective, seems good

nikomatsakis (Jan 22 2019 at 21:42, on Zulip):

@nagisa thanks for the review BTW.

nikomatsakis (Jan 22 2019 at 21:42, on Zulip):

or at least "once over"

nikomatsakis (Jan 22 2019 at 21:44, on Zulip):

Curious to get feedback on this comment.

nagisa (Jan 22 2019 at 21:47, on Zulip):

Curious to get feedback on this comment.

I think we should just deal with pantomdata for now and leave arrays for another day

nagisa (Jan 22 2019 at 21:48, on Zulip):

FWIW another thing I’ve found out that [T; N] as passed by value is not actually representable in C currently.

nagisa (Jan 22 2019 at 21:48, on Zulip):

i.e. you cannot declare fn foo(x: [u8; 128]) in C. At all.

nikomatsakis (Jan 22 2019 at 21:49, on Zulip):

@nagisa so -- basically we filter out zero-sized, repr-rust things only?

nikomatsakis (Jan 22 2019 at 21:49, on Zulip):

and if you have e.g. a zero-sized C struct

nikomatsakis (Jan 22 2019 at 21:49, on Zulip):

we still consider the resulting thing not homogeneous

nikomatsakis (Jan 22 2019 at 21:50, on Zulip):

i.e.,

#[repr(C)] struct Foo { }

#[repr(C)] struct Bar { f: Foo, x: u8, y: u8 } // not homogeneous because `f`
nikomatsakis (Jan 22 2019 at 21:50, on Zulip):

but

nagisa (Jan 22 2019 at 21:50, on Zulip):

I guess. I’m not entirely clear what behaviour really zero sized repr(C) should imply, because C does not allow declaring zero-sized structs AFAIK.

nikomatsakis (Jan 22 2019 at 21:50, on Zulip):
struct Foo { }

#[repr(C)] struct Bar { f: Foo, x: u8, y: u8 } // homogeneous, because `Foo` is repr(rust)
nikomatsakis (Jan 22 2019 at 21:50, on Zulip):

I guess. I’m not entirely clear what behaviour really zero sized repr(C) should imply, because C does not allow declaring zero-sized structs AFAIK.

clang and gcc support them via extension

nikomatsakis (Jan 22 2019 at 21:51, on Zulip):

(and they are filtered out when considered homogeneous aggregates, afaik)

nikomatsakis (Jan 22 2019 at 21:51, on Zulip):

however, I agree they are rare and weird

nikomatsakis (Jan 22 2019 at 21:51, on Zulip):

also, C++ and C differ

nikomatsakis (Jan 22 2019 at 21:51, on Zulip):

we've traditionally "sided" with C here though

nagisa (Jan 22 2019 at 21:51, on Zulip):

okay, so in that case we should match that, I guess. Not sure how much we want to support C extensions.

nikomatsakis (Jan 22 2019 at 21:51, on Zulip):

(C++ excludes zero-sized things, recasting them as requiring at least one byte)

nikomatsakis (Jan 22 2019 at 21:51, on Zulip):

Indeed.

nikomatsakis (Jan 22 2019 at 21:52, on Zulip):

Still, I think if we're going to give it semantics, then matching gcc is good, presuming gcc is not doing something crazy.

nikomatsakis (Jan 22 2019 at 21:52, on Zulip):

The annoying thing is that we can't not give it semantics

nikomatsakis (Jan 22 2019 at 21:52, on Zulip):

i.e., we have to generate some code :)

nikomatsakis (Jan 22 2019 at 21:52, on Zulip):

but otoh I think it's kind of rare and we have room to change things here as long as we're sort of generally making more things work

nikomatsakis (Jan 22 2019 at 21:52, on Zulip):

(e.g., we're changing things right now)

nikomatsakis (Jan 22 2019 at 21:53, on Zulip):

anyway I'm definitely game to land a repr-rust-only PR to start

nikomatsakis (Jan 22 2019 at 21:53, on Zulip):

we can always expand to more cases

nikomatsakis (Jan 22 2019 at 21:53, on Zulip):

(it does seem like we need a way to declare "flexible array", though)

nagisa (Jan 22 2019 at 21:53, on Zulip):

Absolutely. That pattern is very common in C code.

nikomatsakis (Jan 22 2019 at 21:55, on Zulip):

hmm, I guess filtering down to "just repr-rust" will be a bit trickier than I thought

nikomatsakis (Jan 22 2019 at 21:56, on Zulip):

I have to add some flags or something

nikomatsakis (Jan 22 2019 at 21:56, on Zulip):

basically something like the has_vla flag but simpler

nikomatsakis (Jan 22 2019 at 21:57, on Zulip):

(because the code in question doesn't really know what types it is looking at)

nagisa (Jan 25 2019 at 16:20, on Zulip):

That PR ended up being very clean, @nikomatsakis!

nikomatsakis (Jan 25 2019 at 18:44, on Zulip):

@nagisa yes, it came out nice -- in part because we did the simple thing :)

pnkfelix (Feb 15 2019 at 12:57, on Zulip):

Hey, @nikomatsakis , can I get a quick clarification about a comment in a test from that PR ?

pnkfelix (Feb 15 2019 at 12:58, on Zulip):

in particular, I am trying to figure out the intent of the word "ignore" in "Show that homogeneous_aggregate code ignores zero-length arrays."

pnkfelix (Feb 15 2019 at 12:59, on Zulip):

I had thought the intent was that a zero-length array field would not be filtered out, at least not yet, in the homogenous aggregate check. And from what I can tell, that test is encoding my expectation.

pnkfelix (Feb 15 2019 at 12:59, on Zulip):

oh, wait, no, that test is not encoding my expectation

pnkfelix (Feb 15 2019 at 13:00, on Zulip):

(I misunderstood because the test embeds an "error message", but that is just an artifact of how we are exposing the layout for the test via #[rustc_layout(..)]

pnkfelix (Feb 15 2019 at 13:01, on Zulip):

Okay so now after re-reading the comment thread on PR #56877 I guess you did settle on filtering out all ZSTs, not just the Rust ones.

pnkfelix (Feb 15 2019 at 13:09, on Zulip):

(posted summary of my understanding in the description of newly filed issue #58487 )

nikomatsakis (Feb 15 2019 at 18:49, on Zulip):

@pnkfelix yes, this changed back and forth a few times so I can't blame you for being confused. But in my last conversation with @eddyb we settled on "just screen out ZSTs"

eddyb (Feb 15 2019 at 18:59, on Zulip):

yay zulip only took half a minute to get me here

eddyb (Feb 15 2019 at 19:01, on Zulip):

yeah the ZST case where it's T data[]; at the end of a struct is unrepresentable in Rust, and that doesn't make much sense for by-value passing anyway, so we should be free to ignore ZSTs

Last update: Nov 20 2019 at 01:10UTC