Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!
Bug 359993 - Please review patches for dev-util/valgrind-3.6.1 adding OSX support
Summary: Please review patches for dev-util/valgrind-3.6.1 adding OSX support
Status: RESOLVED FIXED
Alias: None
Product: Gentoo Linux
Classification: Unclassified
Component: New packages (show other bugs)
Hardware: AMD64 OS X
: Normal normal (vote)
Assignee: Anthony Basile
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: prefix-gx86
  Show dependency tree
 
Reported: 2011-03-22 18:53 UTC by Fabian Groffen
Modified: 2012-05-10 11:30 UTC (History)
2 users (show)

See Also:
Package list:
Runtime testing required: ---


Attachments
Fix for the 3.7.0 non-exec patch to un-break compilation on Darwin (3.7.0-non-exec-darwin.patch,1.02 KB, patch)
2012-03-23 19:42 UTC, Fabian Groffen
Details | Diff
Darwin support for the 3.7.0 ebuild (valgrind-3.7.0-r3.patch,1.28 KB, patch)
2012-03-23 19:45 UTC, Fabian Groffen
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fabian Groffen gentoo-dev 2011-03-22 18:53:19 UTC
Please review and comment on the following diff.  You won't like the conditional patching for the non-exec-stack stuff.  It needs some major work to get that fixed.  Perhaps just removing the unrelevant files (per arch) is a simpler approach here.


% cvs diff
cvs diff: Diffing .
Index: valgrind-3.6.1.ebuild
===================================================================
RCS file: /var/cvsroot/gentoo-x86/dev-util/valgrind/valgrind-3.6.1.ebuild,v
retrieving revision 1.1
diff -u -r1.1 valgrind-3.6.1.ebuild
--- valgrind-3.6.1.ebuild       19 Feb 2011 12:31:23 -0000      1.1
+++ valgrind-3.6.1.ebuild       22 Mar 2011 18:51:30 -0000
@@ -26,6 +26,10 @@
        sed -i -e 's:^AM_CFLAGS_BASE = :AM_CFLAGS_BASE = -fno-stack-protector :' \
                Makefile.all.am || die
 
+       # Don't force multiarch stuff on OSX
+       sed -i -e 's:-arch \(i386\|x86_64\)::g' \
+               Makefile.all.am || die
+
        # Correct hard coded doc location
        sed -i -e "s:doc/valgrind:doc/${PF}:" \
                docs/Makefile.am || die
@@ -35,7 +39,8 @@
 
        # Don't build in empty assembly files for other platforms or we'll get a QA
        # warning about executable stacks.
-       epatch "${FILESDIR}"/${PN}-3.6.0-non-exec-stack.patch
+       # (patch breaks building on Darwin)
+       [[ ${CHOST} == *-darwin* ]] || epatch "${FILESDIR}"/${PN}-3.6.0-non-exec-stack.patch
 
        # Fix up some suppressions that were not general enough for glibc versions
        # with more than just a major and minor number.
@@ -66,6 +71,8 @@
        if use amd64 || use ppc64; then
                ! has_multilib_profile && myconf="${myconf} --enable-only64bit"
        fi
+       use x86-macos && myconf="${myconf} --enable-only32bit"
+       use x64-macos && myconf="${myconf} --enable-only64bit"
 
        # Don't use mpicc unless the user asked for it (bug #258832)
        if ! use mpi; then
@@ -83,7 +90,15 @@
        emake DESTDIR="${D}" install || die
        dodoc AUTHORS FAQ.txt NEWS README*
 
-       pax-mark m "${D}"/usr/$(get_libdir)/valgrind/*-*-linux
+       pax-mark m "${ED}"/usr/$(get_libdir)/valgrind/*-*-linux
+
+       if [[ ${CHOST} == *-darwin* ]] ; then
+               # fix install_names on mis-named shared modules
+               local l
+               for l in "${ED}"/usr/lib/valgrind/*.so ; do
+                       install_name_tool -id "${EPREFIX}"/usr/lib/valgrind/${l##*/} "${l}"
+               done
+       fi
 }
 
 pkg_postinst() {
cvs diff: Diffing files
Comment 1 Anthony Basile gentoo-dev 2011-03-24 00:14:49 UTC
I assume this is in an effort to get portage into MacOSX?  Two points:

1) I'd like to see more work done on getting non-exec-stack working on darwin.  It the right thing to do even from the point of view of porting this.  I have access to MaxOSX but I don't have time to pursue this.

2) Whats ${ED} in the line

  pax-mark m "${ED}"/usr/$(get_libdir)/valgrind/*-*-linux

?  Why the change?
Comment 2 Jeremy Olexa (darkside) (RETIRED) archtester gentoo-dev Security 2011-03-24 00:36:35 UTC
(In reply to comment #1)
> I assume this is in an effort to get portage into MacOSX?  Two points:

Not quite, it is an effort to merge the ebuild for Gentoo Prefix back to gentoo-x86.

> 
> 1) I'd like to see more work done on getting non-exec-stack working on darwin. 
> It the right thing to do even from the point of view of porting this.  I have
> access to MaxOSX but I don't have time to pursue this.
> 
> 2) Whats ${ED} in the line
> 
>   pax-mark m "${ED}"/usr/$(get_libdir)/valgrind/*-*-linux
> 
> ?  Why the change?

ED is the offset in Gentoo Prefix. Essentially, ED=${D}/${EPREFIX} http://www.gentoo.org/proj/en/gentoo-alt/prefix/techdocs.xml#doc_chap2
Comment 3 Fabian Groffen gentoo-dev 2011-08-29 18:24:55 UTC
(In reply to comment #1)
> I assume this is in an effort to get portage into MacOSX?  Two points:
> 
> 1) I'd like to see more work done on getting non-exec-stack working on darwin. 
> It the right thing to do even from the point of view of porting this.  I have
> access to MaxOSX but I don't have time to pursue this.

As far as I understand from the Apple people, all stacks are already read-write-noexecute.
Comment 4 Fabian Groffen gentoo-dev 2012-03-23 19:42:27 UTC
Created attachment 306465 [details, diff]
Fix for the 3.7.0 non-exec patch to un-break compilation on Darwin
Comment 5 Fabian Groffen gentoo-dev 2012-03-23 19:45:06 UTC
Created attachment 306467 [details, diff]
Darwin support for the 3.7.0 ebuild

Please reconsider adding support for Prefix + Darwin to valgrind.

The changes are mostly the same as for 3.6, but this time instead of disabling the non-execstack patch on Darwin, I've fixed it so it doesn't break the compilation by requiring non-existing targets.

With these changes, valgrind produces some useful output on Darwin 9 (tested).
Comment 6 Anthony Basile gentoo-dev 2012-03-23 22:57:28 UTC
(In reply to comment #5)
> Created attachment 306467 [details, diff] [details, diff]
> Darwin support for the 3.7.0 ebuild
> 
> Please reconsider adding support for Prefix + Darwin to valgrind.
> 
> The changes are mostly the same as for 3.6, but this time instead of
> disabling the non-execstack patch on Darwin, I've fixed it so it doesn't
> break the compilation by requiring non-existing targets.
> 
> With these changes, valgrind produces some useful output on Darwin 9
> (tested).

Have you tested that both of these patches don't break valgrind on the other arches, mostly x86 and amd64.  I don't see that they should, they seem innocent enough, but just in case.  My only hesitation with this is that valgrind is undoubtedly the most fragil package I maintain.

Let me know and I'll apply to the 3.7.0 ~arch ebuilds.
Comment 7 Fabian Groffen gentoo-dev 2012-04-15 15:53:08 UTC
Sorry for the delay.  Yes it works.  I didn't observe any problems with my patch on amd64.
Comment 8 Anthony Basile gentoo-dev 2012-05-01 13:06:07 UTC
Okay this is in with valgrind-3.7.0-r4.ebuild.  I isolated the change in a rev bump.  I also added KEYWORDS="~x86-macos ~x64-macos" since I assume you tested on those arches.

If there's any problem please reopen.
Comment 9 Johan Hattne 2012-05-06 14:43:04 UTC
I'd like to have this bug reopened, because the patches eventually applied seem incomplete.  In particular, I'd like the patch from comment #4 to be applied, and also:

@@ -84,7 +84,7 @@
 }
 
 src_install() {
-       emake DESTDIR="${ED}" install
+       emake DESTDIR="${D}" install
        dodoc AUTHORS FAQ.txt NEWS README*
 
        pax-mark m "${ED}"/usr/$(get_libdir)/valgrind/*-*-linux

to fix a double prefix issue while installing.  Passed test on x64-macos and amd64-linux.
Comment 10 Fabian Groffen gentoo-dev 2012-05-10 09:13:04 UTC
ebuild is indeed broken for Prefix, regardless Darwin.
Comment 11 Fabian Groffen gentoo-dev 2012-05-10 09:22:24 UTC
@blueness, I took the liberty to fix these issues.  You added files/valgrind-3.7.0-non-exec-darwin.patch but never applied it.  Since it was a patch to your exec patch, I applied it to the patch as valgrind-3.7.0-non-exec-stack-v2.patch and used that in -r4 instead.
Comment 12 Anthony Basile gentoo-dev 2012-05-10 11:30:36 UTC
(In reply to comment #11)
> @blueness, I took the liberty to fix these issues.  You added
> files/valgrind-3.7.0-non-exec-darwin.patch but never applied it.  Since it
> was a patch to your exec patch, I applied it to the patch as
> valgrind-3.7.0-non-exec-stack-v2.patch and used that in -r4 instead.

I looked at the changes and that's fine.  Now I see what happened.  Normally I test but I don't have prefix set up anywhere.