* [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).