threads / patch / 17592

patchvalgrind: do not require valgrind 3.4.0 or newer

Subject: [PATCH] valgrind: do not require valgrind 3.4.0 or newer

## tl;dr

9 messages between Feb 5, 2009 and Feb 6, 2009. Diffs are folded; open one to read it.

replies: 8people: 3as markdown or json

Johannes Schindelin· Feb 5, 2009, 18:34 UTC · lore

Valgrind 3.4.0 is pretty new, and even if --track-origins is a nice feature, it is not the end of the world if that is not available. So play nice and use that option only when only an older version of valgrind is available.

In the same spirit, refrain from the use of '...' in suppression files, which is also a feature only valgrind 3.4 and newer understand.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	This patch is on top of my valgrind series.

	In related news, Mark Adler has prepared a patch for the zlib vs 
	valgrind issue which has a low impact on performance, yet keeps 
	valgrind happy.

	I fully expect this patch to hit the next zlib version.

	Note that with the current suppressions, there was only a 
	hard-to-fix issue with certain gcc compilers in conjunction with 
	-DUNALIGNED_OK -O3 flags when compiling zlib, so not many users 
	are affected anyway (ahem, except for all the Ubuntu users, ahem).

 t/valgrind/default.supp |    4 +++-
 t/valgrind/valgrind.sh  |    8 +++++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

Show changes to 2 files +10 −2

t/valgrind/default.supp, t/valgrind/valgrind.sh

diff --git a/t/valgrind/default.supp b/t/valgrind/default.supp
index 5f341b8..9e013fa 100644
--- a/t/valgrind/default.supp
+++ b/t/valgrind/default.supp
@@ -38,6 +38,8 @@
 	writing-data-from-zlib-triggers-even-more-errors
 	Memcheck:Param
 	write(buf)
-	...
+	obj:/lib/ld-*.so
+	fun:write_in_full
+	fun:write_buffer
 	fun:write_loose_object
 }
diff --git a/t/valgrind/valgrind.sh b/t/valgrind/valgrind.sh
index dc92612..a77023a 100755
--- a/t/valgrind/valgrind.sh
+++ b/t/valgrind/valgrind.sh
@@ -2,11 +2,17 @@
 
 base=$(basename "$0")
 
+TRACK_ORIGINS=
+case "$(valgrind --version)" in
+valgrind-{3.[4-9],3.[1-3][0-9],[4-9],[1-3][0-9]}*)
+	TRACK_ORIGINS=--track-origins=yes
+esac
+
 exec valgrind -q --error-exitcode=126 \
 	--leak-check=no \
 	--suppressions="$GIT_VALGRIND/default.supp" \
 	--gen-suppressions=all \
-	--track-origins=yes \
+	$TRACK_ORIGINS \
 	--log-fd=4 \
 	--input-fd=4 \
 	$GIT_VALGRIND_OPTIONS \

-- 
1.6.1.1.598.g140d5

Peter Baumann· Feb 5, 2009, 19:00 UTC · re: Johannes Schindelin · lore

Re: [PATCH] valgrind: do not require valgrind 3.4.0 or newer

On Thu, Feb 05, 2009 at 07:34:27PM +0100, Johannes Schindelin wrote:

> Valgrind 3.4.0 is pretty new, and even if --track-origins is a nice
> feature, it is not the end of the world if that is not available.  So
> play nice and use that option only when only an older version of
> valgrind is available.

Reading the patch/the sentence above suggests that you actuallly ment:

"So play nice and don't use that option when only an older version of
 valgrind is available."

Greetings, Peter

Johannes Schindelin· Feb 5, 2009, 20:20 UTC · re: Peter Baumann · lore

Re: [PATCH] valgrind: do not require valgrind 3.4.0 or newer

Hi,

On Thu, 5 Feb 2009, Peter Baumann wrote:

Show 10 quoted lines

> On Thu, Feb 05, 2009 at 07:34:27PM +0100, Johannes Schindelin wrote:
> > Valgrind 3.4.0 is pretty new, and even if --track-origins is a nice
> > feature, it is not the end of the world if that is not available.  So
> > play nice and use that option only when only an older version of
> > valgrind is available.
> 
> Reading the patch/the sentence above suggests that you actuallly ment: 
> 
> "So play nice and don't use that option when only an older version of
>  valgrind is available."

Yes.  How did you guess?

:-)

Ciao, Dscho

Junio C Hamano· Feb 5, 2009, 20:32 UTC · re: Johannes Schindelin · lore

Re: [PATCH] valgrind: do not require valgrind 3.4.0 or newer

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> Valgrind 3.4.0 is pretty new, and even if --track-origins is a nice
> feature, it is not the end of the world if that is not available.  So
> play nice and use that option only when only an older version of
> valgrind is available.

s/older/newer/?

Show 5 quoted lines

> +TRACK_ORIGINS=
> +case "$(valgrind --version)" in
> +valgrind-{3.[4-9],3.[1-3][0-9],[4-9],[1-3][0-9]}*)
> +	TRACK_ORIGINS=--track-origins=yes
> +esac

What kind of case pattern is that?

Johannes Schindelin· Feb 5, 2009, 20:51 UTC · re: Junio C Hamano · lore

Re: [PATCH] valgrind: do not require valgrind 3.4.0 or newer

Hi,

On Thu, 5 Feb 2009, Junio C Hamano wrote:

Show 8 quoted lines

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > Valgrind 3.4.0 is pretty new, and even if --track-origins is a nice
> > feature, it is not the end of the world if that is not available.  So
> > play nice and use that option only when only an older version of
> > valgrind is available.
> 
> s/older/newer/?

s/only an older/a newer/  even.

Show 7 quoted lines

> > +TRACK_ORIGINS=
> > +case "$(valgrind --version)" in
> > +valgrind-{3.[4-9],3.[1-3][0-9],[4-9],[1-3][0-9]}*)
> > +	TRACK_ORIGINS=--track-origins=yes
> > +esac
> 
> What kind of case pattern is that?

A non-working one.

Grumbles, Dscho

Johannes Schindelin· Feb 5, 2009, 21:03 UTC · re: Junio C Hamano · lore

In the same spirit, refrain from the use of '...' in suppression files, which is also a feature only valgrind 3.4 and newer understand.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/valgrind/default.supp |    4 +++-
 t/valgrind/valgrind.sh  |   11 ++++++++++-
 2 files changed, 13 insertions(+), 2 deletions(-)

Show changes to 2 files +13 −2

t/valgrind/default.supp, t/valgrind/valgrind.sh

diff --git a/t/valgrind/default.supp b/t/valgrind/default.supp
index 5f341b8..9e013fa 100644
--- a/t/valgrind/default.supp
+++ b/t/valgrind/default.supp
@@ -38,6 +38,8 @@
 	writing-data-from-zlib-triggers-even-more-errors
 	Memcheck:Param
 	write(buf)
-	...
+	obj:/lib/ld-*.so
+	fun:write_in_full
+	fun:write_buffer
 	fun:write_loose_object
 }
diff --git a/t/valgrind/valgrind.sh b/t/valgrind/valgrind.sh
index dc92612..582b4dc 100755
--- a/t/valgrind/valgrind.sh
+++ b/t/valgrind/valgrind.sh
@@ -2,11 +2,20 @@
 
 base=$(basename "$0")
 
+TRACK_ORIGINS=
+
+VALGRIND_VERSION=$(valgrind --version)
+VALGRIND_MAJOR=$(expr "$VALGRIND_VERSION" : '[^0-9]*\([0-9]*\)')
+VALGRIND_MINOR=$(expr "$VALGRIND_VERSION" : '[^0-9]*[0-9]*\.\([0-9]*\)')
+test 3 -gt "$VALGRIND_MAJOR" ||
+test 3 -eq "$VALGRIND_MAJOR" -a 4 -gt "$VALGRIND_MINOR" ||
+TRACK_ORIGINS=--track-origins=yes
+
 exec valgrind -q --error-exitcode=126 \
 	--leak-check=no \
 	--suppressions="$GIT_VALGRIND/default.supp" \
 	--gen-suppressions=all \
-	--track-origins=yes \
+	$TRACK_ORIGINS \
 	--log-fd=4 \
 	--input-fd=4 \
 	$GIT_VALGRIND_OPTIONS \

-- 
1.6.1.1.636.gf819e

Junio C Hamano· Feb 6, 2009, 00:12 UTC · re: Johannes Schindelin · lore

Re: [PATCH] valgrind: do not require valgrind 3.4.0 or newer

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

Show 9 quoted lines

> Valgrind 3.4.0 is pretty new, and even if --track-origins is a nice
> feature, it is not the end of the world if that is not available.  So
> play nice and use that option only when only an older version of
> valgrind is available.
>
> In the same spirit, refrain from the use of '...' in suppression
> files, which is also a feature only valgrind 3.4 and newer understand.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Thanks.

Show 8 quoted lines

> +TRACK_ORIGINS=
> +
> +VALGRIND_VERSION=$(valgrind --version)
> +VALGRIND_MAJOR=$(expr "$VALGRIND_VERSION" : '[^0-9]*\([0-9]*\)')
> +VALGRIND_MINOR=$(expr "$VALGRIND_VERSION" : '[^0-9]*[0-9]*\.\([0-9]*\)')
> +test 3 -gt "$VALGRIND_MAJOR" ||
> +test 3 -eq "$VALGRIND_MAJOR" -a 4 -gt "$VALGRIND_MINOR" ||
> +TRACK_ORIGINS=--track-origins=yes

It took me a while to convince myself that

	"3 > major || (3 == major && 4 > minor) || do-this"

is equivalent to

	"if (3 < major || (3 == major && 4 <= minor)) { do-this }"

which would be:

	if test 3 -lt "$VALGRIND_MAJOR" ||
           test 3 -eq "$VALGRIND_MAJOR" -a 4 -le "$VALGRIND_MINOR"
        then
		TRACK_ORIGINS=--track-origins=yes
	fi
        
or more commonly:

	if test "$VALGRIND_MAJOR" -gt 3 ||
           test "$VALGRIND_MAJOR" -eq 3 -a "$VALGRIND_MINOR" -ge 4
        then
		TRACK_ORIGINS=--track-origins=yes
	fi

Johannes Schindelin· Feb 6, 2009, 00:40 UTC · re: Junio C Hamano · lore

Re: [PATCH] valgrind: do not require valgrind 3.4.0 or newer

Hi,

On Thu, 5 Feb 2009, Junio C Hamano wrote:

Show 46 quoted lines

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > Valgrind 3.4.0 is pretty new, and even if --track-origins is a nice
> > feature, it is not the end of the world if that is not available.  So
> > play nice and use that option only when only an older version of
> > valgrind is available.
> >
> > In the same spirit, refrain from the use of '...' in suppression
> > files, which is also a feature only valgrind 3.4 and newer understand.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> Thanks.
> 
> > +TRACK_ORIGINS=
> > +
> > +VALGRIND_VERSION=$(valgrind --version)
> > +VALGRIND_MAJOR=$(expr "$VALGRIND_VERSION" : '[^0-9]*\([0-9]*\)')
> > +VALGRIND_MINOR=$(expr "$VALGRIND_VERSION" : '[^0-9]*[0-9]*\.\([0-9]*\)')
> > +test 3 -gt "$VALGRIND_MAJOR" ||
> > +test 3 -eq "$VALGRIND_MAJOR" -a 4 -gt "$VALGRIND_MINOR" ||
> > +TRACK_ORIGINS=--track-origins=yes
> 
> It took me a while to convince myself that
> 
> 	"3 > major || (3 == major && 4 > minor) || do-this"
> 
> is equivalent to
> 
> 	"if (3 < major || (3 == major && 4 <= minor)) { do-this }"
> 
> which would be:
> 
> 	if test 3 -lt "$VALGRIND_MAJOR" ||
>            test 3 -eq "$VALGRIND_MAJOR" -a 4 -le "$VALGRIND_MINOR"
>         then
> 		TRACK_ORIGINS=--track-origins=yes
> 	fi
>         
> or more commonly:
> 
> 	if test "$VALGRIND_MAJOR" -gt 3 ||
>            test "$VALGRIND_MAJOR" -eq 3 -a "$VALGRIND_MINOR" -ge 4
>         then
> 		TRACK_ORIGINS=--track-origins=yes
> 	fi

Okay.  Want me to resubmit?

Ciao, Dscho

Junio C Hamano· Feb 6, 2009, 00:52 UTC · re: Johannes Schindelin · lore

Re: [PATCH] valgrind: do not require valgrind 3.4.0 or newer

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 52 quoted lines

> Hi,
>
> On Thu, 5 Feb 2009, Junio C Hamano wrote:
>
>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>> 
>> > Valgrind 3.4.0 is pretty new, and even if --track-origins is a nice
>> > feature, it is not the end of the world if that is not available.  So
>> > play nice and use that option only when only an older version of
>> > valgrind is available.
>> >
>> > In the same spirit, refrain from the use of '...' in suppression
>> > files, which is also a feature only valgrind 3.4 and newer understand.
>> >
>> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> 
>> Thanks.
>> 
>> > +TRACK_ORIGINS=
>> > +
>> > +VALGRIND_VERSION=$(valgrind --version)
>> > +VALGRIND_MAJOR=$(expr "$VALGRIND_VERSION" : '[^0-9]*\([0-9]*\)')
>> > +VALGRIND_MINOR=$(expr "$VALGRIND_VERSION" : '[^0-9]*[0-9]*\.\([0-9]*\)')
>> > +test 3 -gt "$VALGRIND_MAJOR" ||
>> > +test 3 -eq "$VALGRIND_MAJOR" -a 4 -gt "$VALGRIND_MINOR" ||
>> > +TRACK_ORIGINS=--track-origins=yes
>> 
>> It took me a while to convince myself that
>> 
>> 	"3 > major || (3 == major && 4 > minor) || do-this"
>> 
>> is equivalent to
>> 
>> 	"if (3 < major || (3 == major && 4 <= minor)) { do-this }"
>> 
>> which would be:
>> 
>> 	if test 3 -lt "$VALGRIND_MAJOR" ||
>>            test 3 -eq "$VALGRIND_MAJOR" -a 4 -le "$VALGRIND_MINOR"
>>         then
>> 		TRACK_ORIGINS=--track-origins=yes
>> 	fi
>>         
>> or more commonly:
>> 
>> 	if test "$VALGRIND_MAJOR" -gt 3 ||
>>            test "$VALGRIND_MAJOR" -eq 3 -a "$VALGRIND_MINOR" -ge 4
>>         then
>> 		TRACK_ORIGINS=--track-origins=yes
>> 	fi
>
> Okay.  Want me to resubmit?

Nah, sorry for being unclear that I was only stating an observation, not complaints.