Unify COFF and ELF data structures and procedures #32478

augusto2112 · 2020-06-19T20:55:01Z

This PR unifies the MetadataSections data structure present both in ImageInspectionELF.h and ImageInspectionCOFF.h, as well as prepare it to be accessible from Swift. It also unifies the procedures which were identical under ELF and COFF in a new ImageInspectionCommon file.

@vedantk @compnerd

stdlib/public/runtime/ImageInspectionCOFF.h

compnerd · 2020-06-19T21:03:52Z

@swift-ci please test Windows platform

stdlib/public/runtime/SwiftRT-COFF.cpp

compnerd · 2020-06-19T21:32:51Z

@swift-ci please test Windows platform

compnerd · 2020-06-19T21:37:31Z

@swift-ci please test Windows platform

compnerd · 2020-06-19T21:57:52Z

@swift-ci please test Windows platform

compnerd · 2020-06-19T21:59:57Z

@swift-ci please test windows platform

compnerd · 2020-06-19T22:32:40Z

@swift-ci please test Linux platform

stdlib/public/runtime/SwiftRT-COFF.cpp

compnerd · 2020-06-19T23:47:12Z

@swift-ci please test

compnerd · 2020-06-19T23:47:51Z

@swift-ci please test Windows platform

vedantk

I think this is in great shape!

edit: I have some questions, but I don't intend them to be blocking.

include/swift/Reflection/ReflectionContext.h

stdlib/public/runtime/ImageInspectionCommon.h

swift-ci · 2020-06-20T00:17:57Z

Build failed
Swift Test OS X Platform
Git Sha - 8279a8310ed0b508b6de7c38f453dc2f3620acf3

compnerd · 2020-06-20T18:39:23Z

@swift-ci please test

compnerd · 2020-06-20T18:41:10Z

@swift-ci please test Windows platform

swift-ci · 2020-06-20T20:03:35Z

Build failed
Swift Test OS X Platform
Git Sha - 29ffee6e72add1aa43c16528099d120cf524aca6

gottesmm

Some comments about style. Question. Would it make more sense to do this refactor in two PRs. The first that refactors one of ELF/COFF to the new world (which ever is easer) and then a second PR that changes the 2nd to use the new world? That will make it easier to review and smaller.

stdlib/public/SwiftShims/MetadataSections.h

stdlib/public/runtime/ImageInspectionCommon.cpp

augusto2112 · 2020-06-22T12:32:04Z

Some comments about style. Question. Would it make more sense to do this refactor in two PRs. The first that refactors one of ELF/COFF to the new world (which ever is easer) and then a second PR that changes the 2nd to use the new world? That will make it easier to review and smaller.

@gottesmm I opened a PR which only moves the ELF MetadataSections here, so if you guys think that makes more sense to the project, that's fine by me. I think it ended up being very small, since the majority of this PR is moving the functions from ImageInspectionCOFF.cpp and ImageInspectionELF.cpp to ImageInspectionCommon.cpp, and it didn't make sense to include those changes since we don't have common routines in that context.

Switching subjects: this PR failed the mac tests during the weekend, but I believe the reason is unrelated to the PR itself. I found two other PRs which failed in the exact same test here and here. The second PR passed with the please clean test os x platform command, so maybe we should run that in whichever PR we choose to focus on first.

vedantk · 2020-06-23T16:12:51Z

@gottesmm Does swift have some guidelines on how to split up a PR? This one (second in the stack) is based on master, so it looks as complicated as the combined changes. Should follow up PRs in a stack be based off the developer topic branch to create a patch stack, or should commits just be split up within a single PR?

adrian-prantl · 2020-06-23T17:46:35Z

stdlib/public/SwiftShims/MetadataSections.h

+  __swift_size_t length;
+} MetadataSectionRange;
+
+struct MetadataSections {


Can you please add doxygen comments to each type declaration (and any non-obvious members)?

I added a doxygen comment for each declaration, although I'm not sure if they're very useful right now.

@compnerd since you wrote these data structures, do you think I should add something more to the documentation?

Minor nit: the doxygen comments in this repository uses /// for the comment leader.
I would describe them as:

/// Specifies the address range corresponding to a section.

and

/// Identifies the address space ranges for the Swift metadata required by the Swift runtime.

The two fields that are are "non-obvious" are next and prev:

/// `next` and `prev` are used by the runtime to construct a /// circularly doubly linked list to quickly iterate over the metadata /// from each image loaded into the address space. These are invasive /// to enable the runtime registration, which occurs at image load time, to /// be allocation-free as it is invoked from an image constructor function /// context where the system may not yet be ready to perform allocations. /// Additionally, avoiding the allocation enables a fast load operation, which /// directly impacts application load time.

augusto2112 · 2020-06-29T19:28:43Z

@vedantk @adrian-prantl @compnerd I've rebased on top of the merge of the previous PR. Could one of you start the tests please? :)

adrian-prantl · 2020-06-29T19:47:05Z

@swift-ci test

swift-ci · 2020-06-29T19:49:19Z

Build failed
Swift Test Linux Platform
Git Sha - 29ffee6e72add1aa43c16528099d120cf524aca6

swift-ci · 2020-06-29T19:49:40Z

Build failed
Swift Test OS X Platform
Git Sha - 29ffee6e72add1aa43c16528099d120cf524aca6

augusto2112 · 2020-06-29T19:59:49Z

@vedantk @adrian-prantl @compnerd The build failed in both macOS and Linux with a "Wrong sha" message. I'm unsure what this means, would that be the version of one of the other sub-projects?

adrian-prantl · 2020-06-29T20:07:18Z

I think it (or someone) relaunched it afterwards. Let me know if you need me to restart it again.

compnerd · 2020-06-29T21:25:24Z

@swift-ci please test Windows platform

…FF procedures

augusto2112 · 2020-06-29T22:38:10Z

@vedantk @adrian-prantl @compnerd All tests have passed, but I had to update the comment describing MetadataSections.

Do we re-run the tests?

vedantk · 2020-06-29T23:43:41Z

@swift-ci test

swift-ci · 2020-06-29T23:44:08Z

Build failed
Swift Test Linux Platform
Git Sha - 9e83aa6e106850e77172256a5d013ec8a5c46d92

swift-ci · 2020-06-29T23:46:13Z

Build failed
Swift Test OS X Platform
Git Sha - 9e83aa6e106850e77172256a5d013ec8a5c46d92

vedantk · 2020-06-30T00:23:24Z

@swift-ci smoke test

augusto2112 · 2020-06-30T09:58:18Z

It seems like the linux test failed to build SwiftPM:

21:56:33 --- bootstrap: note: Bootstrap with the integrated Swift driver
21:56:33 --- bootstrap: note: Building SwiftPM (with swift-build)
21:56:37 Fatal error: unableToFind(tool: "swift"): file /home/buildnode/jenkins/workspace/swift-PR-Linux/branch-master/swiftpm/Sources/Build/ManifestBuilder.swift, line 243

I think this is unrelated to the tests themselves.

compnerd · 2020-06-30T14:49:54Z

@swift-ci please smoke test Linux platform

compnerd

This definitely improves things. Minor cleanups for the future include adding a missing newline on the new header and possibly collapsing the const cast into the reinterpret cast. However, neither one of those cleanups is valuable enough to hold off on this change.

augusto2112 · 2020-07-02T18:25:26Z

This definitely improves things. Minor cleanups for the future include adding a missing newline on the new header and possibly collapsing the const cast into the reinterpret cast. However, neither one of those cleanups is valuable enough to hold off on this change.

@compnerd I've added those changes in #32339

compnerd reviewed Jun 19, 2020

View reviewed changes

stdlib/public/runtime/ImageInspectionCOFF.h Outdated Show resolved Hide resolved

augusto2112 force-pushed the unify_metadata_sections branch from 946b9f0 to 0fc7f99 Compare June 19, 2020 21:17

vedantk reviewed Jun 19, 2020

View reviewed changes

stdlib/public/runtime/SwiftRT-COFF.cpp Outdated Show resolved Hide resolved

compnerd reviewed Jun 19, 2020

View reviewed changes

stdlib/public/runtime/SwiftRT-COFF.cpp Outdated Show resolved Hide resolved

augusto2112 force-pushed the unify_metadata_sections branch from 0fc7f99 to 0288e3a Compare June 19, 2020 21:30

augusto2112 force-pushed the unify_metadata_sections branch from 0288e3a to ee2abcb Compare June 19, 2020 21:53

augusto2112 force-pushed the unify_metadata_sections branch 2 times, most recently from 4bf2415 to 8908394 Compare June 19, 2020 22:49

compnerd reviewed Jun 19, 2020

View reviewed changes

stdlib/public/runtime/SwiftRT-COFF.cpp Show resolved Hide resolved

augusto2112 force-pushed the unify_metadata_sections branch 2 times, most recently from 20b4ac0 to 8279a83 Compare June 19, 2020 23:45

vedantk reviewed Jun 20, 2020

View reviewed changes

include/swift/Reflection/ReflectionContext.h Show resolved Hide resolved

stdlib/public/runtime/ImageInspectionCommon.h Outdated Show resolved Hide resolved

augusto2112 force-pushed the unify_metadata_sections branch from 8279a83 to 29ffee6 Compare June 20, 2020 08:57

gottesmm reviewed Jun 21, 2020

View reviewed changes

augusto2112 force-pushed the unify_metadata_sections branch from 29ffee6 to 9982163 Compare June 22, 2020 12:02

augusto2112 force-pushed the unify_metadata_sections branch 2 times, most recently from b02aaef to e273628 Compare June 23, 2020 15:02

adrian-prantl reviewed Jun 23, 2020

View reviewed changes

augusto2112 force-pushed the unify_metadata_sections branch 2 times, most recently from 503d591 to 9e83aa6 Compare June 29, 2020 19:26

Unify 'MetadataSections' data structure, as well as common ELF and CO…

5cee6f3

…FF procedures

augusto2112 force-pushed the unify_metadata_sections branch from 9e83aa6 to 5cee6f3 Compare June 29, 2020 22:35

vedantk approved these changes Jun 30, 2020

View reviewed changes

compnerd approved these changes Jun 30, 2020

View reviewed changes

compnerd merged commit 7d5def9 into swiftlang:master Jun 30, 2020

Unify COFF and ELF data structures and procedures #32478

Unify COFF and ELF data structures and procedures #32478

Uh oh!

Conversation

augusto2112 commented Jun 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

compnerd commented Jun 19, 2020

Uh oh!

Uh oh!

Uh oh!

compnerd commented Jun 19, 2020

Uh oh!

compnerd commented Jun 19, 2020

Uh oh!

compnerd commented Jun 19, 2020

Uh oh!

compnerd commented Jun 19, 2020

Uh oh!

compnerd commented Jun 19, 2020

Uh oh!

Uh oh!

compnerd commented Jun 19, 2020

Uh oh!

compnerd commented Jun 19, 2020

Uh oh!

vedantk left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

swift-ci commented Jun 20, 2020

Uh oh!

compnerd commented Jun 20, 2020

Uh oh!

compnerd commented Jun 20, 2020

Uh oh!

swift-ci commented Jun 20, 2020

Uh oh!

gottesmm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

augusto2112 commented Jun 22, 2020

Uh oh!

vedantk commented Jun 23, 2020

Uh oh!

adrian-prantl Jun 23, 2020

Choose a reason for hiding this comment

Uh oh!

augusto2112 Jun 23, 2020

Choose a reason for hiding this comment

Uh oh!

compnerd Jun 29, 2020

Choose a reason for hiding this comment

Uh oh!

augusto2112 commented Jun 29, 2020

Uh oh!

adrian-prantl commented Jun 29, 2020

Uh oh!

swift-ci commented Jun 29, 2020

Uh oh!

swift-ci commented Jun 29, 2020

Uh oh!

augusto2112 commented Jun 29, 2020

Uh oh!

adrian-prantl commented Jun 29, 2020

Uh oh!

compnerd commented Jun 29, 2020

Uh oh!

augusto2112 commented Jun 29, 2020

augusto2112 commented Jun 19, 2020 •

edited

Loading

vedantk left a comment •

edited

Loading