Page MenuHomePhabricator

[Support] mapped_file_region: store size as size_t
ClosedPublic

Authored by lebedev.ri on Sep 21 2017, 6:41 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

lebedev.ri created this revision.Sep 21 2017, 6:41 AM
lebedev.ri added a comment.EditedSep 21 2017, 7:23 AM

On Thu, Sep 21, 2017 at 4:56 PM, Zachary Turner <zturner@google.com> 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

On Thu, Sep 21, 2017 at 5:25 PM, Zachary Turner <zturner@google.com> 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 retitled this revision from [Support] mapped_file_region: avoid tautological comparison. to [Support] mapped_file_region: store size as size_t.
lebedev.ri edited the summary of this revision. (Show Details)

Address review notes: just store size as size_t.

This revision was automatically updated to reflect the committed changes.