Skip to content

IRGen: Differentiate between Swift.Bool and ObjCBool (3.0) #4171

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Aug 11, 2016
Merged

IRGen: Differentiate between Swift.Bool and ObjCBool (3.0) #4171

merged 2 commits into from
Aug 11, 2016

Conversation

slavapestov
Copy link
Contributor

@slavapestov slavapestov commented Aug 9, 2016 •
edited
Loading

  • Explanation: On Darwin platforms, we had no way to call a function that returned or took a parameter with the C99 _Bool type. Among other things, this caused problems for JavaScriptCore.framework.
  • Scope: Affects anyone calling these frameworks from Swift.
  • Risk: Medium, because this change touches some tricky internals.
  • Testing: New tests added, existing tests pass on various platforms.
  • Reviewed by: @jrose-apple
  • Radar: 26506458, 27365520
  • JIRA: https://bugs.swift.org/browse/SR-2084

@slavapestov
Copy link
Contributor Author

@swift-ci Please test

@slavapestov slavapestov added this to the Swift 3.0 milestone Aug 9, 2016
@slavapestov
Copy link
Contributor Author

@jrose-apple Can you please review?

@slavapestov
Copy link
Contributor Author

@jckarter Also if you could take a look it would be great!

@slavapestov slavapestov changed the title IRGen: Differentiate between Swift.Bool and ObjCBool IRGen: Differentiate between Swift.Bool and ObjCBool (3.0) Aug 9, 2016
@slavapestov slavapestov force-pushed the bool-is-a-giant-mess-on-darwin-platforms-3.0 branch from 9eb7469 to bc20577 Compare August 9, 2016 23:14
@slavapestov
Copy link
Contributor Author

@swift-ci Please test

@slavapestov slavapestov mentioned this pull request Aug 9, 2016
Merged
jrose-apple
jrose-apple reviewed Aug 9, 2016
View reviewed changes
test/Interpreter/Inputs/ObjCClasses/ObjCClasses.m
@implementation TestingBool

- (void) shouldBeTrueObjCBool: (BOOL)value {
assert(value);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should make the tests even stronger: the memory representation of value should be 1. These tests would pass today with the -1 issue.

static_assert(sizeof(BOOL) == sizeof(char), "unknown architecture");
char expected = 1;
assert(0 == memcmp(&value, &value, 1));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The IRGen tests do verify that the representation is correct, so if you don't mind I'll fix up the tests in a separate patch on master.

@jrose-apple
Copy link
Contributor

This looks solid; I'm much less worried now that I see what changed. My one concern was the weak testing, but that shouldn't block merging this. Thanks, Slava!

@tkremenek
Copy link
Member

@swift-ci smoke test os x

1 similar comment
@tkremenek
Copy link
Member

@swift-ci smoke test os x

@slavapestov slavapestov force-pushed the bool-is-a-giant-mess-on-darwin-platforms-3.0 branch from bc20577 to 4f493db Compare August 10, 2016 04:21
@slavapestov
Copy link
Contributor Author

@swift-ci Please smoke test os x

@slavapestov
Copy link
Contributor Author

@swift-ci smoke test os x

@slavapestov
Copy link
Contributor Author

@swift-ci Please test os x

1 similar comment
@slavapestov
Copy link
Contributor Author

@swift-ci Please test os x

@slavapestov slavapestov force-pushed the bool-is-a-giant-mess-on-darwin-platforms-3.0 branch from 4f493db to 50ac52a Compare August 10, 2016 18:26
@slavapestov
Copy link
Contributor Author

@swift-ci Please test OS X

1 similar comment
@slavapestov
Copy link
Contributor Author

@swift-ci Please test OS X

@slavapestov
IRGen: Differentiate between Swift.Bool and ObjCBool
9447425 
SIL already does this where necessary, except with foreign throwing
functions; this patch changes Sema and the ClangImporter to give
them an ObjCBool foreign error result type explicitly.

This fixes a problem where calls to functions taking and returning
the C99 _Bool type were miscompiled on Mac OS X x86-64, because
IRGen was conflating the Objective-C BOOL type (which is a signed
char on some platforms) and C99 _Bool (which lowers as the LLVM
i1 type).

Fixes <rdar://problem/26506458> and <rdar://problem/27365520>.
@slavapestov
Sema: Fix crash on Linux if ObjCBool type is not present
cd3f2f7
@slavapestov slavapestov force-pushed the bool-is-a-giant-mess-on-darwin-platforms-3.0 branch from 50ac52a to cd3f2f7 Compare August 10, 2016 20:49
@slavapestov
Copy link
Contributor Author

@swift-ci Please test OS X

2 similar comments
@slavapestov
Copy link
Contributor Author

@swift-ci Please test OS X

@slavapestov
Copy link
Contributor Author

@swift-ci Please test OS X

@slavapestov
Copy link
Contributor Author

@gribozavr Any ideas what this failure means? https://ci.swift.org/job/swift-PR-osx/3139/#showFailuresLink

@jrose-apple
Copy link
Contributor

Mishal's in the middle of updating the CI.

@slavapestov
Copy link
Contributor Author

@tkremenek do you mind merging this? Last few runs have only had different unrelated failures

@tkremenek tkremenek merged commit cb79976 into swiftlang:swift-3.0-branch Aug 11, 2016
@slavapestov slavapestov deleted the bool-is-a-giant-mess-on-darwin-platforms-3.0 branch August 19, 2016 05:21
@slavapestov slavapestov mentioned this pull request Aug 11, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@tkremenek tkremenek

Labels
None yet
Projects
None yet
Milestone
Swift 3.0
Development

Successfully merging this pull request may close these issues.
3 participants
@slavapestov @jrose-apple @tkremenek