<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Aug 7, 2014 at 1:11 PM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Richard came up with the idea of updating the FileEntry's name on each getFile, so we'd get the last value instead of the first (which would solve the module map related problem).<div>
Unfortunately this breaks:</div>
<div><a href="http://llvm.org/bugs/show_bug.cgi?id=8974" target="_blank">http://llvm.org/bugs/show_bug.cgi?id=8974</a></div></div></blockquote><div><br></div><div>I've got what I think is a "real fix" for the regression. With that fix, just resetting .Name on each stat works. Will send out code review for the regression fix in a moment.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><br></div><div><br></div></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra">
<br><br><div class="gmail_quote">On Wed, Aug 6, 2014 at 6:10 PM, Ben Langmuir <span dir="ltr"><<a href="mailto:blangmuir@apple.com" target="_blank">blangmuir@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><div><blockquote type="cite"><div>On Aug 6, 2014, at 2:01 AM, Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>> wrote:</div>

<br><div><div dir="ltr">Ok, I might be dense here, but I'm still missing the picture :)<div><br></div><div>So, I see that the idea is to have FileName that basically encapsulates which path/name combination we found the file under, and a FileEntry, which maps to the inode.</div>


<div><br></div><div>Questions:</div><div>1. do we want to replace all uses of FileEntry with FileName, or are there cases where we're only interested in the inodes (for example in SourceManager).</div></div></div></blockquote>

<blockquote type="cite"><div><div dir="ltr"><div>2. how would we detect whether we are on a case insensitive file system?</div></div></div></blockquote><div><br></div></div><div>We can check sensitivity lazily on a per-file basis. If a lookup misses the SeenFileEntries cache, but hits a file we already have a UniqueID for, we can then check whether we already have a FileName for that file that is the same as our lookup up to case-sensitivity.  To do that, we can either store a list of FileName pointers in the FileEntry, or we can add another map to the FileManager to look it up.  Either way, the common-case will be one FIleName per FileEntry</div>

<span><font color="#888888"><div><br></div><div>Ben</div></font></span><div><div><br><blockquote type="cite"><div><div dir="ltr"><div>Cheers,</div><div>/Manuel</div><div><br></div></div><div class="gmail_extra">
<br><br><div class="gmail_quote">On Tue, Aug 5, 2014 at 9:58 PM, Ben Langmuir <span dir="ltr"><<a href="mailto:blangmuir@apple.com" target="_blank">blangmuir@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><div><blockquote type="cite"><div>On Aug 5, 2014, at 12:28 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:</div>


<br><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Aug 5, 2014 at 10:57 AM, Ben Langmuir <span dir="ltr"><<a href="mailto:blangmuir@apple.com" target="_blank">blangmuir@apple.com</a>></span> wrote:<br>



<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><blockquote type="cite"><div>On Aug 5, 2014, at 7:51 AM, Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>> wrote:</div>



<br><div><div dir="ltr">As this is blocking us, I'm taking a stab at it…</div></div></blockquote><div><br></div><div>One thing that stood out to me in Richard's patch:</div><div><br></div><div>* Returning FileName pointers invites users to rely on the identity of the FileName object, which won’t work on case-insensitive file systems and is probably a bad idea in general.  I suggest we wrap FileName * in a “FileRef” object that prevents comparisons or possibly forwards them to comparing the inodes.</div>



</div></div></blockquote><div><br></div><div>I don't agree: there are some uses where we really want to compare the canonicalized FileName pointers. For instance, this is the right behavior for #pragma once, and for various things in HeaderSearch. (When looking up a header relative to a file, it should be the FileName that is the cache key, not the FileEntry.)</div>


</div></div></div></div></blockquote><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>
</div><div><br></div><div>Perhaps the best way to deal with case-insensitive file systems is to give paths that differ by case the same FileName object, since they are the same dentry (which is the notion we're trying to capture here).  </div>


</div></div></div></blockquote><div><br></div></div><div>SGTM.</div><div><div><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Likewise, on Windows, the short and long forms of the same file name should have the same FileName object. We may be able to make that change without introducing a split between FileName and FileEntry; the downside would be that we may potentially load the same inode multiple times if it's found by different paths (relative paths being the most likely offender).</div>



<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><span><font color="#888888">Ben</font></span><div>
<div><div><br></div><br><blockquote type="cite"><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Jul 30, 2014 at 12:04 AM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>




<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>On Mon, Jul 28, 2014 at 11:40 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>





<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><p dir="ltr"><br>
On 28 Jul 2014 17:57, "Ben Langmuir" <<a href="mailto:blangmuir@apple.com" target="_blank">blangmuir@apple.com</a>> wrote:<br>
><br>
><br>
>> On Jul 28, 2014, at 4:37 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:<br>
>><br>
>> On Mon, Jul 28, 2014 at 3:10 PM, Ben Langmuir <<a href="mailto:blangmuir@apple.com" target="_blank">blangmuir@apple.com</a>> wrote:<br>
>>><br>
>>><br>
>>> > On Jul 28, 2014, at 4:31 AM, Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>> wrote:<br>
>>> ><br>
>>> > Hi Richard,<br>
>>> ><br>
>>> > while working with module maps for layering checks a problem with __FILE__ we noticed triggered a couple of questions.<br>
>>> ><br>
>>> > The problem is that __FILE__ uses the path of the first 'stat' call (as that is how FileManager::getFile() works).<br>
>>><br>
>>> I was thinking about this a while ago and independently of this issue I would really like to change this behaviour at some point.  The name of a file is a property of how you look it up, not an intrinsic part of the file itself.<br>







>><br>
>><br>
>> I agree. We have incorrectly conflated the notion of the file identity (inode) with the directory entriy, and we can't tell the two apart. This leads to weird behavior in a number of places. For instance:<br>







>><br>
>>  a/<br>
>>   x.h: #include "y.h"<br>
>>   y.h: int a = 0;<br>
>>  b/<br>
>>   x.h: symlink to a/x.h<br>
>>   y.h: int b = 0;<br>
>>  main.c:<br>
>>   #include "a/x.h"<br>
>>   #include "b/x.h"<br>
>>   int main() { return a + b; }<br>
>><br>
>> On a correct compiler, this would work. For clang, it fails, because b/x.h's #include finds a/y.h, because we use the path by which we first found x.h as the One True Path to x.h. (This also leads to wrong __FILE__, etc.)<br>







>><br>
>> I tried fixing this ~2 years ago by splitting FileEntry into separate dentry and inode classes, but this rapidly snowballed and exposed the same design error being made in various other components.<br>
><br>
><br>
> Interesting.  My motivation was keeping track of virtual and “real” paths for the VFS, which is a special case of the above.  Maybe we can sink the dentry/inode down to the VFS layer eventually?  Getting symlinks and “..” entries to work in the VFS make a lot more sense when we have explicit dentries rather than inferring from the file path.<br>







><br>
> I’d be interested in what issues you ran into here if you remember.</p>
</div><p dir="ltr">I'll see if I can dig out my patch tomorrow.</p></blockquote></div><div>Attached is my WIP patch from (as it turns out) over 2 years ago. My (very) hazy memories were that we had quite a few different places where people were using FileEntry*s for things, and neither a dentry nor an inode seemed like the "right" thing. I don't remember any more details than that. There were also places where we would need to just make a decision, such as: what should #pragma once use as its key? (I think dentry is the right answer here, since the same file found in different directories might mean different things, but that answer may break some people who use #pragma once and don't also have include guards. Conversely, it fixes some builds on content-addressed file systems.)<br>





</div></div></div></div>
</blockquote></div><br></div>
</blockquote></div></div></div><br></div></blockquote></div><br></div></div>
</blockquote></div></div></div><br></div></blockquote></div><br></div>
</div></blockquote></div></div></div><br></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div></div>