Check TargetPlatform instead of HostPlatform for leading underscore
ClosedPublic

Authored by angerman on Mar 15 2017, 8:47 PM.

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.
angerman created this revision.Mar 15 2017, 8:47 PM
bgamari requested changes to this revision.Mar 16 2017, 10:13 AM

I think we should reassess the point of this check and name it more appropriately. It looks like we primarily use FP_LEADING_UNDERSCORE to form symbols names in the RTS. In light of this, I would have thought that this should be named FP_LEADING_UNDERSCORE_TARGET.

aclocal.m4
870

I'm honestly not sure what this test is supposed to be testing for: surely we should specify whether this is a test of the target or host. Moreover, perhaps we should be testing both.

880

It seems to me that we should either fail or fix the broken logic.

883–884

Do we know that this isn't going to break, e.g., OpenBSD?

This revision now requires changes to proceed.Mar 16 2017, 10:13 AM

Thanks for the review @bgamari, I tried to explain my reasoning a bit.

aclocal.m4
870

afaics this is for the -Wl,-u flags (see D3350), and to figure out if symbols we name by hand need to be prefixed with _ or not to be found.

We use this in the rts when we need the symbol name in the assembly. And as mentioned similarly to break the mutual dependencies between rts <-> base <-> prim.
(if we could use --start-group and --end-group, we would not need the -Wl,-u flags, however that seems to only be supported by gnu ld and gold. Not ld64
and I guess there are other linkers who don't support that either. lld does something else, where they record all symbols and solve mutual recursion that way, even though
the behaviour is not guaranteed to be identical to other linkers, they haven't found any issues with software in the wild. Or so they claim in http://llvm.org/devmtg/2016-03/Presentations/EuroLLVM%202016-%20New%20LLD%20linker%20for%20ELF.pdf).

880

The fix is to test properly on the target. We don't really care what the leading underscore on
the host (even though these names trip me up every time), as we are interested on the
behaviour of the target wrt to leading underscores.

Now this rarely pops up as an issue. Never if target == host obviously, but also never if your
target and host share the same behaviour (macOS to iOS), (linux to android), which is why
I believe this was never really caught.

883–884

How would this cause breakage on openbsd? I migh tbe missing something here, but I fail to see the potential implication :(

rwbarton requested changes to this revision.Mar 17 2017, 11:55 AM

This seems fine to me aside from the confusing comment. Did you mean that the case on HostPlatform gives the wrong result when cross-compiling (which you fixed), or that the AC_RUN_IFELSE gives the wrong result when cross-compiling? AC_RUN_IFELSE doesn't actually run anything when cross-compiling, so either way the new comment is wrong. (See https://www.gnu.org/software/autoconf/manual/autoconf-2.63/html_node/Runtime.html).

aclocal.m4
877–880

I'm just confused by this comment because it seems like it explains what is wrong with the old code, which you then fix; in which case we don't need the new comment.

883–884

I'm not sure what @bgamari means either. Using $TargetPlatform here just looks strictly more correct to me. (Assuming $HostPlatform and $TargetPlatform are subject to the same mangling, if any.)

914

The third argument of AC_RUN_IFELSE is the action to take when cross-compiling (instead of trying to run the program). So when we're cross-compiling, hopefully we guessed the correct value earlier, instead of going into this AC_RUN_IFELSE, at least if the value should be yes.

The conclusion is that when not cross-compiling, this patch should make no difference because TargetPlatform and HostPlatform will be equal anyways. When cross-compiling, the change in this patch can only make things worse when we were cross-compiling from a system we know uses leading underscores to a system we don't know uses leading underscores but actually does. The most likely cases of that are mac OS -> iOS cross-compiles, and we seem to handle all those cases already. If it works in other cases it's essentially by coincidence, and we should fix that by adding more known system types.

Or we could replace this test entirely by asking the (possibly cross-)compiler to compile a function call to assembly and check whether there is an underscore before the function name; or use nm, etc.

angerman updated this revision to Diff 11794.Mar 17 2017, 9:47 PM
angerman edited edge metadata.
  • Drop confusing comment.
angerman marked an inline comment as done.Mar 17 2017, 9:55 PM
angerman added inline comments.
aclocal.m4
877–880

Yes, I agree that the comment is adds only to the confusion. Removed.

914

Yes, trying to compile something for the target, and checking if the output actually contains an underscore or not seems like the proper solution, which would probably drop the need for the hacks above?

And yes, I believe you are correct that. The *-applie-ios case is never caught (unless someone really manged to actually run autotools/configure on ios?) but just works, as we compile on macOS, and hence by accident get the right selection.

After reading this a few more times, I'm now confused about my iOS compiler. As the target is arm-apple-darwin10 or aarch64-apple-darwin14. (I meant to clean this up and bring it more in line with
how the rest of the tooling calls this, (-ios).

angerman marked an inline comment as done.Mar 17 2017, 10:04 PM

iverilog seems to do this test as well, however more in line with what @rwbarton suggested:

https://github.com/nedwill/iverilog/blob/master/aclocal.m4#L22-L47

The following test can be found in https://opensource.apple.com/source/gcc/gcc-1495/libjava/libltdl/aclocal.m4.auto.html.

AC_DEFUN(AC_LTDL_SYMBOL_USCORE,
[dnl does the compiler prefix global symbols with an underscore?
AC_REQUIRE([AC_LTDL_GLOBAL_SYMBOL_PIPE])dnl
AC_MSG_CHECKING([for _ prefix in compiled symbols])
AC_CACHE_VAL(ac_cv_sys_symbol_underscore,
[ac_cv_sys_symbol_underscore=no
cat > conftest.$ac_ext <<EOF
void nm_test_func(){}
int main(){nm_test_func;return 0;}
EOF
if AC_TRY_EVAL(ac_compile); then
  # Now try to grab the symbols.
  ac_nlist=conftest.nm
  if AC_TRY_EVAL(NM conftest.$ac_objext \| $ac_cv_sys_global_symbol_pipe \> $ac_nlist) && test -s "$ac_nlist"; then
    # See whether the symbols have a leading underscore.
    if egrep '^. _nm_test_func' "$ac_nlist" >/dev/null; then
      ac_cv_sys_symbol_underscore=yes
    else
      if egrep '^. nm_test_func ' "$ac_nlist" >/dev/null; then
	:
      else
	echo "configure: cannot find nm_test_func in $ac_nlist" >&AC_FD_CC
      fi
    fi
  else
    echo "configure: cannot run $ac_cv_sys_global_symbol_pipe" >&AC_FD_CC
  fi
else
  echo "configure: failed program was:" >&AC_FD_CC
  cat conftest.c >&AC_FD_CC
fi
rm -rf conftest*
])
AC_MSG_RESULT($ac_cv_sys_symbol_underscore)
AC_LTDL_DLSYM_USCORE
])
rwbarton accepted this revision.Mar 23 2017, 5:33 PM
bgamari added inline comments.Mar 28 2017, 4:18 PM
aclocal.m4
883–884

I'm also not sure what @bgamari means here.

Presumably he had a point at the time but it has since fled his mind.

bgamari accepted this revision.Mar 29 2017, 4:23 PM
This revision is now accepted and ready to land.Mar 29 2017, 4:23 PM
This revision was automatically updated to reflect the committed changes.