[PATCH] D137213: [clang][modules] NFCI: Pragma diagnostic mappings: write/read FileID instead of SourceLocation

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

I just realized @jansvoboda11 is probably out on holiday this week (IIRC, Apple usually gets this week off). Since this was committed almost a month ago, I'm guessing this isn't enough of a blocker that we need to revert rather than wait until next week (and there are some commits on top that rely on this!). But probably reverting the stack would be an option if it's critical.

In the meantime, I'll try to help.



================
Comment at: clang/lib/Serialization/ASTReader.cpp:6343
              "Invalid data, missing pragma diagnostic states");
-      SourceLocation Loc = ReadSourceLocation(F, Record[Idx++]);
-      auto IDAndOffset = SourceMgr.getDecomposedLoc(Loc);
-      assert(IDAndOffset.first.isValid() && "invalid FileID for transition");
-      assert(IDAndOffset.second == 0 && "not a start location for a FileID");
+      FileID FID = ReadFileID(F, Record, Idx);
+      assert(FID.isValid() && "invalid FileID for transition");
----------------
eaeltsin wrote:
> This doesn't work as before, likely because ReadFileID doesn't do TranslateSourceLocation.
> 
> Our tests fail.
> 
> I tried calling TranslateSourceLocation here and the tests passed:
> ```
>       SourceLocation Loc = Diag.SourceMgr->getComposedLoc(FID, 0);
>       SourceLocation Loc2 = TranslateSourceLocation(F, Loc);
>       auto IDAndOffset = SourceMgr.getDecomposedLoc(Loc2);
> 
>       // Note that we don't need to set up Parent/ParentOffset here, because
>       // we won't be changing the diagnostic state within imported FileIDs
>       // (other than perhaps appending to the main source file, which has no
>       // parent).
>       auto &F = Diag.DiagStatesByLoc.Files[IDAndOffset.first];
> ```
> 
> Sorry I don't know the codebase, so this fix is definitely ugly :) But it shows the problem.
> 
I don't think that's the issue, since `ReadFileID()` calls `TranslateFileID`, which should seems like it should be equivalent.

However, I notice that the post-increment for `Idx` got dropped! Can you try replacing the line of code with the following and see if that fixes your tests (without any extra TranslateSourceLocation logic)?
```
lang=c++
FileID FID = ReadFileID(F, Record, Idx++);
```

If so, maybe you can contribute that fix with a reduced testcase? I suggest adding me, @vsapsai, @Bigcheese, and @jansvoboda11 as reviewers.

@alexfh, maybe you can check if this fixes your tests as well?

(If this is the issue, it's a bit surprising we don't have existing tests covering this case... and embarrassing I missed it when reviewing initially!)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137213/new/

https://reviews.llvm.org/D137213