[support] remove tautological comparison in Support/Windows/Path.inc

[inglorion]

Summary

The removed code checks that we are able to handle a 64-bit number, but
the code we're calling takes two dwords (for a total of 64 bits), so this
is always true.

[zturner]

Adding more Windows people, because as I look over this code, it actually looks wrong to me. The value we pass for MaximumFileSize seems unnecessarily large. I'm going through the docs at the moment to figure out the correct thing to do here.

[zturner]

Let me know if this is correct. I think we can actually get (potentially very large) savings from this.

llvm/lib/Support/Windows/Path.inc

741–742

I don't think we need Offset as part of this calculation. If we want to map 32 bytes starting at offset 32, the current code will create a file mapping object with a maximum size of 64. This doesn't seem necessary. It should be creating a file mapping object of exactly 32 bytes, just like we requested. So, I propose changing this line to: CreateFileMappingW(FileHandle, 0, flprotect, Size >> 32, Size & 0xFFFFFFFF);

755–759

This part does not need to be changed. because the "View" is a disconnected entity from the "Mapping", This will map Size bytes from file offset Offset into offset 0 of the mapping object.

This is no different from what happened before, except that before our mapping object would just get a ton of unnecessary bytes allocated at the end of it.

[amccarth]

Zach is correct about the overly large reservation in the file mapping and we should fix that too.

llvm/lib/Support/Windows/Path.inc

726

I assume someone was originally worried that SIZE_T would be 32 bits wide in a 32-bit build but somehow believed std::size_t (which is the type of Size) could still somehow be larger than that. Ain't gonna happen.

[rnk]

llvm/lib/Support/Windows/Path.inc

726

I wonder if we should turn these examples of dynamic range checks into static_asserts. We saw a lot of these tautological comparison warnings in Chrome, where it was much less obvious that the types are the same size. Would this be an appropriate replacement for the check?

static_assert(std::numeric_limits<decltype(Size)>::max() <=
              std::numeric_limits<SIZE_T>::max());

[inglorion]

@zturner do you want me to make the semantic changes in the same commit or can we ship the size check removal first and then change the functionality? The latter would be my preference.

llvm/lib/Support/Windows/Path.inc

726

My original plan was to come up with a way to elide the check if we can statically verify that it will fit, but then zturner pointed out that this is always true and we can just delete the check altogether.

Note that the code checks if Size fits in a SIZE_T. Size was a uint64_t until recently and is now a size_t, which is at most a 64-bit unsigned. Offset is already an uint64_t. So we need a 64-bit value even if Size were 32-bit on some system. And we do have 64 bits, because we're passing two 32-bit values to CreateFileMappingW and MapViewOfFile. So a 64-bit value will always fit.

[inglorion]

Is this good to go or does it need changes?

[zturner]

In D39263#906884, @inglorion wrote:

@zturner do you want me to make the semantic changes in the same commit or can we ship the size check removal first and then change the functionality? The latter would be my preference.

Just want to confirm that this follow up change is still going to happen? This is what I was worried about with letting it go in separately. I really don't want to see this change forgotten about, as it seems like a very useful improvement.

[inglorion]

I would never forget about...err, what was it again I needed to remember? Thanks for the reminder. Follow-up change is in D39876.

llvm/lib/Support/Windows/Path.inc

std::error_code setLastModificationAndAccessTime(int FD, TimePoint<> Time) {
   HANDLE FileHandle = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
   if (!SetFileTime(FileHandle, NULL, &FT, &FT))
     return mapWindowsError(::GetLastError());
   return std::error_code();
 }
 
 std::error_code mapped_file_region::init(int FD, uint64_t Offset,
                                          mapmode Mode) {
-  // Make sure that the requested size fits within SIZE_T.
-  if (Size > std::numeric_limits<SIZE_T>::max())
-    return make_error_code(errc::invalid_argument);
-
   HANDLE FileHandle = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
   if (FileHandle == INVALID_HANDLE_VALUE)
     return make_error_code(errc::bad_file_descriptor);
 
   DWORD flprotect;
   switch (Mode) {
   case readonly:  flprotect = PAGE_READONLY; break;
   case readwrite: flprotect = PAGE_READWRITE; break;
   case priv:      flprotect = PAGE_WRITECOPY; break;
   }
 
   HANDLE FileMappingHandle =
       ::CreateFileMappingW(FileHandle, 0, flprotect,
                            (Offset + Size) >> 32,
                            (Offset + Size) & 0xffffffff,
                            0);
   if (FileMappingHandle == NULL) {
     std::error_code ec = mapWindowsError(GetLastError());
     return ec;
   }
 
   DWORD dwDesiredAccess;
   switch (Mode) {
   case readonly:  dwDesiredAccess = FILE_MAP_READ; break;
   case readwrite: dwDesiredAccess = FILE_MAP_WRITE; break;
   case priv:      dwDesiredAccess = FILE_MAP_COPY; break;
   }
   Mapping = ::MapViewOfFile(FileMappingHandle,
                             dwDesiredAccess,
                             Offset >> 32,
                             Offset & 0xffffffff,
                             Size);
   if (Mapping == NULL) {
     std::error_code ec = mapWindowsError(GetLastError());
     ::CloseHandle(FileMappingHandle);
     return ec;
   }
 
   if (Size == 0) {
     MEMORY_BASIC_INFORMATION mbi;