jansvoboda11 added a comment.
Herald added a project: All.

Hi @ilyakuteev, this patch is causing some issues downstream. (See 
`clang/test/Modules/non-affecting-module-maps-source-locations.m` here 
<https://github.com/apple/llvm-project/pull/5451>.)

I think they all boil down to the fact that a `SourceLocation` is a simple 
offset into `SourceManager`'s buffer of concatenated input files. By not 
serializing some input files, we make that buffer shorter by leaving out some 
of its parts. This means that a `SourceLocation` that used to point at the end 
of the last file might now be pointing outside the new, shorter buffer. More 
generally, any `SourceLocation` pointing after any of the removed input files 
is now bogus. It seems that we should be computing the set of non-affecting 
input files at the start of serialization, and then any `SourceLocation` we 
serialize, we need to adjust to account for the changes in the input file 
buffer. I'm not sure how efficiently we can implement this, given that we might 
need to adjust every `SourceLocation`. And we have lots of them.

We rely on this feature for "implicitly discovered, explicitly built modules". 
For our purpose, the (simpler) downstream patch changes the PCM file so that it 
still contains all input files, preventing the issue I just described, but 
includes a bit that says whether a file was affecting or not. My question is: 
would that approach work for your use-case? You mentioned you want to avoid 
rebuilds in distributed compilation when some non-affecting input files are 
missing. You could avoid that by not validating non-affecting input files in 
`ASTReader`. One potential issue I see with this approach is that two PCMs for 
module M produced on two machines with different sets of non-affecting input 
files are not interchangeable. They will have different contents and therefore 
different signature. This could lead to rebuilds of importers that expect one 
signature for PCM M but (after some distributed re-shuffling), the other PCM 
ends up on the machine, creating a mismatch between the expected and actual PCM 
signature.

Please, let me know your thoughts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106876

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

Reply via email to