Set `USE_MMAP` at configure time
ClosedPublic

Authored by erikd on May 15 2016, 10:57 PM.

Details

Summary

The USE_MMAP macro is used in the run time linker and was being set with
some really ugly CPP hackery. Setting in the configure script is much
neater.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
erikd updated this revision to Diff 7593.May 15 2016, 10:57 PM
erikd retitled this revision from to Set `USE_MMAP` at configure time.
erikd updated this object.
erikd edited the test plan for this revision. (Show Details)
erikd added reviewers: hvr, bgamari, rwbarton.
erikd updated this revision to Diff 7594.May 16 2016, 12:50 AM
erikd edited edge metadata.

Fix author address

hvr awarded a token.May 16 2016, 2:33 AM
hvr requested changes to this revision.May 16 2016, 2:37 AM
hvr edited edge metadata.

thanks for doing this!

configure.ac
1085

never heard of Winodws before... :-)

1090

I wonder if we should maybe use a longer macro-define (e.g. RTS_LINKER_USER_MMAP) to reduce the risk of name-clashes with USE_MMAP somewhere... USE_MMAP will now become globally visible when GHC C headers get included...

This revision now requires changes to proceed.May 16 2016, 2:37 AM
erikd updated this revision to Diff 7602.May 16 2016, 3:51 AM
erikd edited edge metadata.
  • Rename USE_MMAP to RTS_LINKER_USE_MMAP.
  • Fix "Winodws" typo.
hvr accepted this revision.May 16 2016, 4:31 AM
hvr edited edge metadata.

see also inline comment, but other than that:

lgtm

configure.ac
1078

please rewrite

if test ${TargetArch} != "powerpc" ; then

into

if test "$TargetArch" != "powerpc" ; then

for consistency (btw, what about powerpc64 on darwin? -- otoh, I think we should just drop Darwin/PPC support unless there's at least one user who can actually still test/run it)

This revision is now accepted and ready to land.May 16 2016, 4:31 AM
erikd updated this revision to Diff 7603.May 16 2016, 4:46 AM
erikd edited edge metadata.
  • Improve code consistency
  • Fix comment typo
bgamari accepted this revision.May 16 2016, 6:39 AM
bgamari edited edge metadata.

Yay! Death to CPP!

Looks good to me.

configure.ac
1090

Agreed; if nothing else it better conveys the macro's meaning.

This revision was automatically updated to reflect the committed changes.
simonmar edited edge metadata.May 17 2016, 7:42 AM

Someone will have to explain to me why this is better :)

  • The configure tests don't actually probe the system, apart from testing the achitecture, which is exactly what the CPP did before.
  • We still have exactly the same amount of conditional compilation
  • And now the code is spread over more places

I personally find the bash much easier to read than the previous CPP expression but yes, you are right, things are a bit more decentralized now.

hvr added a comment.May 17 2016, 8:23 AM
  • The configure tests don't actually probe the system,

...but now we're in a better position to do so.

Moreover, previously I had to run the compiler to figure out what the setting of USE_MMAP was, now I can look at ghcconfig.h and easily tweak that. This can now also become a configure --disable-rts-linker-use-mmap flag, etc

Fair enough, I had a suspicion the answer might be "because it helps us do X and Y in the future.." but I wasn't sure what X and Y you had in mind.