Skip to content

Commit

Permalink
Patch for clang 10 and 11 fixing the load address of Valgrind, contri…
Browse files Browse the repository at this point in the history
…buted by Nick Briggs.
  • Loading branch information
paulfloyd committed Jun 6, 2020
1 parent b0ed733 commit 9748df5
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 37 deletions.
40 changes: 31 additions & 9 deletions configure.ac
Original file line number Diff line number Diff line change
Expand Up @@ -2638,30 +2638,52 @@ CFLAGS=$safe_CFLAGS
# will reside. -Ttext aligns just the .text section start (but not any
# other section).
#
# So test for -Ttext-segment which is supported by all bfd ld versions
# LLVM ld.lld 10.0 changed the semantics of its -Ttext. See "Breaking changes"
# in https://releases.llvm.org/10.0.0/tools/lld/docs/ReleaseNotes.html
# The --image-base option (since version 6.0?) provides the semantics needed.
# -Ttext-segment generates an error, but -Ttext now more closely
# follows the GNU (bfd) ld's -Ttext.
#
# So test first for --image-base support, and if that fails then
# for -Ttext-segment which is supported by all bfd ld versions
# and use that if it exists. If it doesn't exist it must be an older
# version of gold and we can fall back to using -Ttext which has the
# right semantics.

AC_MSG_CHECKING([if the linker accepts -Wl,-Ttext-segment])

safe_CFLAGS=$CFLAGS
CFLAGS="-static -nodefaultlibs -nostartfiles -Wl,-Ttext-segment=$valt_load_address_pri_norml -Werror"
AC_MSG_CHECKING([if the linker accepts -Wl,--image-base])

CFLAGS="-static -nodefaultlibs -nostartfiles -Wl,--image-base=$valt_load_address_pri_norml -Werror"

AC_LINK_IFELSE(
[AC_LANG_SOURCE([int _start () { return 0; }])],
[
linker_using_t_text="no"
AC_SUBST([FLAG_T_TEXT], ["-Ttext-segment"])
AC_SUBST([FLAG_T_TEXT], ["--image-base"])
AC_MSG_RESULT([yes])
], [
linker_using_t_text="yes"
AC_SUBST([FLAG_T_TEXT], ["-Ttext"])
AC_MSG_RESULT([no])
AC_MSG_CHECKING([if the linker accepts -Wl,-Ttext-segment])
CFLAGS="-static -nodefaultlibs -nostartfiles -Wl,-Ttext-segment=$valt_load_address_pri_norml -Werror"
AC_LINK_IFELSE(
[AC_LANG_SOURCE([int _start () { return 0; }])],
[
linker_using_t_text="no"
AC_SUBST([FLAG_T_TEXT], ["-Ttext-segment"])
AC_MSG_RESULT([yes])
], [
linker_using_t_text="yes"
AC_SUBST([FLAG_T_TEXT], ["-Ttext"])
AC_MSG_RESULT([no])
])
])

CFLAGS=$safe_CFLAGS

# If the linker only supports -Ttext (not -Ttext-segment) then we will
# If the linker only supports -Ttext (not -Ttext-segment or --image-base) then we will
# have to strip any build-id ELF NOTEs from the statically linked tools.
# Otherwise the build-id NOTE might end up at the default load address.
# (Pedantically if the linker is gold then -Ttext is fine, but newer
Expand All @@ -2687,7 +2709,7 @@ AC_LINK_IFELSE(
AC_MSG_RESULT([no])
])
else
AC_MSG_NOTICE([ld -Ttext-segment used, no need to strip build-id NOTEs.])
AC_MSG_NOTICE([ld --image-base or -Ttext-segment used, no need to strip build-id NOTEs.])
AC_SUBST([FLAG_NO_BUILD_ID], [""])
fi
CFLAGS=$safe_CFLAGS
Expand Down
50 changes: 22 additions & 28 deletions coregrind/link_tool_exe_freebsd.in
Original file line number Diff line number Diff line change
Expand Up @@ -19,32 +19,38 @@
# first arg
# the alternative load address
# all the rest of the args
# the gcc invokation to do the final link, that
# the compiler invocation to do the final link, that
# the build system would have done, left to itself
#
# We just let the script 'die' if something is wrong, rather than do
# proper error reporting. We don't expect the users to run this
# directly. It is only run as part of the build process, with
# carefully constrained inputs.
#
# Linux specific complications:
# FreeBSD specific complications:
#
# - need to support both old GNU ld and gold: use -Ttext= to
# set the text segment address.
#
# - need to pass --build-id=none (that is, -Wl,--build-id=none to
# gcc) if it accepts it, to ensure the linker doesn't add a
# notes section which ends up at the default load address and
# so defeats our attempts to keep that address clear for the
# client. However, older linkers don't support this flag, so it
# is tested for by configure.in and is shipped to us as part of
# argv[2 ..].
# - in the initial version of this file, the linker(s) it was targeted
# at supported only -Ttext to load the code at an alternative address,
# and did not require removing the build notes in order to function
# correctly, so the work done by configure to determine what should go
# into the FLAG_T_TEXT was ignored.
#
# - LLVM's ld.lld, for at least versions 8.0 (shipping with FreeBSD 12.1)
# and 9.0 support the -Ttext option and behave as desired. As of
# LLVM ld.lld version 10.0 a breaking change made -Ttext unusable,
# however the --image-base option has the desired semantics.
# It turns out that ld.lld has supported --image-base since at least
# as far back as version 8.0.
#
# So: what we actually do:
#
# pass the specified command to the linker as-is, except, add
# "-static" and "-Ttext=<argv[1]>" to it.
# "-static" and the value of FLAG_T_TEXT as determined by configure.
# Previously we did this by adding these options after the first
# word of the rest of the arguments, which works in the common case
# when it's something like "gcc". But the linker invocation itself
# might be multiple words, say if it's "ccache gcc". So we now put
# the new options at the end instead.
#

use warnings;
Expand All @@ -55,27 +61,15 @@ die "Not enough arguments"
if (($#ARGV + 1) < 5);

my $ala = $ARGV[0];
shift; # Remove $ala from @ARGV

# check for plausible-ish alt load address
die "Bogus alt-load address"
if (length($ala) < 3 || index($ala, "0x") != 0);

# The cc invokation to do the final link
my $cc = $ARGV[1];

# and the 'restargs' are argv[2 ..]

# so, build up the complete command here:
# 'cc' -static -Ttext='ala' 'restargs'

my $cmd="$cc -static -Wl,-Ttext=$ala";

# Add the rest of the parameters
foreach my $n (2 .. $#ARGV) {
$cmd = "$cmd $ARGV[$n]";
}
my $cmd = join(" ", @ARGV, "-static -Wl,@FLAG_T_TEXT@=$ala");

#print "link_tool_exe_linux: $cmd\n";
#print "link_tool_exe_freebsd: $cmd\n";


# Execute the command:
Expand Down

4 comments on commit 9748df5

@mattst88
Copy link

@mattst88 mattst88 commented on 9748df5 Jun 21, 2021

Choose a reason for hiding this comment

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

I ran into the problems with -Ttext with lld when building valgrind for ChromeOS, and after a lot of debugging I came to the same conclusions as documented in the comments of this patch.

Has there been an attempt to upstream this patch? If not, I'd be happy to do so.

@paulfloyd
Copy link
Owner Author

Choose a reason for hiding this comment

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

The long term goal is to upstream this entire repo. I'm hoping that this will happen for Valgrind 3.18 sometime in the next year. I'm expecting the debuginfo reader to be changed at some time (it's slow, hard to maintain and doesn't add much). Since that is a fairly significant source of differences between clang and GCC I'm waiting for that to be done before I start campaigning to add FreeBSD officially.

I do have upstream commit rights, so I can add this separately. If you'd like that I strongly recommend that you open an item in the Valgrind bugzilla. Just a warning though, there are only a small number of Valgrind developers with not a lot of spare time. So even this might be rather slow.

@mattst88
Copy link

Choose a reason for hiding this comment

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

Thanks much!

It indeed would be nice to have at least this patch upstreamed sooner, since that greatly simplifies the justification for cherry-picking the patch in downstream distributions.

Upstream valgrind bug filed as: https://bugs.kde.org/show_bug.cgi?id=439046
My analysis on the Google bug tracker: https://issuetracker.google.com/issues/191520718

@paulfloyd
Copy link
Owner Author

Choose a reason for hiding this comment

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

It looks like my entire patch set (including this) should be landing in the next week or so. I'll try to remember to update https://bugs.kde.org/show_bug.cgi?id=439046 around that time.

Please sign in to comment.