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

Use DWARF unwinding with Clang to avoid overproduction of linking warnings with clang + gfortran #18

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

GiudGiud
Copy link

jwpeterson
jwpeterson previously approved these changes Jan 30, 2025
Copy link
Member

@jwpeterson jwpeterson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have any issues with this, but I also don't use clang on Darwin so I don't have an easy way of testing it. @roystgnr should we just merge this and propagate the changes to libmesh to make it easier for @GiudGiud to actually test it in MOOSE?

Copy link
Member

@roystgnr roystgnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GiudGiud reports verifying that this fixed his warnings for clang on Darwin. I'm just worried that it might cause problems on other configurations. We're passing linker options here based on autodetection of compiler type, right? But I believe it's common for a system to combine LLVM compilers (which would cause us to detect clang) with non-LLVM linkers (which might scream and die if they don't recognize -keep_dwarf_unwind -no_compact_unwind).

And ... I was going to test my theory manually, but it looks like that's what already happened here: https://civet.inl.gov/job/2659310/

The configure script misdiagnoses a failure to link to libhdf5 as "HDF5 not found", but if you look at the immediate failure then it's clearer:

/usr/bin/ld: unrecognized option '-keep_dwarf_unwind'
/usr/bin/ld: use the --help option for usage information
clang-15: error: linker command failed with exit code 1 (use -v to see invocation)

So we can't safely set these flags unless we verify them first at configure time, e.g. by temporarily setting them in LDFLAGS and using AC_TRY_LINK on a "Hello world" program to make sure they're usable.

@jwpeterson
Copy link
Member

Yes, definitely sounds like an AC_LINK_IFELSE (I think that's the preferred one) type test is called for here.

@GiudGiud
Copy link
Author

something like that?

@GiudGiud
Copy link
Author

Should I add a message in the "yes" case too?

@roystgnr
Copy link
Member

I'd think we only want a message in the yes case, personally. If we don't add any flags, maybe there's a problem, but "your environment has problems unless we carefully find the right flags to overcome them" sounds like a "your environment" problem we don't need to bug the user about rather than a "we" problem we do. But if we do add flags, even with the best of intentions, ... it's maybe not morally required to talk about what we're doing, but it's at least polite. Something like "retaining dwarf unwinding for gfortran compatibility" that would fit on one line, leaving the more detailed explanations for the .m4 comment, maybe?

@jwpeterson
Copy link
Member

I think the way AC_LINK_IFELSE works is that you first set the LIBS variable to the flags you want to try, then you call AC_LINK_IFELSE and if it passes you enable the flags in the libmesh LDFLAGS, otherwise you don't. There's an example in xdr.m4:

AC_DEFUN([TEST_XDR],
[
  old_CPPFLAGS="$CPPFLAGS"
  old_LIBS="$LIBS"
  CPPFLAGS="$CPPFLAGS $XDRINCLUDES"
  LIBS="$LIBS $XDRLINKLIBS"

  AC_LINK_IFELSE([AC_LANG_PROGRAM([@%:@include <stdio.h>
                                   @%:@include <rpc/rpc.h>
                                   @%:@include <rpc/xdr.h>],
                                  [
                                    XDR * xdr;
                                    FILE * fp;
                                    xdrstdio_create(xdr, fp, XDR_ENCODE);
                                  ])],
                 [
                   AC_MSG_RESULT(yes)
                   AC_DEFINE(HAVE_XDR, 1, [Flag indicating headers and libraries for XDR IO are available])
                   enablexdr=yes
                 ],
                 [
                   AC_MSG_RESULT(no)
                   enablexdr=no
                 ])

  dnl Reset flags after testing
  CPPFLAGS="$old_CPPFLAGS"
  LIBS="$old_LIBS"
])

@GiudGiud GiudGiud force-pushed the PR_clang branch 2 times, most recently from b4a0318 to 5358f41 Compare January 30, 2025 17:55
@GiudGiud
Copy link
Author

I think the way AC_LINK_IFELSE works is that you first set the LIBS variable to the flags you want to try, then you call AC_LINK_IFELSE and if it passes you enable the flags in the libmesh LDFLAGS, otherwise you don't. There's an example in xdr.m4:

I think I have something similar to the example now

dnl On Darwin with clang + gfortran, we get very many warnings for compact unwinding issues
dnl We deliberately keep relying on the less performant dwarf unwinding until the over-production of warnings is solved.
OLD_ACSM_LDFLAGS="$ACSM_LDFLAGS"
ACSM_LDFLAGS+=" -Wl,-femit-dwarf-unwind=no_compact_unwind"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand it, AC_LINK_IFELSE specifically uses the variables LIBS and LDFLAGS to test linking:

https://www.gnu.org/software/autoconf/manual/autoconf-2.69/html_node/Running-the-Linker.html

So I think you need to do like in the xdr example, and set the LIBS variable prior to calling AC_LINK_IFELSE. Also, it seems to me that there is an issue with your test program, it will need to #include <iostream> in order to be well-formed.

@jwpeterson jwpeterson self-requested a review February 3, 2025 15:07
@jwpeterson jwpeterson dismissed their stale review February 3, 2025 15:08

PR not working yet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants