Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

overlay: Use SIZE_T as correct type for memory addresses #3160

Merged
merged 2 commits into from Jul 13, 2017

Conversation

@Kissaki
Copy link
Member

commented Jul 10, 2017

Use correct types for handling pointer size/memory addresses.

Make use of boost optional as GetFnOffsetInModules return type. The
unsigned return type does not allow for negative error values anymore,
which we did not make use of anyway, and optional is more explicit.

Replaces workaround/error detection from the commits
114495e
a3e7958

Fixes #1924


Time spent: ~1h10

@Kissaki Kissaki added the overlay label Jul 10, 2017

@Kissaki Kissaki requested a review from mkrautz Jul 10, 2017

@davidebeatrici davidebeatrici added the code label Jul 10, 2017

@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2017

Apple travis build failed with

FAIL!  : TestServerResolver::simpleA() Compared values are not the same
   Actual   (spy.count()): 0
   Expected (1)          : 1
   Loc: [TestServerResolver.cpp(86)]
FAIL!  : TestServerResolver::simpleAAAA() Compared values are not the same
   Actual   (spy.count()): 0
   Expected (1)          : 1
   Loc: [TestServerResolver.cpp(125)]

I doubt these windows overlay changes changed anything for the ServerResolver logic.

@davidebeatrici

This comment has been minimized.

Copy link
Member

commented Jul 10, 2017

For me it's safe to merge.

@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2017

After restarting the failed build it succeeded.

@mkrautz

This comment has been minimized.

Copy link
Member

commented Jul 11, 2017

  • I don't like that the overlay now depends on Boost. This is minor, since we can hopefully just s/boost/std/ when we migrate to VS2017 for 1.4.x.
  • I don't like the use of the Windows type SIZE_T. I'd probably prefer straight unsigned long. (Or a typedef of unsigned long...)
@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2017

The overlay is Windows specific and SIZE_T defines what size a pointer has.

The maximum number of bytes to which a pointer can point. Use for a count that must span the full range of a pointer.

Using long would be wrong on x64. (ref, types)

An int and a long are 32-bit values on 64-bit Windows operating systems. For programs that you plan to compile for 64-bit platforms, you should be careful not to assign pointers to 32-bit variables. Pointers are 64-bit on 64-bit platforms, and you will truncate the pointer value if you assign it to a 32-bit variable.

typedef ULONG_PTR SIZE_T;
#if defined(_WIN64)
typedef unsigned __int64 ULONG_PTR;
#else
typedef unsigned long ULONG_PTR;
#endif

So we would at least have to typedef these two cases. I’d prefer to use the types that are provided and well defined for this use case. Yes we depend on a windows header and type, but pointer size depends on the compiler and we should make use of the compilers type definition.

@mkrautz

This comment has been minimized.

Copy link
Member

commented Jul 11, 2017

I'd prefer size_t, to keep things portable. It's 64-bit on 64-bit Windows as well.

@mkrautz
Copy link
Member

left a comment

WDYT about size_t instead?

@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2017

std::uintptr_t would be the correct type to use, but is an optional type.

On many platforms (an exception is systems with segmented addressing) std::size_t can safely store the value of any non-member pointer, in which case it is synonymous with std::uintptr_t. [ref]

I’ll change the code to use std::size_t.

@Kissaki Kissaki force-pushed the Kissaki:1924-address-types branch from 5662adc to ea814ae Jul 12, 2017

@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2017

I replaced SIZE_T with size_t and pushed.

@mkrautz

This comment has been minimized.

Copy link
Member

commented Jul 12, 2017

The fields have the i-prefix, which means they're an int. Probably best to do a cleanup commit first, (remove the prefixes from members of these structs), and then do the size_t change?

@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2017

You’re ok with them not having a prefix? Or should they have some prefix for size_t now?

huh, I thought our coding style required varname type prefixes. I guess I was wrong.

@Kissaki Kissaki force-pushed the Kissaki:1924-address-types branch from ea814ae to 93f70a7 Jul 12, 2017

@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2017

I made the change and pushed.

@mkrautz

This comment has been minimized.

Copy link
Member

commented Jul 12, 2017

LGTM once the build passes

Kissaki added 2 commits Jul 12, 2017
overlay: Use size_t as correct type for memory addresses
Use correct types for handling pointer size/memory addresses.

Make use of boost optional as GetFnOffsetInModules return type. The
unsigned return type does not allow for negative error values anymore,
which we did not make use of anyway, and optional is more explicit.

Replaces workaround/error detection from the commits
114495e
a3e7958

Fixes #1924

@Kissaki Kissaki force-pushed the Kissaki:1924-address-types branch from 93f70a7 to 36fc0f1 Jul 13, 2017

@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Jul 13, 2017

FAIL!  : TestServerResolver::simpleSrv() Compared values are not the same
   Actual   (spy.count()): 0
   Expected (1)          : 1
   Loc: [TestServerResolver.cpp(39)]
FAIL!  : TestServerResolver::simpleA() Compared values are not the same
   Actual   (spy.count()): 0
   Expected (1)          : 1
   Loc: [TestServerResolver.cpp(86)]

#3162. Retriggered the build.

@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Jul 13, 2017

One AppVeyor build failed as well with an env error

git submodule --quiet update --init --recursive
warning: unable to unlink C:/MumbleBuild/mumble/.git/modules/sbcelt-src/modules/celt-0.7.0/objects/f5/04ca3a0b8b4b500e709a0b79cf806714f1342f.temp: Invalid argument
warning: unable to unlink C:/MumbleBuild/mumble/.git/modules/sbcelt-src/modules/celt-0.7.0/objects/6c/12497c77f31c290b43eadd12911349202ee2f3.temp: Invalid argument
warning: unable to unlink C:/MumbleBuild/mumble/.git/modules/sbcelt-src/modules/celt-0.7.0/objects/99/a19474a0eb683893664d60a33adc4dc3ec1e4f.temp: Invalid argument
powershell ./scripts/appveyor/appveyor-install-environment.ps1
Environment not cached. Downloading
Invoke-WebRequest : The remote server returned an error: (403) Forbidden.
At C:\MumbleBuild\mumble\scripts\appveyor\appveyor-install-environment.ps1:31 
char:5
+     Invoke-WebRequest -Uri $env_url -OutFile $cached_env
+     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : InvalidOperation: (System.Net.HttpWebRequest:Htt 
   pWebRequest) [Invoke-WebRequest], WebException
    + FullyQualifiedErrorId : WebCmdletWebResponseException,Microsoft.PowerShe 
   ll.Commands.InvokeWebRequestCommand
 
Command exited with code 1

@mkrautz mkrautz merged commit ea5c038 into mumble-voip:master Jul 13, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Kissaki Kissaki deleted the Kissaki:1924-address-types branch Jul 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.