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

Lint for the reserved ABI of bool #46176

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@est31
Copy link
Contributor

est31 commented Nov 21, 2017

The ABI of bool is reserved, see these links:

Currently the improper_ctypes lint is inconsistent
with that position by treating bools as if they had
a specified ABI.

To fix this inconsistency, this changes treatment of
bools by the lint.

This might break some code, so possibly it has to
be phased in slowly...

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Nov 21, 2017

r? @michaelwoerister

(rust_highfive has picked a reviewer for you, use r? to override)

@est31

This comment has been minimized.

Copy link
Contributor

est31 commented Nov 21, 2017

I don't neccessarily endorse this PR myself. If the apparent inconsistency gets fixed by having bool finally specified, I would be happier.

Lint for the reserved ABI of bool
The ABI of bool is reserved, see these links:

* rust-lang/rfcs#954 (comment)
* #46156 (comment)
* rust-lang/rfcs#992

Currently the improper_ctypes lint is inconsistent
with that position by treating bools as if they had
a specified ABI.

To fix this inconsistency, this changes treatment of
bools by the lint.

This might break some code, so possibly it has to
be phased in slowly...

@est31 est31 force-pushed the est31:bool_lint branch from c0f63c9 to f3321b1 Nov 22, 2017

@hsivonen

This comment has been minimized.

Copy link
Contributor

hsivonen commented Nov 22, 2017

As someone who has (successfully, AFAICT, on multiple platforms—the ones Firefox 56+ runs on) used bool in FFI with the assumption that it has the same representation as _Bool, I'd much prefer Rust to officially guarantee that instead of saying that booleans aren't a legitimate part of FFI. In other words, I think this shouldn't be merged.

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented Nov 22, 2017

Nominating for discussion in the @rust-lang/compiler team meeting.

@hsivonen

This comment has been minimized.

Copy link
Contributor

hsivonen commented Nov 22, 2017

To elaborate: Not having a type that is equivalent to _Bool in FFI is terribly inconvenient in terms of not being able to expose an idiomatic C11 API for a Rust library directly but having to expose a legacy C API and wrap that as a C11 API on the C side. But worse, not having _Bool in FFI means that Rust can't call C11 libraries without a C-side wrapper.

As for Rust bool being the type that's equivalent to _Bool in FFI, that's already the case in practice (AFAICT), so making it not so is a needless inconvenience.

As for weird embedded C compilers having weird _Bool, I think the inconvenience should be on those who wish to link with code from such compilers instead of everyone working on normal (GCC, clang, MSVC) C compilers being inconvenienced. Rust already can't interop with all imaginable unreasonable but C-compliant C implementations due to Rust relying on two's complement representation of signed integers. Likewise, it should be OK for Rust to interop with _Bool for mainstream C only.

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Nov 22, 2017

New lints require an RFC. This kind of blurs the line.

@est31

This comment has been minimized.

Copy link
Contributor

est31 commented Nov 22, 2017

This is more a bugfix than a new lint addition or something like that ("feature change" etc). Unless I understood it wrongly, the purpose of the improper_ctypes lint is to check for types that have no defined ABI. Right now it seems to have missed the bool type!

@nagisa nagisa added the T-lang label Nov 23, 2017

@hsivonen

This comment has been minimized.

Copy link
Contributor

hsivonen commented Nov 24, 2017

When bool works in FFI in practice today on stable channel, making it not work or making the compiler complain about it would break Rust's stability promise in practice. (Even if a working stable feature works unintentionally, making it not work or making the compiler complain about it is a breaking change: Rust programmers should be able to rely on the stability of stable Rust without researching which functionality works intentionally and which funtionality works unintentionally. Additionally, I think the existing behavior of bool working in FFI with mainstream C implementations is desirable behavior.)

@est31

This comment has been minimized.

Copy link
Contributor

est31 commented Nov 24, 2017

it would break Rust's stability promise in practice

It is established practice that something can be broken if it only works due to a bug. See this search. And the current behaviour is clearly a bug: the ABI of bool is not specified yet, even though the lint assumes that it is specified.

Probably this change has to be phased in slowly, as the other breaking changes, I'll change my PR if the compiler team has deemed that they want to do the change.

@bluss

This comment has been minimized.

Copy link
Contributor

bluss commented Nov 24, 2017

What you are saying is that this is not credible as a soundness bug fix, even of a lint. I understand the not credible part. On the other hand, ffi is unsafe code zone and there are many details the compiler can not check for you there.

It should only be good if we get better at this approximate and pragmatic safety work of warning about portability and safety issues in code outside of safe rust.

@hsivonen

This comment has been minimized.

Copy link
Contributor

hsivonen commented Nov 24, 2017

There's a big difference between a theoretical break to fix a soundness bug and withdrawing a practically working behavior that's in use in deployed code over a theoretical concern.

As for FFI being unsafe, the use of a particular type in a signature is not the kind of unsafe that the compiler couldn't have checked, so I don't think FFI being unsafe makes it OK to make a breaking change.

Withdrawing a working feature because it was not specified officially seems like the C approach of blaming the programmer for relying on what wasn't specified as reliable and not like Rust's stability story. ☹️

@hsivonen

This comment has been minimized.

Copy link
Contributor

hsivonen commented Nov 24, 2017

As for it being just a lint: Either the lint is the first step towards breaking it, in which case it's not just a lint, or the existing stable behavior is going to continue to work in which case the lint would uselessly bother programmers on mainstream platforms.

A lint that warns all programmers that their FFI code might not link with C code produced on some niche platform that Rust doesn't support yet would be a gross misbalancing of presently relevant mainstream needs vs. potential future niche concerns.

@est31

This comment has been minimized.

Copy link
Contributor

est31 commented Nov 24, 2017

@hsivonen this PR is only enacting the existing policy on this issue, which is that bool has an unspecified ABI. I think you should better talk to the lang team to get the policy specified. This would be the best outcome, and then this PR can be closed.

@hsivonen

This comment has been minimized.

Copy link
Contributor

hsivonen commented Nov 24, 2017

What's the right place to discuss the policy?

@hsivonen

This comment has been minimized.

Copy link
Contributor

hsivonen commented Nov 26, 2017

What's the right place to discuss the policy?

I posted to the internals forum.

@est31

This comment has been minimized.

Copy link
Contributor

est31 commented Nov 27, 2017

@hsivonen thanks for the post!

@arielb1

This comment has been minimized.

Copy link
Contributor

arielb1 commented Nov 30, 2017

This is being discussed in the internals thread and needs more research.

@rkruppe

This comment has been minimized.

Copy link
Contributor

rkruppe commented Jan 19, 2018

Does the C standard permit platforms where _Bool is permitted to take values other than 0 or 1?

This is a vague question -- what does "take values other than" mean? As others have explained, the standard is rather strict about what values a C program may get when converting or type-punning. But what bit patterns are actually stored in memory or registers is a different matter, because the standard does not specify the ABI, just program behavior (see also this post by ubsan).

For example, (void*)0 results in a "null pointer" but that does not have to be the address zero. Likewise, AFAIK, a conforming implementation might store "boolean 0" and "boolean 1" differently (e.g., as an all-zero or all-ones byte), or permit multiple bit patterns to represent the same boolean value (e.g., any non-zero value is) and only canonicalize to integer 0 or 1 during type conversion (and if legal type punning might be happening).

None of this necessarily has to matter for Rust, if none of the platforms we care about do this (just as we don't support platforms where a byte isn't 8 bit), but it's important to remember that for our purposes we're interested in ABIs, which are not part of the language standard.

@hsivonen

This comment has been minimized.

Copy link
Contributor

hsivonen commented Jan 22, 2018

@cuviper: Thank you! Great to see that there aren't problems on platforms that Rust already supports. (As for powerpc*-*-darwin*, it's not an architecture we need to design for: While the platform still has some users, it's been abandoned by its maker years ago and Rust doesn't support the corresponding OS versions on the Intel side, either.)

@whitequark: Do I understand correctly that 1) TI C54x/C55x are supported neither by GCC nor clang and 2) TI C54x/C55x have been superseded by C6x, which is supported by GCC and can address 8-bit bytes?

@rkruppe

This comment has been minimized.

Copy link
Contributor

rkruppe commented Jan 22, 2018

regarding architectures that don't support byte level memory accesses, those have far bigger problems supporting Rust (or C++11 for that matter) than the size of bool, see nikomatsakis/rust-memory-model#43

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 22, 2018

Just to clarify: do we know of any cases where C++ bool and C's _Bool are treated differently?

@cuviper

This comment has been minimized.

Copy link
Member

cuviper commented Jan 22, 2018

FWIW, in the Itanium C++ ABI (used by many targets), 2.2 POD Data Types says:

If the base ABI specifies rules for the C99 type _Bool, then bool follows those rules. Otherwise, it has size and alignment 1.

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Jan 22, 2018

@rfcbot fcp close

Based on the survey done over the last few days, I propose to close this PR (which adds a lint based on the fact that bool is unspecified) by specifying the representation of bool:

  • bool has the same representation as the platform's _Bool type.
  • We document this, and also document that on every platform we currently support, this means that the size of bool is 1.
@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Jan 22, 2018

Team member @withoutboats has proposed to close this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rkruppe

This comment has been minimized.

Copy link
Contributor

rkruppe commented Jan 22, 2018

Why that way around and not the alternative? (Specify how bool currently behaves, and document that this matches _Bool on all platforms we support)

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Jan 22, 2018

Why that way around and not the alternative? (Specify how bool currently behaves, and document that this matches _Bool on all platforms we support)

People could come to the conclusion that they need a c_bool type for their FFI to be forward compatible with platforms we don't yet support. I think defining it as the same representation as _Bool / C++ bool makes it the least likely someone does something painful to avoid entirely hypothetical problems.

@est31

This comment has been minimized.

Copy link
Contributor

est31 commented Jan 22, 2018

@withoutboats do I understand you correctly that this "close" FCP also functions as a "merge" FCP for #46156 ?

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Jan 22, 2018

I think we should merge that PR and also somewhere (the reference?) clarify precisely that it has the same representation as _Bool.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 23, 2018

@withoutboats agreed; I was going to propose the same.

@cuviper thanks, just what I wanted to know

@est31

This comment has been minimized.

Copy link
Contributor

est31 commented Feb 1, 2018

friendly ping @jseyfried , your mark is missing.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 1, 2018

I took the libery of tagging for @jseyfried

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Feb 1, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@est31

This comment has been minimized.

Copy link
Contributor

est31 commented Feb 2, 2018

Closing in favour of #46156

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