[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.

Now that the behaviour change is understood, maybe it'd be useful to split the 
patch in two:

- First, this patch, plus a call to `ASTReader::getInputFile()` only for its 
side effects, to make this patch actually NFC.
- Second, committed a few days later, a patch that removes the call and adds a 
test confirming `-I` is no longer implied by `-fembed-all-input-files`.

That way, the future temporary reverts won't churn the file format. For 
example, downstreams that hit problems and need to temporarily revert can just 
revert the second patch.

(... assuming that @rsmith/others confirm that implying `-I` is undesirable... 
or, if it's desired, surely there's a more explicit way to implement it.)




Comment at: clang/lib/Serialization/ASTWriter.cpp:3013
 
-SourceLocation Loc = Diag.SourceMgr->getComposedLoc(FileIDAndFile.first, 
0);
-assert(!Loc.isInvalid() && "start loc for valid FileID is invalid");
-AddSourceLocation(Loc, Record);
+AddFileID(FileIDAndFile.first, Record);
 

This is where I'm suggesting a side-effects-only call to 
`ASTReader::getInputFile()` could be added. (Or to something that transitively 
will call it.)


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

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

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a subscriber: rsmith.
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");

dexonsmith wrote:
> alexfh wrote:
> > alexfh wrote:
> > > dexonsmith wrote:
> > > > alexfh wrote:
> > > > > alexfh wrote:
> > > > > > jansvoboda11 wrote:
> > > > > > > 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  = 
> > > > > > > > > > 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 , const RecordDataImpl ,
> > > > > > > > unsigned ) 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.
> > > > > > I've spent multiple hours trying to extract an observable test 
> > > > > > case. It turned out to be too hairy of a yaq to shave: for each 
> > > > > > compilation a separate sandboxed environment is created with a 
> > > 

[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 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:
> alexfh wrote:
> > dexonsmith wrote:
> > > alexfh wrote:
> > > > alexfh wrote:
> > > > > jansvoboda11 wrote:
> > > > > > 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  = 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 , const RecordDataImpl ,
> > > > > > > unsigned ) 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.
> > > > > I've spent multiple hours trying to extract an observable test case. 
> > > > > It turned out to be too hairy of a yaq to shave: for each compilation 
> > > > > a separate sandboxed environment is created with a separate symlink 
> > > > > tree with just the inputs necessary for that action. Some of the 
> > > > > inputs are prebuilt module files (e.g. for libc++) that are 
> > > > > version-locked with the compiler. So far @jgorbe and I could reduce 
> > > > > 

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh 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:
> > alexfh wrote:
> > > alexfh wrote:
> > > > jansvoboda11 wrote:
> > > > > 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  = 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 , const RecordDataImpl ,
> > > > > > unsigned ) 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.
> > > > I've spent multiple hours trying to extract an observable test case. It 
> > > > turned out to be too hairy of a yaq to shave: for each compilation a 
> > > > separate sandboxed environment is created with a separate symlink tree 
> > > > with just the inputs necessary for that action. Some of the inputs are 
> > > > prebuilt module files (e.g. for libc++) that are version-locked with 
> > > > the compiler. So far @jgorbe and I could reduce this to four 
> > > > compilation steps with their own symlink trees with inputs. While I 
> > > > could figure out some of the factors that affect reproducibility (for 
> > > > example, symlinks are 

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh 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");

dexonsmith wrote:
> alexfh wrote:
> > alexfh wrote:
> > > jansvoboda11 wrote:
> > > > 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  = 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 , const RecordDataImpl ,
> > > > > unsigned ) 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.
> > > I've spent multiple hours trying to extract an observable test case. It 
> > > turned out to be too hairy of a yaq to shave: for each compilation a 
> > > separate sandboxed environment is created with a separate symlink tree 
> > > with just the inputs necessary for that action. Some of the inputs are 
> > > prebuilt module files (e.g. for libc++) that are version-locked with the 
> > > compiler. So far @jgorbe and I could reduce this to four compilation 
> > > steps with their own symlink trees with inputs. While I could figure out 
> > > some of the factors that affect reproducibility (for example, symlinks 
> > > are important, since making a deep copy of the input directories makes 
> > > the issue disappear), it will take a few more hours of concentrated yak 
> > > shaving to bring this to a shareable state. I'm not sure I have much more 
> > > time to 

[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 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:
> alexfh wrote:
> > jansvoboda11 wrote:
> > > 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  = 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 , const RecordDataImpl ,
> > > > unsigned ) 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.
> > I've spent multiple hours trying to extract an observable test case. It 
> > turned out to be too hairy of a yaq to shave: for each compilation a 
> > separate sandboxed environment is created with a separate symlink tree with 
> > just the inputs necessary for that action. Some of the inputs are prebuilt 
> > module files (e.g. for libc++) that are version-locked with the compiler. 
> > So far @jgorbe and I could reduce this to four compilation steps with their 
> > own symlink trees with inputs. While I could figure out some of the factors 
> > that affect reproducibility (for example, symlinks are important, since 
> > making a deep copy of the input directories makes the issue disappear), it 
> > will take a few more hours of concentrated yak shaving to bring this to a 
> > shareable state. I'm not sure I have much more time to sink into 
> > investigating this. 
> > 
> > It seems like examining code based on @eaeltsin's finding may be a more 
> > fruitful path to synthesizing a regression test. Could you try following 
> > that 

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh 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:
> jansvoboda11 wrote:
> > 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  = 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 , const RecordDataImpl ,
> > > unsigned ) 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.
> I've spent multiple hours trying to extract an observable test case. It 
> turned out to be too hairy of a yaq to shave: for each compilation a separate 
> sandboxed environment is created with a separate symlink tree with just the 
> inputs necessary for that action. Some of the inputs are prebuilt module 
> files (e.g. for libc++) that are version-locked with the compiler. So far 
> @jgorbe and I could reduce this to four compilation steps with their own 
> symlink trees with inputs. While I could figure out some of the factors that 
> affect reproducibility (for example, symlinks are important, since making a 
> deep copy of the input directories makes the issue disappear), it will take a 
> few more hours of concentrated yak shaving to bring this to a shareable 
> state. I'm not sure I have much more time to sink into investigating this. 
> 
> It seems like examining code based on @eaeltsin's finding may be a more 
> fruitful path to synthesizing a regression test. Could you try following that 
> path?
One more observation: `-fmodules-embed-all-files` and `-Wno-mismatched-tags` 
compiler options turned out to be important.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a subscriber: jgorbe.
alexfh 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");

jansvoboda11 wrote:
> 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  = 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 , const RecordDataImpl ,
> > unsigned ) 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.
I've spent multiple hours trying to extract an observable test case. It turned 
out to be too hairy of a yaq to shave: for each compilation a separate 
sandboxed environment is created with a separate symlink tree with just the 
inputs necessary for that action. Some of the inputs are prebuilt module files 
(e.g. for libc++) that are version-locked with the compiler. So far @jgorbe and 
I could reduce this to four compilation steps with their own symlink trees with 
inputs. While I could figure out some of the factors that affect 
reproducibility (for example, symlinks are important, since making a deep copy 
of the input directories makes the issue disappear), it will take a few more 
hours of concentrated yak shaving to bring this to a shareable state. I'm not 
sure I have much more time to sink into investigating this. 

It seems like examining code based on @eaeltsin's finding may be a more 
fruitful path to synthesizing a regression test. Could you try following that 
path?


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

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

[Jan Svoboda via Phabricator via cfe-commits]

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  = 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 , const RecordDataImpl ,
> unsigned ) 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

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh 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");

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  = 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 , const RecordDataImpl ,
unsigned ) 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.


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

[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  = 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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Evgeny Eltsin via Phabricator via cfe-commits]

eaeltsin added a comment.

Looks like our tests fail because ReadFileID doesn't translate file ID as 
ReadSourceLocation/TranslateSourceLocation do. Please see the prototype fix 
inline.




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");

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  = Diag.DiagStatesByLoc.Files[IDAndOffset.first];
```

Sorry I don't know the codebase, so this fix is definitely ugly :) But it shows 
the problem.



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

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

Heads up: this commit has broken compilation in a small number of cases: some 
#includes from modularized headers now fail to be found. I'm still trying to 
figure out what exactly happened, but some behavior has definitely changed by 
this commit.


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

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

[Jan Svoboda via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGf61c135a6908: [clang][modules] NFCI: Pragma diagnostic 
mappings: write/read FileID instead of… (authored by jansvoboda11).

Changed prior to commit:
  https://reviews.llvm.org/D137213?vs=472460=472480#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137213

Files:
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp


Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -3010,9 +3010,7 @@
   continue;
 ++NumLocations;
 
-SourceLocation Loc = Diag.SourceMgr->getComposedLoc(FileIDAndFile.first, 
0);
-assert(!Loc.isInvalid() && "start loc for valid FileID is invalid");
-AddSourceLocation(Loc, Record);
+AddFileID(FileIDAndFile.first, Record);
 
 Record.push_back(FileIDAndFile.second.StateTransitions.size());
 for (auto  : FileIDAndFile.second.StateTransitions) {
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -6340,17 +6340,15 @@
 while (NumLocations--) {
   assert(Idx < Record.size() &&
  "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");
   unsigned Transitions = Record[Idx++];
 
   // 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  = Diag.DiagStatesByLoc.Files[IDAndOffset.first];
+  auto  = Diag.DiagStatesByLoc.Files[FID];
   F.StateTransitions.reserve(F.StateTransitions.size() + Transitions);
   for (unsigned I = 0; I != Transitions; ++I) {
 unsigned Offset = Record[Idx++];
Index: clang/include/clang/Serialization/ASTBitCodes.h
===
--- clang/include/clang/Serialization/ASTBitCodes.h
+++ clang/include/clang/Serialization/ASTBitCodes.h
@@ -41,7 +41,7 @@
 /// Version 4 of AST files also requires that the version control branch and
 /// revision match exactly, since there is no backward compatibility of
 /// AST files at this time.
-const unsigned VERSION_MAJOR = 23;
+const unsigned VERSION_MAJOR = 24;
 
 /// AST file minor version number supported by this version of
 /// Clang.


Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -3010,9 +3010,7 @@
   continue;
 ++NumLocations;
 
-SourceLocation Loc = Diag.SourceMgr->getComposedLoc(FileIDAndFile.first, 0);
-assert(!Loc.isInvalid() && "start loc for valid FileID is invalid");
-AddSourceLocation(Loc, Record);
+AddFileID(FileIDAndFile.first, Record);
 
 Record.push_back(FileIDAndFile.second.StateTransitions.size());
 for (auto  : FileIDAndFile.second.StateTransitions) {
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -6340,17 +6340,15 @@
 while (NumLocations--) {
   assert(Idx < Record.size() &&
  "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");
   unsigned Transitions = Record[Idx++];
 
   // 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  = Diag.DiagStatesByLoc.Files[IDAndOffset.first];
+  auto  = Diag.DiagStatesByLoc.Files[FID];
   

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

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

In D137213#3900850 , @dexonsmith 
wrote:

> LGTM, although I there's format-is-probably-compatible-version as a courtesy 
> for tooling, does that need a bump here?

Good point, I'll bump `VERSION_MAJOR` in 
`clang/include/clang/Serialization/ASTBitCodes.h` before committing.


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

[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 accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

LGTM, although I there's format-is-probably-compatible-version as a courtesy 
for tooling, does that need a bump here?


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

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

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 created this revision.
jansvoboda11 added reviewers: Bigcheese, vsapsai.
Herald added a subscriber: ributzka.
Herald added a project: All.
jansvoboda11 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

For pragma diagnostic mappings, we always write/read `SourceLocation` with 
offset 0. This is equivalent to just writing a `FileID`, which is exactly what 
this patch starts doing.

Depends on D137211 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D137213

Files:
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp


Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -3010,9 +3010,7 @@
   continue;
 ++NumLocations;
 
-SourceLocation Loc = Diag.SourceMgr->getComposedLoc(FileIDAndFile.first, 
0);
-assert(!Loc.isInvalid() && "start loc for valid FileID is invalid");
-AddSourceLocation(Loc, Record);
+AddFileID(FileIDAndFile.first, Record);
 
 Record.push_back(FileIDAndFile.second.StateTransitions.size());
 for (auto  : FileIDAndFile.second.StateTransitions) {
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -6340,17 +6340,15 @@
 while (NumLocations--) {
   assert(Idx < Record.size() &&
  "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");
   unsigned Transitions = Record[Idx++];
 
   // 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  = Diag.DiagStatesByLoc.Files[IDAndOffset.first];
+  auto  = Diag.DiagStatesByLoc.Files[FID];
   F.StateTransitions.reserve(F.StateTransitions.size() + Transitions);
   for (unsigned I = 0; I != Transitions; ++I) {
 unsigned Offset = Record[Idx++];


Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -3010,9 +3010,7 @@
   continue;
 ++NumLocations;
 
-SourceLocation Loc = Diag.SourceMgr->getComposedLoc(FileIDAndFile.first, 0);
-assert(!Loc.isInvalid() && "start loc for valid FileID is invalid");
-AddSourceLocation(Loc, Record);
+AddFileID(FileIDAndFile.first, Record);
 
 Record.push_back(FileIDAndFile.second.StateTransitions.size());
 for (auto  : FileIDAndFile.second.StateTransitions) {
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -6340,17 +6340,15 @@
 while (NumLocations--) {
   assert(Idx < Record.size() &&
  "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");
   unsigned Transitions = Record[Idx++];
 
   // 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  = Diag.DiagStatesByLoc.Files[IDAndOffset.first];
+  auto  = Diag.DiagStatesByLoc.Files[FID];
   F.StateTransitions.reserve(F.StateTransitions.size() + Transitions);
   for (unsigned I = 0; I != Transitions; ++I) {
 unsigned Offset = Record[Idx++];
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits