Fix Swift following bitstream reader API update #25845

jfbastien · 2019-06-27T23:48:09Z

Upstream change in rL364464 broke downstream Swift.

jfbastien · 2019-06-27T23:48:54Z

Looks like I might have broken something:

/s/swift/swift/stdlib/public/SwiftOnoneSupport/SwiftOnoneSupport.swift:362:11: error: Cannot pre-specialize $sSlsE20_failEarlyRangeCheck_6boundsy5IndexQz_SnyADGtF
  (0..<0)._prespecializeCollection(index: 0, range: (0..<0))
          ^

(there are a bunch of these)
But at least something builds! I'll look into what I did wrong.

jrose-apple

I don't see anything obviously wrong, though I admit to skimming a lot. The main thing is that since Swift already isn't set up to recover from these kinds of errors, ModuleFile::error() is already fatal in a good chunk of the program. So you can crash more deliberately there and simplify the code.

jrose-apple · 2019-06-27T23:49:52Z

include/swift/Serialization/BCReadingExtras.h

@@ -38,7 +38,9 @@ class BCOffsetRAII {

  ~BCOffsetRAII() {
    if (Cursor)
-      Cursor->JumpToBit(Offset);
+      if (llvm::Error Err = Cursor->JumpToBit(Offset))


I don't think this can ever fail because it's jumping to a position the cursor was previously at.

jrose-apple · 2019-06-27T23:51:23Z

tools/driver/modulewrap_main.cpp

+      }
+    } else {
+      Instance.getDiags().diagnose(SourceLoc(), diag::error_parse_input_file,
+                                   Filename, toString(MaybeRead.takeError()));


It still freaks me out that this is available in the else clause. Looks correct, though.

jrose-apple · 2019-06-27T23:52:10Z

lib/Serialization/ModuleFile.cpp

+      if (maybeRead.get() != byte)
+        return false;
+    } else {
+      // FIXME this drops the error on the floor.


I think this is correct behavior anyway. It was reading raw bytes; if it's too short, it's not a valid signature.

OK I removed the FIXME.

jrose-apple · 2019-06-28T00:06:04Z

lib/Serialization/ModuleFile.cpp

+    if (llvm::Error Err = DeclMemberTablesCursor.JumpToBit(subTableOffset)) {
+      // FIXME this drops the error on the floor.
+      consumeError(std::move(Err));
+      error();


From here through pretty much the rest of the file, it's too late to recover, so you can use fatal(std::move(Err)) instead.

I updated this and subsequent consumeError.

jrose-apple · 2019-06-28T00:10:00Z

lib/Serialization/DeserializeSIL.cpp

+  if (!maybeKind) {
+    LLVM_DEBUG(llvm::dbgs() << "Cursor advance error in readSILFunction: "
+                            << toString(maybeKind.takeError()) << "\n");
+    MF->error();


Same here and below: MF->error() is going to abort anyway, so you might as well use MF->fatal(maybeKind.takeError()).

These are all going to be a problem in non-asserts builds, right? Because the LLVM_DEBUG will be compiled out? Or is that okay because it happens to line up with the checking of cleanup being compiled out?

It will be a problem in debug, yes. I'm trying to match current behavior and / or surrounding code. I'll update as you suggest (here and elsewhere).

Sorry, I mean because the error won't be taken in release builds if you use LLVM_DEBUG.

Ah yeah, that too.

jrose-apple · 2019-06-28T00:13:33Z

lib/Serialization/Deserialization.cpp

+      cursor.advance(AF_DontPopBlockAtEnd);
+  if (!maybeNext) {
+    // FIXME this drops the error on the floor.
+    consumeError(maybeNext.takeError());


This should probably at least assert, to match the current behavior.

I made it cantFail.

That turns out to not be something we can do at the moment because it causes failures. I'll use bug-compatible old behavior.

Uh. Are you saying we have existing corrupted modules?

I haven't dug in :)

jrose-apple · 2019-06-28T00:14:11Z

lib/Serialization/Deserialization.cpp

+      DeclTypeCursor.advance(AF_DontPopBlockAtEnd);
+  if (!maybeEntry) {
+    // FIXME this drops the error on the floor.
+    consumeError(maybeEntry.takeError());


I don't think we can continue on past this, but I guess that's no worse.

jrose-apple · 2019-06-28T00:17:44Z

lib/Serialization/Deserialization.cpp

+  if (!maybeNext) {
+    // FIXME this drops the error diagnostic on the floor.
+    consumeError(maybeNext.takeError());
+    error();


fatal again, throughout this whole file wherever you would be calling error(). Besides what I mentioned above, it's noreturn, so you don't have to make a fake return value.

I've updated these.

jfbastien · 2019-06-28T23:19:11Z

include/swift/Serialization/ModuleFile.h

+    if (expected)
+      return std::move(expected.get());
+    fatal(expected.takeError());
+  }


This part is new, it simplifies a good part of this patch.

jrose-apple

Let's go with this.

jrose-apple · 2019-06-29T00:07:54Z

Expected to fail, but it should at least get to building the overlays.

@swift-ci Please smoke test

Upstream change in rL364464 broke downstream Swift.

jfbastien · 2019-06-29T14:49:23Z

Looks like building failed because master-next had new changes that I needed to pick up. I picked them up here (also messed git up a bit in the process and fixed it).

jfbastien · 2019-06-29T22:55:15Z

@swift-ci Please smoke test

jfbastien · 2019-06-30T04:01:25Z

The current build / test failures don't seem directly caused by this change.

jrose-apple · 2019-07-01T16:06:54Z

The SIL serialization one might be: "Record kind for a SIL instruction is not supported - DeserializeSIL.cpp:1006". The non-SIL serialization one is also failing in a SIL way, so that could also be related. The others almost certainly not.

jrose-apple · 2019-07-01T16:20:08Z

…but I don't see anything obviously wrong, and this still gets master-next a lot closer, so let's take it.

* Fix Swift following bitstream reader API update Upstream change in rL364464 broke downstream Swift. (cherry picked from commit 50de105) Conflicts: lib/Serialization/Deserialization.cpp lib/Serialization/ModuleFile.cpp tools/driver/modulewrap_main.cpp

jfbastien requested review from jrose-apple and bob-wilson June 27, 2019 23:48

jrose-apple reviewed Jun 28, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

jfbastien commented Jun 28, 2019

View reviewed changes

jrose-apple approved these changes Jun 29, 2019

View reviewed changes

jfbastien added 9 commits June 29, 2019 07:39

Fix Swift following bitstream reader API update

ed501b7

Upstream change in rL364464 broke downstream Swift.

format

3c8379a

Address Jordan's comments.

9c5da0e

format

37b7bc3

Don't use AAA with fatalIfUnexpected.

cb91e71

format

a3ede2b

Address more of Jordan's (offline) comments.

735e35d

format

bf0ab47

Pick up new changes in master-next, rebase.

7e87847

jfbastien force-pushed the fix-master-next-bitstream-api branch from 152128f to 7e87847 Compare June 29, 2019 15:12

This comment has been minimized.

Sign in to view

Fix other merge issue.

5ac1eae

This comment has been minimized.

Sign in to view

Fix another issue.

37691f6

jfbastien merged commit 50de105 into master-next Jul 1, 2019

jfbastien deleted the fix-master-next-bitstream-api branch July 1, 2019 17:08

pschuh added a commit to pschuh/swift that referenced this pull request Jul 12, 2019

Fix typo introduced in PR swiftlang#25845.

a78b306

pschuh added a commit that referenced this pull request Jul 12, 2019

Fix typo introduced in PR #25845. (#26103)

059736e

Fix Swift following bitstream reader API update #25845

Fix Swift following bitstream reader API update #25845

Uh oh!

Conversation

jfbastien commented Jun 27, 2019

Uh oh!

jfbastien commented Jun 27, 2019

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jfbastien Jun 28, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Jun 29, 2019

Uh oh!

jfbastien commented Jun 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

jfbastien commented Jun 29, 2019

Uh oh!

jfbastien commented Jun 30, 2019

Uh oh!

jrose-apple commented Jul 1, 2019

Uh oh!

jrose-apple commented Jul 1, 2019

Uh oh!

Uh oh!

jfbastien Jun 28, 2019 •

edited

Loading

jfbastien commented Jun 29, 2019 •

edited

Loading