[cfe-dev] Patch for handling windows files/directories

[Ted Kremenek]

Thanks Argiris.  I working on integrating this patch.  I recognized a  
while ago that relying on bogus inodes on Windows was not a real  
solution, so your suggested changes are welcome.

The problem is that we don't want to include any of the platform  
specific nastiness in FileManager.h.  Instead I think we should use a  
pimpl approach where the platform-specific handling of UniqueDirs/ 
UniqueFiles is hidden within an opaque type.  The opaque type is  
defined within FileManager.cpp (which will include all the platform  
specific nastiness) but the public interface in FileManager.h will not  
have special #ifdefs.

I got a little behind today with other things; if you are up to or  
have time to redo the patch using something like the strategy I just  
mentioned, please go for it (although I doubt what I said sounded very  
clear).  Otherwise, I'll look into integrating these changes either  
over the weekend or on Monday.

Thanks again.

Ted

On Feb 22, 2008, at 10:15 AM, Argiris Kirtzidis wrote:

> Hi Ted,
>
> It seems that the extension doesn't matter, the other patch that I  
> sent to llvmbugs had '.patch', this one '.diff'.
> I attached it as .zip now.
>
> Should I send, in the future, both a .patch (which is more  
> convenient to check out) and a .zip (so that you can apply it ;) ?
>
>
> Ted Kremenek wrote:
>> Hi Argiris,
>>
>> I'm reviewing this patch.  I'll try and get back to you tomorrow  
>> (Friday).  BTW, you could send me this as an attachment with  
>> a .patch extension? My mail client is inlining the patch and  
>> messing with the whitespace, thus making it impossible to apply it  
>> using the patch program.
>>
>> Ted
>>
>> On Feb 22, 2008, at 3:05 AM, Argiris Kirtzidis wrote:
>>
>>> Hi,
>>>
>>> The patch that I sent is problematic for non-windows systems,  
>>> sorry about that; new one attached (with better handling of  
>>> directory separator chars too).
>>>
>>> Argiris Kirtzidis wrote:
>>>> Hi,
>>>>
>>>> The attached patch allows clang to handle files/directories  
>>>> properly on windows systems.
>>> Index: Basic/FileManager.cpp
>>> ===================================================================
>>> --- Basic/FileManager.cpp    (revision 47456)
>>> +++ Basic/FileManager.cpp    (working copy)
>>> @@ -35,6 +35,28 @@
>>> /// represent a dir name that doesn't exist on the disk.
>>> #define NON_EXISTENT_DIR  
>>> reinterpret_cast<DirectoryEntry*>((intptr_t)-1)
>>>
>>> +#ifdef LLVM_ON_WIN32
>>> +#define IS_DIR_SEPARATOR_CHAR(x) ((x) == '/' || (x) == '\\')
>>> +
>>> +namespace {
>>> +  // FIXME: Add a portable GetFullPath to the llvm::sys::Path  
>>> class ?
>>> +  static std::string GetFullPath(const char *relPath)
>>> +  {
>>> +    char *absPathStrPtr = _fullpath(NULL, relPath, 0);
>>> +    assert(absPathStrPtr && "_fullpath() returned NULL!");
>>> +
>>> +    std::string absPath(absPathStrPtr);
>>> +
>>> +    free(absPathStrPtr);
>>> +    return absPath;
>>> +  }
>>> +}
>>> +
>>> +#else
>>> +#define IS_DIR_SEPARATOR_CHAR(x) ((x) == '/')
>>> +#endif
>>> +
>>> +
>>> /// getDirectory - Lookup, cache, and verify the specified  
>>> directory.  This
>>> /// returns null if the directory doesn't exist.
>>> ///
>>> @@ -63,11 +85,18 @@
>>>  if (stat(InterndDirName, &StatBuf) ||   // Error stat'ing.
>>>      !S_ISDIR(StatBuf.st_mode))          // Not a directory?
>>>    return 0;
>>> -
>>> +
>>> +#ifdef LLVM_ON_WIN32
>>> +  // It exists.  See if we have already opened a directory with  
>>> the same full path.
>>> +  std::string FullPath(GetFullPath(InterndDirName));
>>> +  DirectoryEntry &UDE =
>>> +    UniqueDirs.GetOrCreateValue(FullPath.c_str(),  
>>> FullPath.c_str() + FullPath.size()).getValue();
>>> +#else
>>>  // It exists.  See if we have already opened a directory with the  
>>> same inode.
>>>  // This occurs when one dir is symlinked to another, for example.
>>>  DirectoryEntry &UDE =
>>>    UniqueDirs[std::make_pair(StatBuf.st_dev, StatBuf.st_ino)];
>>> +#endif
>>>
>>>  NamedDirEnt.setValue(&UDE);
>>>  if (UDE.getName()) // Already have an entry with this inode,  
>>> return it.
>>> @@ -108,7 +137,7 @@
>>>  // strip off everything after it.
>>>  // FIXME: this logic should be in sys::Path.
>>>  const char *SlashPos = NameEnd-1;
>>> -  while (SlashPos >= NameStart && SlashPos[0] != '/')
>>> +  while (SlashPos >= NameStart && ! 
>>> IS_DIR_SEPARATOR_CHAR(SlashPos[0]))
>>>    --SlashPos;
>>>
>>>  const DirectoryEntry *DirInfo;
>>> @@ -142,11 +171,18 @@
>>>  }
>>>  //llvm::cerr << ": exists\n";
>>>
>>> +#ifdef LLVM_ON_WIN32
>>> +  // It exists.  See if we have already opened a file with the  
>>> same full path.
>>> +  std::string FullPath(GetFullPath(InterndFileName));
>>> +  FileEntry &UFE =
>>> +    UniqueFiles.GetOrCreateValue(FullPath.c_str(),  
>>> FullPath.c_str() + FullPath.size()).getValue();
>>> +#else
>>>  // It exists.  See if we have already opened a file with the same  
>>> inode.
>>>  // This occurs when one dir is symlinked to another, for example.
>>>  FileEntry &UFE =
>>>    
>>> const_cast<FileEntry&>(*UniqueFiles.insert(FileEntry(StatBuf.st_dev,
>>>                                                         
>>> StatBuf.st_ino)).first);
>>> +#endif
>>>
>>>
>>>  NamedFileEnt.setValue(&UFE);
>>> Index: include/clang/Basic/FileManager.h
>>> ===================================================================
>>> --- include/clang/Basic/FileManager.h    (revision 47456)
>>> +++ include/clang/Basic/FileManager.h    (working copy)
>>> @@ -16,6 +16,7 @@
>>>
>>> #include "llvm/ADT/StringMap.h"
>>> #include "llvm/Bitcode/SerializationFwd.h"
>>> +#include "llvm/Config/Config.h"
>>> #include <map>
>>> #include <set>
>>> #include <string>
>>> @@ -48,6 +49,8 @@
>>>  friend class FileManager;
>>> public:
>>>  FileEntry(dev_t device, ino_t inode) : Name(0), Device(device),  
>>> Inode(inode){}
>>> +  // Add a default constructor for use with llvm::StringMap
>>> +  FileEntry() : Name(0), Device(0), Inode(0) {}
>>>
>>>  const char *getName() const { return Name; }
>>>  off_t getSize() const { return Size; }
>>> @@ -72,10 +75,19 @@
>>> /// names (e.g. symlinked) will be treated as a single file.
>>> ///
>>> class FileManager {
>>> +
>>> +#ifdef LLVM_ON_WIN32
>>> +  // ino_t is useless on windows, cache using full path as unique  
>>> criterion
>>> +  /// UniqueDirs/UniqueFiles - Cache from full path to existing  
>>> directories/files.
>>> +  ///
>>> +  llvm::StringMap<DirectoryEntry> UniqueDirs;
>>> +  llvm::StringMap<FileEntry> UniqueFiles;
>>> +#else
>>>  /// UniqueDirs/UniqueFiles - Cache from ID's to existing  
>>> directories/files.
>>>  ///
>>>  std::map<std::pair<dev_t, ino_t>, DirectoryEntry> UniqueDirs;
>>>  std::set<FileEntry> UniqueFiles;
>>> +#endif
>>>
>>>  /// DirEntries/FileEntries - This is a cache of directory/file  
>>> entries we have
>>>  /// looked up.  The actual Entry is owned by UniqueFiles/ 
>>> UniqueDirs above.
>>> _______________________________________________
>>> cfe-dev mailing list
>>> cfe-dev at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>>
>>
> <clang-win32-2.zip>