[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

benlangmuir added a comment. In D135220#3899674 , @dexonsmith wrote: > In D135220#3898849 , @hans wrote: > >> Relatedly, we ran into a problem with `clang-cl /showIncludes` not including >> all files in its outp

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

dexonsmith added a comment. In D135220#3898849 , @hans wrote: > Relatedly, we ran into a problem with `clang-cl /showIncludes` not including > all files in its output when they're linked: > https://github.com/llvm/llvm-project/issues/58726 Interestingl

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

dexonsmith added a comment. In D135220#3870991 , @hans wrote: > One thought, which I'm not sure is relevant, is that this is only observable > for headers which are included more than once, which is rare because normally > there are include guards (or p

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

hans added a comment. Relatedly, we ran into a problem with `clang-cl /showIncludes` not including all files in its output when they're linked: https://github.com/llvm/llvm-project/issues/58726 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135220/

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

hans added a comment. In D135220#3862893 , @dexonsmith wrote: > In D135220#3862058 , @hans wrote: > >> The build system folks replied saying they're not using symlinks, but hard >> links for compiler inputs. Dro

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

benlangmuir added a comment. (catching up after I was away last week) I agree with @dexonsmith's analysis above. One historical note: > As a heuristic on top of (4), "last-ref" (before this patch) probably gets > the answer the user expects more often than "first-ref" (this patch) does, > whic

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

dexonsmith added a comment. In D135220#3862058 , @hans wrote: > The build system folks replied saying they're not using symlinks, but hard > links for compiler inputs. Dropping the `-s` flag in the repro above > demonstrates this. I think this only hap

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

hans added a comment. The build system folks replied saying they're not using symlinks, but hard links for compiler inputs. Dropping the `-s` flag in the repro above demonstrates this. Here's a new version of the reproducer that's a little less convoluted: $ echo 1, 2, 3 > /tmp/foo.h $ ln

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

hans added a comment. Here's a reproducer of what's happening in our case. It's using the preprocessor because that's the easiest, but the real problem for us is that the debug info is pointing to the wrong file (and that it's different depending on the symlinkedness of the include). $ cat /

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

hans added a comment. Heads up that we're seeing a problem in Chromium that bisects to this change. (http://crbug.com/1373836) In our case what's happening is that the compiler is getting the name of an included file wrong, which ends up in the debug info (and also shows in the preprocessor ou

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

benlangmuir added a comment. > I was mostly trying to confirm that symlink setups will be fine I don't know if we promise this is supported anywhere, but I know we've made this kind of test change to force unique files several times in the past. Repository: rG LLVM Github Monorepo CHANGES S

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

goncharov added a comment. In D135220#3840354 , @benlangmuir wrote: > Okay, I was able to reproduce by symlinking all the 0-byte header files to > header0 (any choice probably works). The behaviour is deterministic before > and after my change. > > Th

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

benlangmuir added a comment. Okay, I was able to reproduce by symlinking all the 0-byte header files to header0 (any choice probably works). The behaviour is deterministic before and after my change. This was only passing by luck in this setup, because it was relying on mutation of `FileEntry

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

dexonsmith added a comment. In D135220#3840257 , @goncharov wrote: > It's possbile to make it deterministic by making headers unique though. Also, this is the first time you've mentioned non-determinism. Is this non-deterministic? Repository: rG LLV

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

dexonsmith added a comment. In D135220#3840257 , @goncharov wrote: > In D135220#3840177 , @dexonsmith > wrote: > >> In D135220#3839671 , @goncharov >> wrote: >> >>> That

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

goncharov added a comment. In D135220#3840177 , @dexonsmith wrote: > In D135220#3839671 , @goncharov > wrote: > >> That change might be problematic for content addressing storages. E.g. >> clang/test/Driver/cl-

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

dexonsmith added a comment. In D135220#3839671 , @goncharov wrote: > That change might be problematic for content addressing storages. E.g. > clang/test/Driver/cl-pch-showincludes.cpp started to fail in my setup as > clang/test/Driver/header{0,1,3,4}.h

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

benlangmuir added a comment. > That change might be problematic for content addressing storages. E.g. > clang/test/Driver/cl-pch-showincludes.cpp started to fail in my setup as > clang/test/Driver/header{0,1,3,4}.h are all identical and can be symlinked What is the failure you're seeing? I wou

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

goncharov added subscribers: dexonsmith, goncharov. goncharov added a comment. That change might be problematic for content addressing storages. E.g. clang/test/Driver/cl-pch-showincludes.cpp started to fail as clang/test/Driver/header{0,1,3,4}.h are all identical and can be symlinked. cc @dexo

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

This revision was automatically updated to reflect the committed changes. Closed by commit rG5ea78c4113f8: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef (authored by benlangmuir). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135220

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

benlangmuir added inline comments. Comment at: clang/include/clang/Basic/SourceManager.h:146-155 /// References the file which the contents were actually loaded from. /// /// Can be different from 'Entry' if we overridden the contents of one file /// with the contents

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

benlangmuir updated this revision to Diff 465457. benlangmuir edited the summary of this revision. benlangmuir added a comment. - Used `OptionalFileEntryRefDegradesToFileEntryPtr` at two callsites - Attempted to fix Windows path / vs \ in test. CHANGES SINCE LAST ACTION https://reviews.llvm.or

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

bnbarham accepted this revision. bnbarham added inline comments. Comment at: clang/include/clang/Basic/SourceManager.h:146-155 /// References the file which the contents were actually loaded from. /// /// Can be different from 'Entry' if we overridden the contents of one

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

jansvoboda11 accepted this revision. jansvoboda11 added a comment. This revision is now accepted and ready to land. LGTM with a couple of small suggestions. Comment at: clang/include/clang/Basic/SourceManager.h:1062 +Optional F = sloc.getFile().getContentCache().OrigEntry;

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

benlangmuir created this revision. benlangmuir added reviewers: jansvoboda11, bnbarham. Herald added a subscriber: arphaman. Herald added a project: All. benlangmuir requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Update `SourceManager::Con