fix -Wunused-but-set-variable for gcc-4.6 on x64

fix -Wunused-but-set-variable for gcc-4.6 on x64

* src/third_party/valgrind/valgrind.h: Update from upstream valgrind
  r11899, so as to get around some unused value warnings.  Also adds
  support for darwin.

  This version of valgrind.h differs from the original in that all
  instances of "unsigned long long int" have been replaced with
  "uint64_t", as the former is not allowed in ISO C++ 89.

  See https://bugs.kde.org/show_bug.cgi?id=211926 for the upstream bug
  report.

* src/x64/cpu-x64.cc:
* src/builtins.cc:
* src/conversions-inl.h:
* src/debug.cc:
* src/frames.cc:
* src/full-codegen.cc:
* src/jsregexp.cc:
* src/objects.cc:
* src/parser.cc:
* src/platform-linux.cc:
* src/x64/code-stubs-x64.cc:
* src/x64/deoptimizer-x64.cc:
* src/x64/full-codegen-x64.cc:
* src/x64/lithium-codegen-x64.cc:
* src/x64/regexp-macro-assembler-x64.cc:
* src/x64/stub-cache-x64.cc: Remove a number of assigned but
  unreferenced variables.

* SConstruct (CCTEST_EXTRA_FLAGS): Punt on -Wunused-but-set-variable for
  the test suite.

BUG=1291
TEST=A build and tools/test.py passes.

Committed: http://code.google.com/p/v8/source/detail?r=8688

[William Hesse, 2011-07-19 12:16:43]

Are the changes to Valgrind a necessary part of this change, to get rid of the
error, or are they a different change, that got mixed in?

All except the Valgrind changes look good to me, with the comment below.  If you
want the valgrind changes reviewed and approved as part of this change, let me
know, and I will review them (though I've never looked at Valgrind before).

http://codereview.chromium.org/7400023/diff/1/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

http://codereview.chromium.org/7400023/diff/1/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:2500: Register arg1 = rdi;
This is so asymmetric, and weird, that I would prefer a USE(arg1); line below,
instead of not declaring it.
I assume that the USE() macro also gets rid of unused variable errors?

[William Hesse, 2011-07-19 12:20:54]

Oops - nevermind.  I didn't look at the review message.

The Valgrind version update is fine, except why is the (void) cast necessary in
cpu-x64.cc.  We tend to use static_cast, instead of c-style casts.  Is there any
other place in our code where we cast to void?  I would use a static_cast.

Also, I didn't check the last 4 files yet.

On 2011/07/19 12:16:43, William Hesse wrote:
> Are the changes to Valgrind a necessary part of this change, to get rid of the
> error, or are they a different change, that got mixed in?
> 
> All except the Valgrind changes look good to me, with the comment below.  If
you
> want the valgrind changes reviewed and approved as part of this change, let me
> know, and I will review them (though I've never looked at Valgrind before).
> 
> http://codereview.chromium.org/7400023/diff/1/src/x64/code-stubs-x64.cc
> File src/x64/code-stubs-x64.cc (right):
> 
>
http://codereview.chromium.org/7400023/diff/1/src/x64/code-stubs-x64.cc#newco...
> src/x64/code-stubs-x64.cc:2500: Register arg1 = rdi;
> This is so asymmetric, and weird, that I would prefer a USE(arg1); line below,
> instead of not declaring it.
> I assume that the USE() macro also gets rid of unused variable errors?

[William Hesse, 2011-07-19 12:33:20]

The last 4 files are also fine, but the Valgrind update is not.  Valgrind is
used across the whole Chromium project, so that the entire browser is compiled
with Valgrind.  So unless Valgrind is updated to the new version in Chromium as
a whole, I don't think a new header file in V8 will be compatible.

Perhaps you could leave the Valgrind untouched, and enable the
unused-but-set-variable only for the non-Valgrind builds?




On 2011/07/19 12:20:54, William Hesse wrote:
> Oops - nevermind.  I didn't look at the review message.
> 
> The Valgrind version update is fine, except why is the (void) cast necessary
in
> cpu-x64.cc.  We tend to use static_cast, instead of c-style casts.  Is there
any
> other place in our code where we cast to void?  I would use a static_cast.
> 
> Also, I didn't check the last 4 files yet.
> 
> On 2011/07/19 12:16:43, William Hesse wrote:
> > Are the changes to Valgrind a necessary part of this change, to get rid of
the
> > error, or are they a different change, that got mixed in?
> > 
> > All except the Valgrind changes look good to me, with the comment below.  If
> you
> > want the valgrind changes reviewed and approved as part of this change, let
me
> > know, and I will review them (though I've never looked at Valgrind before).
> > 
> > http://codereview.chromium.org/7400023/diff/1/src/x64/code-stubs-x64.cc
> > File src/x64/code-stubs-x64.cc (right):
> > 
> >
>
http://codereview.chromium.org/7400023/diff/1/src/x64/code-stubs-x64.cc#newco...
> > src/x64/code-stubs-x64.cc:2500: Register arg1 = rdi;
> > This is so asymmetric, and weird, that I would prefer a USE(arg1); line
below,
> > instead of not declaring it.
> > I assume that the USE() macro also gets rid of unused variable errors?

[wingo, 2011-07-19 13:47:32]

On 2011/07/19 12:20:54, William Hesse wrote:
> The Valgrind version update is fine, except why is the (void) cast necessary
in
> cpu-x64.cc.  We tend to use static_cast, instead of c-style casts.  Is there
any
> other place in our code where we cast to void?  I would use a static_cast.

Fixed to use USE instead of a cast to void.  A static_cast<void> would have
worked too.

[wingo, 2011-07-19 13:52:10]

Thanks for the review, and sorry for the multiple mails; I'm not used to
rietveld yet.

Regarding valgrind, I doubt it would have an impact on Chrome.  As you probably
know, the only purpose of the valgrind.h include is to emit some seemingly
useless instructions into FlushICache.  When run under valgrind, those
instructions are recognized by the valgrind VM as indicating that (in this case)
valgrind should flush part of its caches.

valgrind.h is not installed.  I don't know if Chrome includes it; it shouldn't,
though.  So updating the valgrind.h there in no way links v8 to a particular
version of valgrind.  Additionally I don't think that the instruction sequence
has changed, so older valgrind should be just as happy; though I have not tested
this.

The one change that could be problematic would be the emission of these
instructions on Darwin now, whereas before I don't think that they were emitted.

I am happy to remove the valgrind part of that patch if that is what you want. 
But I wanted to clarify that we were all on the same page before doing so.

Thanks again!

Andy

http://codereview.chromium.org/7400023/diff/1/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

http://codereview.chromium.org/7400023/diff/1/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:2500: Register arg1 = rdi;
On 2011/07/19 12:16:43, William Hesse wrote:
> This is so asymmetric, and weird, that I would prefer a USE(arg1); line below,
> instead of not declaring it.

Ah, I had not seen USE; thanks for the pointer. It's a template actually. 
Fixed.

[William Hesse, 2011-07-19 14:31:49]

Let's remove Valgrind and the -wunused-value warnings change, and commit the
rest.  Then we can run trybot runs on Chromium valgrind with the Valgrind
changes.  Have you checked to see if we are already on a different version than
Chromium?  If so, then we can obviously change our version.




On 2011/07/19 13:52:10, wingo wrote:
> Thanks for the review, and sorry for the multiple mails; I'm not used to
> rietveld yet.
> 
> Regarding valgrind, I doubt it would have an impact on Chrome.  As you
probably
> know, the only purpose of the valgrind.h include is to emit some seemingly
> useless instructions into FlushICache.  When run under valgrind, those
> instructions are recognized by the valgrind VM as indicating that (in this
case)
> valgrind should flush part of its caches.
> 
> valgrind.h is not installed.  I don't know if Chrome includes it; it
shouldn't,
> though.  So updating the valgrind.h there in no way links v8 to a particular
> version of valgrind.  Additionally I don't think that the instruction sequence
> has changed, so older valgrind should be just as happy; though I have not
tested
> this.
> 
> The one change that could be problematic would be the emission of these
> instructions on Darwin now, whereas before I don't think that they were
emitted.
> 
> I am happy to remove the valgrind part of that patch if that is what you want.

> But I wanted to clarify that we were all on the same page before doing so.
> 
> Thanks again!
> 
> Andy
> 
> http://codereview.chromium.org/7400023/diff/1/src/x64/code-stubs-x64.cc
> File src/x64/code-stubs-x64.cc (right):
> 
>
http://codereview.chromium.org/7400023/diff/1/src/x64/code-stubs-x64.cc#newco...
> src/x64/code-stubs-x64.cc:2500: Register arg1 = rdi;
> On 2011/07/19 12:16:43, William Hesse wrote:
> > This is so asymmetric, and weird, that I would prefer a USE(arg1); line
below,
> > instead of not declaring it.
> 
> Ah, I had not seen USE; thanks for the pointer. It's a template actually. 
> Fixed.

[wingo, 2011-07-19 14:40:37]

On 2011/07/19 14:31:49, William Hesse wrote:
> Let's remove Valgrind and the -wunused-value warnings change, and commit the
> rest.  Then we can run trybot runs on Chromium valgrind with the Valgrind
> changes.  Have you checked to see if we are already on a different version
than
> Chromium?  If so, then we can obviously change our version.

It seems that the valgrind.h in base/third_party/valgrind has already been
updated, see http://codereview.chromium.org/6327018/.

I have pushed an updated version of this patch without the valgrind changes.

[William Hesse, 2011-07-20 08:02:05]

LGTM.  But what is with the SConstruct flag - is it turning off the warning
because Valgrind still fails it, or is it turning on the warning?  If it is
turning off the warning, will it be turned on with the Valgrind commit?

I'll commit this, and then you can post a CL with the Valgrind changes.




On 2011/07/19 14:40:37, wingo wrote:
> On 2011/07/19 14:31:49, William Hesse wrote:
> > Let's remove Valgrind and the -wunused-value warnings change, and commit the
> > rest.  Then we can run trybot runs on Chromium valgrind with the Valgrind
> > changes.  Have you checked to see if we are already on a different version
> than
> > Chromium?  If so, then we can obviously change our version.
> 
> It seems that the valgrind.h in base/third_party/valgrind has already been
> updated, see http://codereview.chromium.org/6327018/.
> 
> I have pushed an updated version of this patch without the valgrind changes.

[William Hesse, 2011-07-20 08:11:22]

Oh, now I see in the comments why you are turning off the warning - it is just
turned off for the cctests.

On 2011/07/20 08:02:05, William Hesse wrote:
> LGTM.  But what is with the SConstruct flag - is it turning off the warning
> because Valgrind still fails it, or is it turning on the warning?  If it is
> turning off the warning, will it be turned on with the Valgrind commit?
> 
> I'll commit this, and then you can post a CL with the Valgrind changes.
> 
> 
> 
> 
> On 2011/07/19 14:40:37, wingo wrote:
> > On 2011/07/19 14:31:49, William Hesse wrote:
> > > Let's remove Valgrind and the -wunused-value warnings change, and commit
the
> > > rest.  Then we can run trybot runs on Chromium valgrind with the Valgrind
> > > changes.  Have you checked to see if we are already on a different version
> > than
> > > Chromium?  If so, then we can obviously change our version.
> > 
> > It seems that the valgrind.h in base/third_party/valgrind has already been
> > updated, see http://codereview.chromium.org/6327018/.
> > 
> > I have pushed an updated version of this patch without the valgrind changes.