jansvoboda11 added inline comments.
================
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");
----------------
alexfh wrote:
> dexonsmith wrote:
> > 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!)
> I've noticed the dropped `Idx` post-increment as well, but I went a step
> further and looked at the `ReadFileID` implementation, which is actually
> doing a post-increment itself, and accepts `Idx` by reference:
> ```
> FileID ReadFileID(ModuleFile &F, const RecordDataImpl &Record,
> unsigned &Idx) const {
> return TranslateFileID(F, FileID::get(Record[Idx++]));
> }
> ```
>
> Thus, it seems to be correct. But what @eaeltsin has found is actually a
> problem for us. I'm currently trying to make an isolated test case, but it's
> quite tricky (as header modules are =\). It may be the case that our build
> setup relies on something clang doesn't explicitly promises, but the fact is
> that the behavior (as observed by our build setup) has changed. I'll try to
> revert the commit for now to get us unblocked and provide a test case as soon
> as I can.
Thanks for helping out @dexonsmith, we did have the week off.
@eaeltsin @alexfhDone, are you able to provide the failing test case? I'm happy
to look into it and re-land this with a fix.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D137213/new/
https://reviews.llvm.org/D137213
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
