Re: [PATCH 6/8] gdbserver build-id attribute generator

[Jan Kratochvil <jan.kratochvil@redhat.com>]

On Tue, 16 Apr 2013 17:14:47 +0200, Aleksandar Ristovski wrote:
> On 13-04-14 10:16 AM, Jan Kratochvil wrote:
> >On Tue, 09 Apr 2013 17:27:43 +0200, Aleksandar Ristovski wrote:
> >>+	      break;
> >>+	    }
> >>+
> >>+	  nhdr = (void *) pt_note;
> >>+	  while ((void *) nhdr < (void *) pt_end)
> >
> >When it is all const there should be (const void *).  But in fact it is easier
> >to use const gdb_byte * when pt_end is already such type:
> >	  while ((const gdb_byte *) nhdr < pt_end)
> 
> gdb_byte * is o.k., but there is no really a need for const - when
> casting is for the purpose of pointer comparison, IMO const only
> clutters the reading.

As stated also elsewhere in this review one of the review purposes is to keep
the GDB (and GNU) coding unified (when not explicitly defined by GNU Coding
Standards).

GDB was not formerly using const at all, it is moving now towards to
const-correct typing.  AFAIK const is never used for values in GDB, only for
targets.  Too many const attributes are a problem for GNU Coding Standards
compliant code as the narrow 80 columns formatting then needs multi-line
statements which are difficult to read.

But when I agreed even with const values when you are used to it please do not
break the const-correctness afterwards.  I do not see what is a purpose of all
the const attributes when in the end there are free de-const-ing casts.


> >>+	    {
> >>+	      const size_t namesz
> >>+		= ELFXX_ROUNDUP (ELFXX_FLD (*nhdr, n_namesz));
> >>+	      const size_t descsz
> >>+		= ELFXX_ROUNDUP (ELFXX_FLD (*nhdr, n_descsz));
> >>+	      const size_t note_sz = ELFXX_SIZEOF (*nhdr) + namesz + descsz;
> >>+
> >>+	      if (((gdb_byte *) nhdr + note_sz) > pt_end || note_sz == 0
> >
> >It should be (const gdb_byte *) because nhdr is already const *.
> 
> I like using const, as you may have noticed, unless it only
> clutters, like is the case when casting is used for the purpose of
> pointer comparison like here.

It really does not matter what you like, it matters what is the current GDB
coding practice.

And when a pointer is const type * I find a bug to cast it to non-const type *.
What if the caller really pointed it to a read-only segment.  non-const type *
is at least confusing in such case.


> >>+		  break;
> >>+		}
> >>+	      if (ELFXX_FLD (*nhdr, n_type) == NT_GNU_BUILD_ID
> >>+		  && ELFXX_FLD (*nhdr, n_namesz) == 4)
> >>+		{
> >>+		  const char gnu[4] = "GNU\0";
> >>+		  const char *const pname
> >>+		    = (char *) nhdr + ELFXX_SIZEOF (*nhdr);
> >
> >It should be (const char *) because nhdr is already const *.
> 
> When assigning to const, then const in the cast for pointer
> arithmetic does not matter and IMO only clutters.

const * cast to non-const * cast is a bug.

When there is even no agreement on how to use const it is better to keep the
GDB standards and not to use too many const keywords.


> >>+	}
> >>  		{
> >>  		  /* Expand to guarantee sufficient storage.  */
> >>-		  uintptr_t document_len = p - document;
> >>+		  const ptrdiff_t document_len = p - document;
> >>
> >>-		  document = xrealloc (document, 2 * allocated);
> >>  		  allocated *= 2;
> >>+		  document = xrealloc (document, allocated);
> >>  		  p = document + document_len;
> >>  		}
> >
> >This "code cleanup" change is unrelated to this patch.  But it is IMO not
> >worth checking in separately, it does not improve it anyhow IMO.
> 
> It uses correct type for pointer difference and does not repeat
> arithmetic. I removed it, it will probably reduce clash with Gary's
> patch.

You are right ptrdiff_t is more correct there but this 64-bit correctness is
unreal to be ever violated.  GDB has more serious 64-bit sizes violations...


Thanks,
Jan