* [PATCH] gold: remove false const from Target::do_adjust_elf_header
@ 2013-10-11 18:46 Roland McGrath
2013-10-11 21:26 ` Cary Coutant
0 siblings, 1 reply; 5+ messages in thread
From: Roland McGrath @ 2013-10-11 18:46 UTC (permalink / raw)
To: binutils
The claim of constness is false for Target_arm (because it calls
set_processor_specific_flags), so it should not be claimed for the
general case.
OK for trunk and 2.24?
Thanks,
Roland
gold/
* target.h (Target::adjust_elf_header, Target::adjust_elf_header):
Remove const from declaration.
* target.cc (Sized_target::do_adjust_elf_header): Update definition.
* sparc.cc (Target_sparc::do_adjust_elf_header): Likewise.
* parameters.h (Parameters::target): Make return value non-const.
--- a/gold/parameters.h
+++ b/gold/parameters.h
@@ -99,7 +99,7 @@ class Parameters
{ return this->target_ != NULL; }
// The target of the output file we are generating.
- const Target&
+ Target&
target() const
{
gold_assert(this->target_valid());
--- a/gold/sparc.cc
+++ b/gold/sparc.cc
@@ -217,7 +217,7 @@ class Target_sparc : public Sized_target<size, big_endian>
const elfcpp::Ehdr<size, big_endian>& ehdr);
void
- do_adjust_elf_header(unsigned char* view, int len) const;
+ do_adjust_elf_header(unsigned char* view, int len);
private:
@@ -4341,7 +4341,7 @@ template<int size, bool big_endian>
void
Target_sparc<size, big_endian>::do_adjust_elf_header(
unsigned char* view,
- int len) const
+ int len)
{
elfcpp::Ehdr_write<size, big_endian> oehdr(view);
--- a/gold/target.cc
+++ b/gold/target.cc
@@ -1,6 +1,6 @@
// target.cc -- target support for gold.
-// Copyright 2009, 2010, 2011 Free Software Foundation, Inc.
+// Copyright 2009, 2010, 2011, 2013 Free Software Foundation, Inc.
// Written by Doug Kwan <dougkwan@google.com>.
// This file is part of gold.
@@ -219,7 +219,7 @@ Target::do_plt_fde_location(const Output_data*
plt, unsigned char*,
template<int size, bool big_endian>
void
Sized_target<size, big_endian>::do_adjust_elf_header(unsigned char* view,
- int len) const
+ int len)
{
elfcpp::ELFOSABI osabi = this->osabi();
if (osabi != elfcpp::ELFOSABI_NONE)
--- a/gold/target.h
+++ b/gold/target.h
@@ -1,6 +1,6 @@
// target.h -- target support for gold -*- C++ -*-
-// Copyright 2006, 2007, 2008, 2009, 2010, 2011, 2012
+// Copyright 2006, 2007, 2008, 2009, 2010, 2011, 2012, 2013
// Free Software Foundation, Inc.
// Written by Ian Lance Taylor <iant@google.com>.
@@ -238,7 +238,7 @@ class Target
// Adjust the output file header before it is written out. VIEW
// points to the header in external form. LEN is the length.
void
- adjust_elf_header(unsigned char* view, int len) const
+ adjust_elf_header(unsigned char* view, int len)
{ return this->do_adjust_elf_header(view, len); }
// Return address and size to plug into eh_frame FDEs associated with a PLT.
@@ -548,7 +548,7 @@ class Target
// By default, we set the EI_OSABI field if requested (in
// Sized_target).
virtual void
- do_adjust_elf_header(unsigned char*, int) const = 0;
+ do_adjust_elf_header(unsigned char*, int) = 0;
// Return address and size to plug into eh_frame FDEs associated with a PLT.
virtual void
@@ -1018,7 +1018,7 @@ class Sized_target : public Target
// Set the EI_OSABI field if requested.
virtual void
- do_adjust_elf_header(unsigned char*, int) const;
+ do_adjust_elf_header(unsigned char*, int);
// Handle target specific gc actions when adding a gc reference.
virtual void
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] gold: remove false const from Target::do_adjust_elf_header
2013-10-11 18:46 [PATCH] gold: remove false const from Target::do_adjust_elf_header Roland McGrath
@ 2013-10-11 21:26 ` Cary Coutant
2013-10-11 21:45 ` Roland McGrath
0 siblings, 1 reply; 5+ messages in thread
From: Cary Coutant @ 2013-10-11 21:26 UTC (permalink / raw)
To: Roland McGrath; +Cc: binutils
There are a lot of places where we call parameters->target() and
really do want a const Target&. If we're going to make it non-const,
it really should be Target* instead of Target& (as a general rule, we
don't like to return const references).
The call to parameters->target().adjust_elf_header() in output.cc is
so clearly wrong that I don't understand how this compiles with GCC
(for ARM, that is). Just by leaving the "const" off of the override in
Target_arm, we can completely subvert the const-ness of the reference
returned by parameters->target()?
I think a better approach would be to remove the "const" from the
Target* passed to the Output_file_header ctor and stored in a private
data member, then modify the call to adjust_elf_header() to be
"this->target_->adjust_elf_header()". There's no reason to call
parameters->target() here when we already have a pointer to the target
readily available.
-cary
On Fri, Oct 11, 2013 at 11:46 AM, Roland McGrath <mcgrathr@google.com> wrote:
> The claim of constness is false for Target_arm (because it calls
> set_processor_specific_flags), so it should not be claimed for the
> general case.
>
> OK for trunk and 2.24?
>
>
> Thanks,
> Roland
>
>
> gold/
> * target.h (Target::adjust_elf_header, Target::adjust_elf_header):
> Remove const from declaration.
> * target.cc (Sized_target::do_adjust_elf_header): Update definition.
> * sparc.cc (Target_sparc::do_adjust_elf_header): Likewise.
> * parameters.h (Parameters::target): Make return value non-const.
>
> --- a/gold/parameters.h
> +++ b/gold/parameters.h
> @@ -99,7 +99,7 @@ class Parameters
> { return this->target_ != NULL; }
>
> // The target of the output file we are generating.
> - const Target&
> + Target&
> target() const
> {
> gold_assert(this->target_valid());
> --- a/gold/sparc.cc
> +++ b/gold/sparc.cc
> @@ -217,7 +217,7 @@ class Target_sparc : public Sized_target<size, big_endian>
> const elfcpp::Ehdr<size, big_endian>& ehdr);
>
> void
> - do_adjust_elf_header(unsigned char* view, int len) const;
> + do_adjust_elf_header(unsigned char* view, int len);
>
> private:
>
> @@ -4341,7 +4341,7 @@ template<int size, bool big_endian>
> void
> Target_sparc<size, big_endian>::do_adjust_elf_header(
> unsigned char* view,
> - int len) const
> + int len)
> {
> elfcpp::Ehdr_write<size, big_endian> oehdr(view);
>
> --- a/gold/target.cc
> +++ b/gold/target.cc
> @@ -1,6 +1,6 @@
> // target.cc -- target support for gold.
>
> -// Copyright 2009, 2010, 2011 Free Software Foundation, Inc.
> +// Copyright 2009, 2010, 2011, 2013 Free Software Foundation, Inc.
> // Written by Doug Kwan <dougkwan@google.com>.
>
> // This file is part of gold.
> @@ -219,7 +219,7 @@ Target::do_plt_fde_location(const Output_data*
> plt, unsigned char*,
> template<int size, bool big_endian>
> void
> Sized_target<size, big_endian>::do_adjust_elf_header(unsigned char* view,
> - int len) const
> + int len)
> {
> elfcpp::ELFOSABI osabi = this->osabi();
> if (osabi != elfcpp::ELFOSABI_NONE)
> --- a/gold/target.h
> +++ b/gold/target.h
> @@ -1,6 +1,6 @@
> // target.h -- target support for gold -*- C++ -*-
>
> -// Copyright 2006, 2007, 2008, 2009, 2010, 2011, 2012
> +// Copyright 2006, 2007, 2008, 2009, 2010, 2011, 2012, 2013
> // Free Software Foundation, Inc.
> // Written by Ian Lance Taylor <iant@google.com>.
>
> @@ -238,7 +238,7 @@ class Target
> // Adjust the output file header before it is written out. VIEW
> // points to the header in external form. LEN is the length.
> void
> - adjust_elf_header(unsigned char* view, int len) const
> + adjust_elf_header(unsigned char* view, int len)
> { return this->do_adjust_elf_header(view, len); }
>
> // Return address and size to plug into eh_frame FDEs associated with a PLT.
> @@ -548,7 +548,7 @@ class Target
> // By default, we set the EI_OSABI field if requested (in
> // Sized_target).
> virtual void
> - do_adjust_elf_header(unsigned char*, int) const = 0;
> + do_adjust_elf_header(unsigned char*, int) = 0;
>
> // Return address and size to plug into eh_frame FDEs associated with a PLT.
> virtual void
> @@ -1018,7 +1018,7 @@ class Sized_target : public Target
>
> // Set the EI_OSABI field if requested.
> virtual void
> - do_adjust_elf_header(unsigned char*, int) const;
> + do_adjust_elf_header(unsigned char*, int);
>
> // Handle target specific gc actions when adding a gc reference.
> virtual void
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] gold: remove false const from Target::do_adjust_elf_header
2013-10-11 21:26 ` Cary Coutant
@ 2013-10-11 21:45 ` Roland McGrath
2013-10-11 21:48 ` Cary Coutant
0 siblings, 1 reply; 5+ messages in thread
From: Roland McGrath @ 2013-10-11 21:45 UTC (permalink / raw)
To: Cary Coutant; +Cc: binutils
On Fri, Oct 11, 2013 at 2:26 PM, Cary Coutant <ccoutant@google.com> wrote:
> The call to parameters->target().adjust_elf_header() in output.cc is
> so clearly wrong that I don't understand how this compiles with GCC
> (for ARM, that is). Just by leaving the "const" off of the override in
> Target_arm, we can completely subvert the const-ness of the reference
> returned by parameters->target()?
Separate compilation. In output.cc the compiler only sees the abstract
class definition, where the method is const. In arm.cc the override is
non-const, but that (evidently) doesn't make it a different overload (in
the sense of having a separate vtable slot). GCC (at least 4.6.3, I
haven't tried newer ones) does not warn about an override of a virtual
function having mismatching constness, which is all that can be seen when
compiling arm.cc.
> I think a better approach would be to remove the "const" from the
> Target* passed to the Output_file_header ctor and stored in a private
> data member, then modify the call to adjust_elf_header() to be
> "this->target_->adjust_elf_header()". There's no reason to call
> parameters->target() here when we already have a pointer to the target
> readily available.
Like this?
Thanks,
Roland
gold/
* target.h (Target::adjust_elf_header, Target::do_adjust_elf_header):
Remove const from declaration.
* target.cc (Sized_target::do_adjust_elf_header): Update definition.
* sparc.cc (Target_sparc::do_adjust_elf_header): Likewise.
* output.h (Output_file_header): Remove const from member target_
and corresponding constructor argument.
* output.cc (Output_file_header::Output_file_header): Update prototype.
(Output_file_header::do_sized_write): Use this->target_ in place
of parameters()->target().
--- a/gold/output.cc
+++ b/gold/output.cc
@@ -435,7 +435,7 @@ Output_segment_headers::do_size() const
// Output_file_header methods.
-Output_file_header::Output_file_header(const Target* target,
+Output_file_header::Output_file_header(Target* target,
const Symbol_table* symtab,
const Output_segment_headers* osh)
: target_(target),
@@ -577,7 +577,7 @@ Output_file_header::do_sized_write(Output_file* of)
// Let the target adjust the ELF header, e.g., to set EI_OSABI in
// the e_ident field.
- parameters->target().adjust_elf_header(view, ehdr_size);
+ this->target_->adjust_elf_header(view, ehdr_size);
of->write_output_view(0, ehdr_size, view);
}
--- a/gold/output.h
+++ b/gold/output.h
@@ -573,7 +573,7 @@ class Output_segment_headers : public Output_data
class Output_file_header : public Output_data
{
public:
- Output_file_header(const Target*,
+ Output_file_header(Target*,
const Symbol_table*,
const Output_segment_headers*);
@@ -617,7 +617,7 @@ class Output_file_header : public Output_data
off_t
do_size() const;
- const Target* target_;
+ Target* target_;
const Symbol_table* symtab_;
const Output_segment_headers* segment_header_;
const Output_section_headers* section_header_;
--- a/gold/sparc.cc
+++ b/gold/sparc.cc
@@ -217,7 +217,7 @@ class Target_sparc : public Sized_target<size, big_endian>
const elfcpp::Ehdr<size, big_endian>& ehdr);
void
- do_adjust_elf_header(unsigned char* view, int len) const;
+ do_adjust_elf_header(unsigned char* view, int len);
private:
@@ -4339,7 +4339,7 @@ template<int size, bool big_endian>
void
Target_sparc<size, big_endian>::do_adjust_elf_header(
unsigned char* view,
- int len) const
+ int len)
{
elfcpp::Ehdr_write<size, big_endian> oehdr(view);
--- a/gold/target.cc
+++ b/gold/target.cc
@@ -1,6 +1,6 @@
// target.cc -- target support for gold.
-// Copyright 2009, 2010, 2011 Free Software Foundation, Inc.
+// Copyright 2009, 2010, 2011, 2013 Free Software Foundation, Inc.
// Written by Doug Kwan <dougkwan@google.com>.
// This file is part of gold.
@@ -219,7 +219,7 @@ Target::do_plt_fde_location(const Output_data*
plt, unsigned char*,
template<int size, bool big_endian>
void
Sized_target<size, big_endian>::do_adjust_elf_header(unsigned char* view,
- int len) const
+ int len)
{
elfcpp::ELFOSABI osabi = this->osabi();
if (osabi != elfcpp::ELFOSABI_NONE)
--- a/gold/target.h
+++ b/gold/target.h
@@ -1,6 +1,6 @@
// target.h -- target support for gold -*- C++ -*-
-// Copyright 2006, 2007, 2008, 2009, 2010, 2011, 2012
+// Copyright 2006, 2007, 2008, 2009, 2010, 2011, 2012, 2013
// Free Software Foundation, Inc.
// Written by Ian Lance Taylor <iant@google.com>.
@@ -238,7 +238,7 @@ class Target
// Adjust the output file header before it is written out. VIEW
// points to the header in external form. LEN is the length.
void
- adjust_elf_header(unsigned char* view, int len) const
+ adjust_elf_header(unsigned char* view, int len)
{ return this->do_adjust_elf_header(view, len); }
// Return address and size to plug into eh_frame FDEs associated with a PLT.
@@ -548,7 +548,7 @@ class Target
// By default, we set the EI_OSABI field if requested (in
// Sized_target).
virtual void
- do_adjust_elf_header(unsigned char*, int) const = 0;
+ do_adjust_elf_header(unsigned char*, int) = 0;
// Return address and size to plug into eh_frame FDEs associated with a PLT.
virtual void
@@ -1018,7 +1018,7 @@ class Sized_target : public Target
// Set the EI_OSABI field if requested.
virtual void
- do_adjust_elf_header(unsigned char*, int) const;
+ do_adjust_elf_header(unsigned char*, int);
// Handle target specific gc actions when adding a gc reference.
virtual void
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] gold: remove false const from Target::do_adjust_elf_header
2013-10-11 21:45 ` Roland McGrath
@ 2013-10-11 21:48 ` Cary Coutant
2013-10-11 22:03 ` Roland McGrath
0 siblings, 1 reply; 5+ messages in thread
From: Cary Coutant @ 2013-10-11 21:48 UTC (permalink / raw)
To: Roland McGrath; +Cc: binutils
> gold/
> * target.h (Target::adjust_elf_header, Target::do_adjust_elf_header):
> Remove const from declaration.
> * target.cc (Sized_target::do_adjust_elf_header): Update definition.
> * sparc.cc (Target_sparc::do_adjust_elf_header): Likewise.
> * output.h (Output_file_header): Remove const from member target_
> and corresponding constructor argument.
> * output.cc (Output_file_header::Output_file_header): Update prototype.
> (Output_file_header::do_sized_write): Use this->target_ in place
> of parameters()->target().
This is OK. Thanks!
-cary
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] gold: remove false const from Target::do_adjust_elf_header
2013-10-11 21:48 ` Cary Coutant
@ 2013-10-11 22:03 ` Roland McGrath
0 siblings, 0 replies; 5+ messages in thread
From: Roland McGrath @ 2013-10-11 22:03 UTC (permalink / raw)
To: Cary Coutant; +Cc: binutils
Committed.
Thanks,
Roland
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-10-11 22:03 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-10-11 18:46 [PATCH] gold: remove false const from Target::do_adjust_elf_header Roland McGrath
2013-10-11 21:26 ` Cary Coutant
2013-10-11 21:45 ` Roland McGrath
2013-10-11 21:48 ` Cary Coutant
2013-10-11 22:03 ` Roland McGrath
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).