Page MenuHomePhabricator

macros used to detect mmap functions
Closed, ResolvedPublic

Description

when grepping code, we can see that HAVE_SYS_MMAN_H and HAVE_MMAP are defined in config.h
meson.build also defines HAVE_MMAN_H wich is never used.

that is a lot for just one usage.

what about simplifying all this ? I would say just using HAVE_MMAP (which is detected by including sys/mmap.h in meson.build) to include sys/mman.h and for the usage of mmap, munmap, mremap and mprotect.

What do you think of this request ?

vtorri created this task.Aug 3 2019, 6:27 AM
vtorri triaged this task as High priority.

does mprotect work on windows? mprotect is the newer of those. the rest are old (unix/posix-wise) with the exception of mremap which is linux-only.

so what i see is a generic HAVE_MMAP for mmap/munmap. we don't use mremap so... in theory a HAVE_MREMAP but we don't need it and a HAVE_MPROTECT because it's rather new compared to mmap/mumap. these days osx/ios certainly have mprotect. i would assume bsd has it too and linux. that's pretty much everything, so the question is more windows - mprotect there?

Just from a consistency POV:

  • HAVE_XXXX_H is used to express that header XXXX is available on that system,
  • HAVE_YYYY is used to express that function YYYY is available.

So HAVE_MMAP would express that mmap() is there, i would not bind it to more functions. In regards to HAVE_MMAP_H: this is something weird that someone introduced at some point, if you want to replace those #ifdef's, feel free to. But i would not merge more function checking into a single #define.

vtorri added a comment.EditedAug 4 2019, 6:56 AM

@raster VirtualProtect can be used. Also, mprotect exists in FreeBSD since BSD 4.4 [1](quite old), and is in POSIX 2001 [2] and solaris

[1] https://www.freebsd.org/cgi/man.cgi?sektion=2&query=mprotect
[2] https://linux.die.net/man/2/mprotect

so we can use HAVE_MMAP for mprotect too, i think

@bu5hm4n don't you think that we can use HAVE_MMAP to include sys/mman.h, as the detection of mmap is successful if it is in sys/mman.h ?

vtorri added a comment.Aug 4 2019, 8:15 AM

@bu5hm4n you added the definition of HAVE_MMAN_H : https://git.enlightenment.org/core/efl.git/commit/header_checks/meson.build?id=46d464e5bfc10398461a33a2256c1c58d509dd1a

here is where it is used :

$ git grep HAVE_MMAN_H
header_checks/meson.build: config_h.set10('HAVE_MMAN_H', true)
src/lib/elementary/efl_ui_selection_manager.c:#ifdef HAVE_MMAN_H
src/lib/elementary/efl_ui_selection_manager.c:#ifdef HAVE_MMAN_H

I know where it is defined & used, that is why I say "if you want to replace those #ifdef's, feel free to"

raster added a comment.Aug 4 2019, 2:44 PM

@vtorri - i can't seem to confirm minw32 has mprotect ... and evil doesn't. so are you saying that they are functionally equivalent (VirtualProtect and mprotect) and so evil could implement an mprotect on windows?

vtorri added a comment.Aug 4 2019, 9:04 PM

@raster yes, they are functionnaly equivalent. i've already implemented it on my computer. I'll provide a diff soon for it

vtorri added a comment.Aug 5 2019, 7:01 AM

also, there are a lot of files that include sys/mman.h without guarding it with HAVE_SYS_MMAN_H. They must be guarded, right ?

raster added a comment.Aug 6 2019, 1:36 AM

this means someone is providing mmanh.h even on windows... the rest (osx,bsd, linux) have it for sure. android will too and ios, solaris... not sure who doesn't have it that we care about then... except windows and that seems to be there?

vtorri added a comment.Aug 6 2019, 3:07 AM

there is no mman.h on Windows, only in Evil. It's currently exported. I want to make it private in Evil (creates an evil_mman.h, included in evil_private.h), then :

#ifdef HAVE_SYS_MMAN_H
# include <sys/mman.h>
#endif

#ifdef _WIN32
# include <evil_private.h> /* mmap */
#endif

so for efl there is no use of knowing if mman.h exists (HAVE_MMAN_H) as it always will. evil guarantees that. so we just need more

#ifdef _WIN32
#  include <evil_private.h> /* mmap */
#endif
#  include <sys/mman.h>
#endif

because CFLAGS etc. will be set up to have the evil includes path and thus always provide sys/mman.h ... :) if evil_private does not include mman.h for us we can get rid of the #else entirely and just always include mman.h ... :) for efl's use. not "public headers" i mean.

vtorri added a comment.Aug 8 2019, 3:13 AM

i plan to remove sys/mman.h from evil and just a private evil_mman.h (included by evil_private.h)

diff to review : #9542

vtorri closed this task as Resolved.Aug 19 2019, 11:36 AM