[Support] mapped_file_region: store size as size_t

[lebedev.ri]

Summary

Found when testing stage-2 build with D38101.

In file included from /build/llvm/lib/Support/Path.cpp:1045:
/build/llvm/lib/Support/Unix/Path.inc:648:14: error: comparison 'uint64_t' (aka 'unsigned long') > 18446744073709551615 is always false [-Werror,-Wtautological-constant-compare]
  if (length > std::numeric_limits<size_t>::max()) {
      ~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

size_t is uint64_t here, apparently, thus any uint64_t value
always fits into size_t.

Initial patch was to use some preprocessor logic to
not check if the size is known to fit at compile time.
But Zachary Turner suggested using this approach.

[lebedev.ri]

On Thu, Sep 21, 2017 at 4:56 PM, Zachary Turner <[e-mail]> wrote:

I would move this logic into the init function.

Do note that the check is comparing length (function param), and not Size
(class member variable), even thought they currently have the same data type.

But there's another problem. mmap doesn't even accept a size_t, it accepts
an off_t. Also, as far as I'm aware, off_t is a signed type. So perhaps you
can compare against std::numeric_limits<off_t>::max() which will never be a
tautological comparison

Hmm, am i looking in the wrong place?

MMAP(3POSIX)                                                                                                            POSIX Programmer's Manual                                                                                                           MMAP(3POSIX)

PROLOG
      This manual page is part of the POSIX Programmer's Manual.  The Linux implementation of this interface may differ (consult the corresponding Linux manual page for details of Linux behavior), or the interface may not be implemented on Linux.

NAME
      mmap — map pages of memory

SYNOPSIS
      #include <sys/mman.h>

      void *mmap(void *addr, size_t len, int prot, int flags,
          int fildes, off_t off);

MMAP(2)                                                                                                                 Linux Programmer's Manual                                                                                                                MMAP(2)

NAME
      mmap, munmap - map or unmap files or devices into memory

SYNOPSIS
      #include <sys/mman.h>

      void *mmap(void *addr, size_t length, int prot, int flags,
                 int fd, off_t offset);
      int munmap(void *addr, size_t length);

mman.h:

#ifndef __USE_FILE_OFFSET64
extern void *mmap (void *__addr, size_t __len, int __prot,
		   int __flags, int __fd, __off_t __offset) __THROW;
#else
# ifdef __REDIRECT_NTH
extern void * __REDIRECT_NTH (mmap,
			      (void *__addr, size_t __len, int __prot,
			       int __flags, int __fd, __off64_t __offset),
			      mmap64);
# else
#  define mmap mmap64
# endif
#endif

[lebedev.ri]

On Thu, Sep 21, 2017 at 5:25 PM, Zachary Turner <[e-mail]> wrote:

No, I just fail. I somehow thought the check was against the Offset
parameter.

Is changing the function to take a size_t an option?

We certainly could do that. But then, if size_t is *not* as large as uint64_t,
when someone passes uint64_t value into the constructor, it will be either
silently truncated, or there will only be a compile-time warning about truncation...
I'd say that is worse than this preprocessor magic?

[lebedev.ri]

Address review notes: just store size as size_t.

[lebedev.ri]

Ping

[alekseyshl]

Our bot is broken by this change: http://lab.llvm.org:8011/builders/sanitizer-windows/builds/17178.

include/llvm/Support/FileSystem.h

public:
   enum mapmode {
     readonly, ///< May only access map via const_data as read only.
     readwrite, ///< May access map via data and modify it. Written to path.
     priv ///< May modify via data, but changes are lost on destruction.
   };
 
 private:
   /// Platform-specific mapping state.
-  uint64_t Size;
+  size_t Size;
   void *Mapping;
 
   std::error_code init(int FD, uint64_t Offset, mapmode Mode);
 
 public:
   mapped_file_region() = delete;
   mapped_file_region(mapped_file_region&) = delete;
   mapped_file_region &operator =(mapped_file_region&) = delete;
 
   /// \param fd An open file descriptor to map. mapped_file_region takes
   ///   ownership if closefd is true. It must have been opended in the correct
   ///   mode.
   mapped_file_region(int fd, mapmode mode, uint64_t length, uint64_t offset,
                      std::error_code &ec);
 
   ~mapped_file_region();
 
-  uint64_t size() const;
+  size_t size() const;
   char *data() const;
 
   /// Get a const view of the data. Modifying this memory has undefined
   /// behavior.
   const char *const_data() const;
 
   /// \returns The minimum alignment offset must be.
   static int alignment();

lib/Support/Unix/Path.inc

 #endif // #if defined (__APPLE__)
 
   Mapping = ::mmap(nullptr, Size, prot, flags, FD, Offset);
   if (Mapping == MAP_FAILED)
     return std::error_code(errno, std::generic_category());
   return std::error_code();
 }
 
-mapped_file_region::mapped_file_region(int fd, mapmode mode, uint64_t length,
+mapped_file_region::mapped_file_region(int fd, mapmode mode, size_t length,
                                        uint64_t offset, std::error_code &ec)
     : Size(length), Mapping() {
-  // Make sure that the requested size fits within SIZE_T.
-  if (length > std::numeric_limits<size_t>::max()) {
-    ec = make_error_code(errc::invalid_argument);
-    return;
-  }
-
   ec = init(fd, offset, mode);
   if (ec)
     Mapping = nullptr;
 }
 
 mapped_file_region::~mapped_file_region() {
   if (Mapping)
     ::munmap(Mapping, Size);