Mark enums with non-zero discriminant as non-zero #37224

petrochenkov · 2016-10-16T22:50:40Z

eddyb

LGTM although I believe it could be simpler.

eddyb · 2016-10-16T22:58:00Z

src/librustc/ty/layout.rs

@@ -769,6 +770,7 @@ pub enum Layout {
    CEnum {
        discr: Integer,
        signed: bool,
+        non_zero: bool,


You already have the range (see rustc_trans::adt for a way to determine if a value is in the range), why would this additional field be needed?

enum E { A = -1, B = 1, }

0 is in the range, but the enum is still non-zero.
(Also, non_zero: bool fits into previous padding so it doesn't increase the size of Layout)

That's annoying. What I expect to eventually have would use 2 there, that's why I don't like non_zero: it's a special-case that makes sense mostly on pointers.
If you want to discuss the full niche optimization, I'd be down for it (IRC?).
If you don't want to change this implementation I'll r+ and take out this later (and/or switch to multi-ranges - LLVM actually supports them for !range).

I didn't plan to implement the full optimization so far.

eddyb · 2016-10-16T23:00:32Z

src/librustc/ty/layout.rs

+                                CEnum { discr: value, .. } => {
+                                    return success(RawNullablePointer {
+                                        nndiscr: discr as u64,
+                                        value: Int(value)


Maybe make this match produce the Primitive type and do the return after the match?

eddyb · 2016-10-16T23:02:05Z

src/tools/compiletest/src/main.rs

@@ -15,6 +15,8 @@
 #![feature(test)]
 #![feature(libc)]

+#![cfg_attr(stage0, feature(question_mark))]


A few people hit this, I guess it got removed by accident. Thanks!

Note that #37177 already contains this (and is in the queue)

#37202 contains it too :D
The sooner it lands the better.

eddyb · 2016-10-16T23:40:57Z

@bors r+

bors · 2016-10-16T23:40:58Z

📌 Commit 49e6b46 has been approved by eddyb

@eddyb

Mark enums with non-zero discriminant as non-zero cc rust-lang/rfcs#1230 r? @eddyb

@eddyb

Mark enums with non-zero discriminant as non-zero cc rust-lang/rfcs#1230 r? @eddyb

Rollup of 23 pull requests - Successful merges: #36964, #37108, #37117, #37124, #37161, #37176, #37182, #37193, #37198, #37202, #37208, #37218, #37221, #37224, #37230, #37231, #37233, #37236, #37240, #37254, #37257, #37265, #37267 - Failed merges: #37213, #37220, #37261

Manishearth · 2016-11-08T05:27:25Z

Btw, this caused some rather tricky issues in Servo (rust-lang/rust-bindgen#225).

IMO when changing reprs for things which may come from FFI (e.g. an enum) we should be more careful and have a cycle of warnings or a compiler note or something. Not sure how it would be implemented. (I agree that it's non ideal for C++ to abuse enums as bitflags but apparently it's a common pattern)

The only reason we could figure this out was because I knew how Rust's encoded enum debuginfo works, and I also knew that this PR had happened -- which is a rather high bar for being able to debug things like this. It's otherwise quite an opaque and nonsensical bug that feels like a compiler bug (in this case, setting foo = Some(thing) made it stay None)

cc @eddyb @nikomatsakis

nikomatsakis · 2016-11-08T20:29:15Z

Hmm. @Manishearth I've not read this PR, but I assume that the enums in question were not marked as #[repr(C)] or any such thing?

(In which case, was the "safe for C" lint not firing, or was it disabled?)

Manishearth · 2016-11-08T21:04:12Z

They were marked as repr(u8) which IIRC is equivalent?

eddyb · 2016-11-08T21:18:14Z

That's just setting the size of the enum, it's still UB to have a value other than the assigned ones.
EDIT: Not saying #[repr(C)] doesn't also have that UB property still, neither escape it.
EDIT2: Looks like gross misuse, nothing to see here.

rust-highfive assigned eddyb Oct 16, 2016

eddyb requested changes Oct 16, 2016

View reviewed changes

Mark enums with non-zero discriminant as non-zero

49e6b46

petrochenkov force-pushed the nz branch from 13b6a98 to 49e6b46 Compare October 16, 2016 23:24

bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Oct 17, 2016

eddyb mentioned this pull request Oct 18, 2016

Rollup of 16 pull requests #37262

Closed

eddyb added a commit to eddyb/rust that referenced this pull request Oct 19, 2016

Rollup merge of rust-lang#37224 - petrochenkov:nz, r=eddyb

f399259

Mark enums with non-zero discriminant as non-zero cc rust-lang/rfcs#1230 r? @eddyb

eddyb added a commit to eddyb/rust that referenced this pull request Oct 19, 2016

Rollup merge of rust-lang#37224 - petrochenkov:nz, r=eddyb

beaa4c5

Mark enums with non-zero discriminant as non-zero cc rust-lang/rfcs#1230 r? @eddyb

eddyb mentioned this pull request Oct 19, 2016

Rollup of 23 pull requests #37269

Merged

bors merged commit 49e6b46 into rust-lang:master Oct 19, 2016

eddyb mentioned this pull request Oct 30, 2016

Remove null value from Opcode bytecodealliance/cranelift#18

Closed

bholley mentioned this pull request Nov 8, 2016

Generating Rust enums for C++ enums causes NonZero-optimization footguns rust-lang/rust-bindgen#225

Closed

petrochenkov deleted the nz branch March 16, 2017 19:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mark enums with non-zero discriminant as non-zero #37224

Mark enums with non-zero discriminant as non-zero #37224

petrochenkov commented Oct 16, 2016

eddyb left a comment

eddyb Oct 16, 2016

petrochenkov Oct 16, 2016 •

edited by eddyb

Loading

eddyb Oct 16, 2016

petrochenkov Oct 16, 2016

eddyb Oct 16, 2016

petrochenkov Oct 16, 2016

eddyb Oct 16, 2016

jonas-schievink Oct 16, 2016

petrochenkov Oct 16, 2016

eddyb commented Oct 16, 2016

bors commented Oct 16, 2016

Manishearth commented Nov 8, 2016

nikomatsakis commented Nov 8, 2016 •

edited

Loading

Manishearth commented Nov 8, 2016

eddyb commented Nov 8, 2016 •

edited

Loading

Mark enums with non-zero discriminant as non-zero #37224

Mark enums with non-zero discriminant as non-zero #37224

Conversation

petrochenkov commented Oct 16, 2016

eddyb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov Oct 16, 2016 • edited by eddyb Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb commented Oct 16, 2016

bors commented Oct 16, 2016

Manishearth commented Nov 8, 2016

nikomatsakis commented Nov 8, 2016 • edited Loading

Manishearth commented Nov 8, 2016

eddyb commented Nov 8, 2016 • edited Loading

petrochenkov Oct 16, 2016 •

edited by eddyb

Loading

nikomatsakis commented Nov 8, 2016 •

edited

Loading

eddyb commented Nov 8, 2016 •

edited

Loading