This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] gold: fix some signed-unsigned comparison warnings


>> Seems to me that offset_in_output_section is misdeclared here. As
>> passed from relocate_relocs(), the actual parameter is declared as an
>> Elf_Addr, which is unsigned. I think offset_in_output_section should
>> be Arm_address here.
>
> It seems to me the really proper type here is Elf_Off (albeit that is
> actually identical to Elf_Addr), since what this refers to an offset in an
> ELF file.  In fact, I think most of the uses of off_t throughout gold
> really ought to be Elf_Off, but I'm not trying to clean everything up
> right now.

Hmm, you have a point, although I'm not sure I completely agree.
Elf_Off is unsigned because it's always used in the ELF spec as an
absolute position within a file -- i.e., Elf_Off is to the file what
Elf_Addr is to the virtual address space. We should be using off_t
internally for file offsets, and Elf_Off only when looking at a
structure in the ELF file. For offsets relative to some section, which
is really what this is, we should be using section_offset_type. And in
that case, we can compare it directly with -1 rather than
invalid_address. It looks to me like the code in target.h is using
Elf_Addr simply because it was convenient to use in the templatized
code.

I found exactly three uses of Elf_Off in gold: 1 in layout.cc, and 2
in powerpc.cc. The one in layout.cc looks good, but powerpc.cc is
using it as a relative offset in one place, and as an Elf_Addr in
another.

(I find the need to draw a distinction between offset and address, or
between offset and size, to be tedious, though. The line gets blurred
in places, and I'd kind of prefer to use one type for all three.)

> diff --git a/gold/dwarf_reader.cc b/gold/dwarf_reader.cc
> index c80e8cb..ec0e845 100644
> --- a/gold/dwarf_reader.cc
> +++ b/gold/dwarf_reader.cc
> @@ -57,7 +57,7 @@ Sized_elf_reloc_mapper<size, big_endian>::symbol_section(
>      unsigned int symndx, Address* value, bool* is_ordinary)
>  {
>    const int symsize = elfcpp::Elf_sizes<size>::sym_size;
> -  gold_assert((symndx + 1) * symsize <= this->symtab_size_);
> +  gold_assert(static_cast<off_t>((symndx + 1) * symsize) <=
> this->symtab_size_);
>    elfcpp::Sym<size, big_endian> elfsym(this->symtab_ + symndx * symsize);
>    *value = elfsym.get_st_value();
>    return this->object_->adjust_sym_shndx(symndx, elfsym.get_st_shndx(),

You didn't like my suggestion to declare symsize as an off_t to begin with?

-cary


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]