Skip to content

Support reading binary swiftdeps files #190

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

Merged
merged 3 commits into from
Aug 6, 2020

Conversation

owenv
Copy link
Contributor

@owenv owenv commented Aug 3, 2020

A couple of notes:

  • The yaml versions of the test inputs I checked in aren't needed to run the tests, I just found them useful to verify the tests were correct (I generated them from the binary versions using swift-dependency-tool)
  • input info maps are now the only yaml the driver needs to read. Maybe we could switch those to JSON so we can drop the yaml dependency and simplify the build a little?

Also cc @belkadan who's the original author of the bitstream reader code added in the first commit. I've added you as a commit author, but let me know if you'd like a different form of attribution!

Copy link
Contributor

@belkadan belkadan left a comment

Choose a reason for hiding this comment

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

Reviewing my own old code for your benefit (but also for you to fix, heh):

let topByteIndex = upperBound >> 3
var result: UInt64 = 0
if upperBound & 7 != 0 {
let mask: UInt8 = (1 << UInt8(upperBound & 7)) &- 1
Copy link
Contributor

Choose a reason for hiding this comment

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

The cast to UInt8 shouldn't be necessary in modern Swift, which allows mixed-type casts. I don't think that has a performance impact either…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this turned out to be needed so that 7 gets inferred as Int instead of UInt8 There might still be a simpler way to write this, but I couldn't find it

}

extension Bitcode {
init(data: Data) throws {
Copy link
Contributor

Choose a reason for hiding this comment

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

One downside of this approach is that it's going to read the entire file structure into memory at once, rather than processing entry by entry. That's probably fine for now, but it'll be something worth cleaning up later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I added a fix to come back and improve this in a follow-up

owenv and others added 2 commits August 3, 2020 22:02
Originally authored by Jordan Rose
Modified to drop some printing and logging code that's not needed for the driver's use cases

Co-authored-by: Jordan Rose <[email protected]>
@owenv owenv force-pushed the binary-swiftdeps branch from f802169 to 02366d0 Compare August 4, 2020 05:10
@owenv
Copy link
Contributor Author

owenv commented Aug 4, 2020

@swift-ci test

@owenv
Copy link
Contributor Author

owenv commented Aug 4, 2020

@swift-ci test

@belkadan
Copy link
Contributor

belkadan commented Aug 4, 2020

input info maps are now the only yaml the driver needs to read. Maybe we could switch those to JSON so we can drop the yaml dependency and simplify the build a little?

Driver options and inputs are "supposed" to be stable for external devs; in particular, I'm sure CMake is generating one of these by now. (It's even documented! Because I wanted a place to point people when they kept trying to compile Swift code like C code!)

Input info maps, not output file maps. (I didn't recognize the name because my code called them "build records" or "master dependencies files".)

Copy link
Member

@DougGregor DougGregor left a comment

Choose a reason for hiding this comment

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

Excellent! Unless @belkadan has any reservations about merging this, I say go for it!

@belkadan
Copy link
Contributor

belkadan commented Aug 5, 2020

I only reviewed the parts I wrote originally but they look good to me. :-)

@owenv owenv merged commit 6b84f8b into swiftlang:master Aug 6, 2020
fengjixuchui referenced this pull request in fengjixuchui/swift-driver Aug 6, 2020
Merge pull request apple#190 from owenv/binary-swiftdeps
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants