Fix call to CreateFileMappingW. #1902

mol123 · 2024-08-27T11:26:42Z

The call shouldn't rely on the size variable, since it is available only conditionally, based on preprocessor macros.

#1901

The call shouldn't rely on the `size` variable, since it is available only conditionally, based on preprocessor macros. The `dwMaximumSize{High,Low}` arguments are actually optional, so omitting them. yhirose#1901

yhirose · 2024-08-27T13:16:57Z

httplib.h

@@ -2859,8 +2859,7 @@ inline bool mmap::open(const char *path) {
  hMapping_ =
      ::CreateFileMappingFromApp(hFile_, NULL, PAGE_READONLY, size_, NULL);


For consistency, size_ can be 0 as well. (I don't like it though...)

For CreateFileMappingFromApp, the documentation doesn't state that size can be 0, so I would continue passing size_ to stay aligned with the documentation : https://learn.microsoft.com/en-us/windows/win32/api/memoryapi/nf-memoryapi-createfilemappingfromapp

For CreateFileMappingW, the documentation explictly states "If this parameter and dwMaximumSizeHigh are 0 (zero), the maximum size of the file mapping object is equal to the current size of the file that hFile identifies." https://learn.microsoft.com/en-us/windows/win32/api/memoryapi/nf-memoryapi-createfilemappingw

I got it, thanks!

How do you come to this code https://github.com/yhirose/cpp-httplib/pull/1902/files#diff-2324f73f4a4958b756eb44d0607268befc5de62db01164ccd73e154da808d8f3R2849-R2861? I think this block is never reached by a compiler if Windows SDK is updated (latest one w/ Windows 7).

I would remove this block completely and deal with bit shifts at LARGE_INTEGER::HighPart. LARGE_INTEGER is supported on Windows 7, LARGE_INTEGER::QuadPart is not supported.

According to my SDK's fileapi.h:

GetFileSize is available if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_DESKTOP | WINAPI_PARTITION_SYSTEM | WINAPI_PARTITION_GAMES)

GetFileSizeEx is available if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_APP | WINAPI_PARTITION_SYSTEM | WINAPI_PARTITION_GAMES)

So the else block would be used if only WINAPI_PARTITION_DESKTOP would be set.

So the else block would be used if only WINAPI_PARTITION_DESKTOP would be set.

This is not true. WINAPI_PARTITION_DESKTOP is included in WINAPI_PARTITION_APP and #else is never reached.
winapifamily.h:

#ifndef WINAPI_PARTITION_DESKTOP #define WINAPI_PARTITION_DESKTOP (WINAPI_FAMILY == WINAPI_FAMILY_DESKTOP_APP) #endif #ifndef WINAPI_PARTITION_APP #define WINAPI_PARTITION_APP \ (WINAPI_FAMILY == WINAPI_FAMILY_DESKTOP_APP || \ WINAPI_FAMILY == WINAPI_FAMILY_PC_APP || \ WINAPI_FAMILY == WINAPI_FAMILY_PHONE_APP) #endif

The condition

#if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_APP | WINAPI_PARTITION_SYSTEM | \ WINAPI_PARTITION_GAMES)

includes ALL Windows editions.

* +---------------------------+ * | *Partition* | * +---+---+---+---+---+---+---+ * | | | | | | | | * | | | | | | | | * | | | | P | | | | * | | | | H | | | | * | D | | | O | | | | * | E | | P | N | S | S | | * | S | | C | E | Y | E | G | * | K | | _ | _ | S | R | A | * | T | A | A | A | T | V | M | * +-------------------------+----+ O | P | P | P | E | E | E | * | *Platform/Family* \| P | P | P | P | M | R | S | * +------------------------------+---+---+---+---+---+---+---+ * | WINAPI_FAMILY_DESKTOP_APP | X | X | X | | | | | * +------------------------------+---+---+---+---+---+---+---+ * | WINAPI_FAMILY_PC_APP | | X | X | | | | | * +------------------------------+---+---+---+---+---+---+---+ * | WINAPI_FAMILY_PHONE_APP | | X | | X | | | | * +----------------------------- +---+---+---+---+---+---+---+ * | WINAPI_FAMILY_SYSTEM | | | | | X | | | * +----------------------------- +---+---+---+---+---+---+---+ * | WINAPI_FAMILY_SERVER | | | | | X | X | | * +------------------------------+---+---+---+---+---+---+---+ * | WINAPI_FAMILY_GAMES | | | | | | | X | * +------------------------------+---+---+---+---+---+---+---+

yhirose · 2024-08-27T13:25:07Z

Could you confirm that passing 0 to the create file mapping functions dons't make any performance regression? (I guess passing length of the file explicitly performs better.)

Also I don't see enough examples which pass 0 length to the APIs...
https://github.com/search?q=CreateFileMappingFromApp+language%3AC%2B%2B&type=code&l=C%2B%2B

mol123 · 2024-08-27T19:01:06Z

I've run a benchmark of just creating an httplib::detail::mmap object of a small file, reading the data, and throwing the mmap away on Windows 7. There is no significant change in performance.

With size set to zero: 10011 ns with 56000 iterations.

With size set explicitly: 10529 ns with 74667 iterations.

UnrealKaraulov · 2024-08-28T05:07:51Z

I think this is just misprint in code.

#if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_APP | WINAPI_PARTITION_SYSTEM |   \
                            WINAPI_PARTITION_GAMES)
  LARGE_INTEGER size{};
  if (!::GetFileSizeEx(hFile_, &size)) { return false; }
  size_ = static_cast<size_t>(size.QuadPart);
#else
  DWORD sizeHigh;
  DWORD sizeLow;
  sizeLow = ::GetFileSize(hFile_, &sizeHigh);
  if (sizeLow == INVALID_FILE_SIZE) { return false; }
  size_ = (static_cast<size_t>(sizeHigh) << (sizeof(DWORD) * 8)) | sizeLow;
#endif

#if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_APP | WINAPI_PARTITION_SYSTEM) && \
    (_WIN32_WINNT >= _WIN32_WINNT_WIN8)
  hMapping_ =
      ::CreateFileMappingFromApp(hFile_, NULL, PAGE_READONLY, size_, NULL);
#else
  hMapping_ = ::CreateFileMappingW(hFile_, NULL, PAGE_READONLY, size.HighPart,
                                   size.LowPart, NULL);
#endif

LARGE_INTEGER size can missing if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_APP | WINAPI_PARTITION_SYSTEM | WINAPI_PARTITION_GAMES) not executed

Maybe need just

Change

#if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_APP | WINAPI_PARTITION_SYSTEM |   \
                            WINAPI_PARTITION_GAMES)
  LARGE_INTEGER size{};
  if (!::GetFileSizeEx(hFile_, &size)) { return false; }
  size_ = static_cast<size_t>(size.QuadPart);
#else
  DWORD sizeHigh;
  DWORD sizeLow;
  sizeLow = ::GetFileSize(hFile_, &sizeHigh);
  if (sizeLow == INVALID_FILE_SIZE) { return false; }
  size_ = (static_cast<size_t>(sizeHigh) << (sizeof(DWORD) * 8)) | sizeLow;
#endif

to

LARGE_INTEGER size{};
if (!::GetFileSizeEx(hFile_, &size)) { return false; }
size_ = static_cast<size_t>(size.QuadPart);

If GetFileSizeEx has starting from WinXP...

or to

#if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_APP | WINAPI_PARTITION_SYSTEM) && \
    (_WIN32_WINNT >= _WIN32_WINNT_WIN8)
  DWORD sizeHigh;
  DWORD sizeLow;
  sizeLow = ::GetFileSize(hFile_, &sizeHigh);
  if (sizeLow == INVALID_FILE_SIZE) { return false; }
  size_ = (static_cast<size_t>(sizeHigh) << (sizeof(DWORD) * 8)) | sizeLow;
#else
  LARGE_INTEGER size{};
  if (!::GetFileSizeEx(hFile_, &size)) { return false; }
  size_ = static_cast<size_t>(size.QuadPart);
#endif

for passing size variable in same case as CreateFileMappingW

The call shouldn't rely on the `size` variable, since it is available only conditionally, based on preprocessor macros. yhirose#1901

yhirose · 2024-08-29T23:29:16Z

@mol123 thank you for this pull request. But, I am not comfortable with the changes, since it makes the current code more complicated... I am more interested in completely rewriting the complicated preprocessor conditions to support various Windows environments. (#1903)

UnrealKaraulov · 2024-09-12T10:09:51Z

@yhirose _WIN32_WINNT_WIN8 not found if compile with Windows 7 SDK. This cause issue.

1>c:\projects\cs1.6\unrealcheatfinder\unrealcheatfinder\src\httplib.h(2915): error C2039: CreateFile2: не является членом "`global namespace'"
1>c:\projects\cs1.6\unrealcheatfinder\unrealcheatfinder\src\httplib.h(2915): error C3861: CreateFile2: идентификатор не найден
1>c:\projects\cs1.6\unrealcheatfinder\unrealcheatfinder\src\httplib.h(2938): error C2039: CreateFileMappingFromApp: не является членом "`global namespace'"
1>c:\projects\cs1.6\unrealcheatfinder\unrealcheatfinder\src\httplib.h(2938): error C3861: CreateFileMappingFromApp: идентификатор не найден
1>c:\projects\cs1.6\unrealcheatfinder\unrealcheatfinder\src\httplib.h(2956): error C2039: MapViewOfFileFromApp: не является членом "`global namespace'"
1>c:\projects\cs1.6\unrealcheatfinder\unrealcheatfinder\src\httplib.h(2956): error C3861: MapViewOfFileFromApp: идентификатор не найден

mol123 · 2024-09-12T10:18:37Z

As a workaroud, the following can be added before including the header:

#ifndef _WIN32_WINNT_WIN8
 #define _WIN32_WINNT_WIN8 0x0602
#endif

Fix call to CreateFileMappingW.

71c3d1f

The call shouldn't rely on the `size` variable, since it is available only conditionally, based on preprocessor macros. The `dwMaximumSize{High,Low}` arguments are actually optional, so omitting them. yhirose#1901

mol123 force-pushed the win-largeint-fix branch from c2a6384 to 71c3d1f Compare August 27, 2024 11:43

yhirose reviewed Aug 27, 2024

View reviewed changes

Fix file size computation on 32-bit Windows.

ea66347

mol123 referenced this pull request in mol123/cpp-httplib Aug 28, 2024

Fix call to CreateFileMappingW.

c2a6384

The call shouldn't rely on the `size` variable, since it is available only conditionally, based on preprocessor macros. yhirose#1901

yhirose closed this Aug 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix call to CreateFileMappingW. #1902

Fix call to CreateFileMappingW. #1902

Uh oh!

mol123 commented Aug 27, 2024

Uh oh!

yhirose Aug 27, 2024 •

edited

Loading

Uh oh!

mol123 Aug 27, 2024

Uh oh!

yhirose Aug 27, 2024

Uh oh!

sergio-nsk Aug 29, 2024 •

edited

Loading

Uh oh!

mol123 Aug 29, 2024

Uh oh!

sergio-nsk Aug 29, 2024 •

edited

Loading

Uh oh!

yhirose commented Aug 27, 2024

Uh oh!

mol123 commented Aug 27, 2024

Uh oh!

UnrealKaraulov commented Aug 28, 2024 •

edited

Loading

Uh oh!

yhirose commented Aug 29, 2024

Uh oh!

UnrealKaraulov commented Sep 12, 2024 •

edited

Loading

Uh oh!

mol123 commented Sep 12, 2024

Uh oh!

Uh oh!

		@@ -2859,8 +2859,7 @@ inline bool mmap::open(const char *path) {
		hMapping_ =
		::CreateFileMappingFromApp(hFile_, NULL, PAGE_READONLY, size_, NULL);

Fix call to CreateFileMappingW. #1902

Fix call to CreateFileMappingW. #1902

Uh oh!

Conversation

mol123 commented Aug 27, 2024

Uh oh!

yhirose Aug 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mol123 Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

yhirose Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

sergio-nsk Aug 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mol123 Aug 29, 2024

Choose a reason for hiding this comment

Uh oh!

sergio-nsk Aug 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yhirose commented Aug 27, 2024

Uh oh!

mol123 commented Aug 27, 2024

Uh oh!

UnrealKaraulov commented Aug 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yhirose commented Aug 29, 2024

Uh oh!

UnrealKaraulov commented Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mol123 commented Sep 12, 2024

Uh oh!

Uh oh!

yhirose Aug 27, 2024 •

edited

Loading

sergio-nsk Aug 29, 2024 •

edited

Loading

sergio-nsk Aug 29, 2024 •

edited

Loading

UnrealKaraulov commented Aug 28, 2024 •

edited

Loading

UnrealKaraulov commented Sep 12, 2024 •

edited

Loading