public inbox for elfutils@sourceware.org
 help / color / mirror / Atom feed
From: Roland McGrath <roland@hack.frob.com>
To: elfutils-devel@lists.fedorahosted.org
Subject: Re: [PATCH] Support 1-sized reads in read_ubyte_unaligned_inc and read_sbyte_unaligned_inc
Date: Thu, 11 Sep 2014 15:16:19 -0700	[thread overview]
Message-ID: <20140911221619.A9AC62C3972@topped-with-meat.com> (raw)
In-Reply-To: m21trj3upp.fsf@redhat.com

[-- Attachment #1: Type: text/plain, Size: 2129 bytes --]

> Five, four of them with constant size and one with a 64bit?8:4 sort of
> expression.  The reads are done through a macro that checks bounds,
> There's one macro for all the widths, mostly because I didn't like to
> have four macros with unknown cut'n'paste errors.  I expect that the
> compiler will be able to see through and just inline a check and an
> access for the right width directly, but I didn't actually check.

The existing uses are in practice 64bit?8:4 cases.  They just happen
to allow a nonsensical 2 case rather than diagnosing it (i.e. a
crazy FDE encoding) early.  readelf.c's encoded_ptr_size has exactly
this one caller, so it could just be rolled in there and have a
diagnostic and early bail for anything that's not a 4- or 8-wide
encoding.  Then we could have a memory-access.h macro that is
specifically only for "ptr_size" (4 or 8), and using the single-size
named macros for the constant-size cases.

For your macro used for constant-size cases, you could make it a
single macro that takes the size literal as a macro argument and
uses read_##size##ubyte_unaligned_inc.

> Admittedly this is all somewhat moot.  I don't check bounds with LEB's
> anyway, and most of libdw just checks post fact that the pointers are
> still in bounds.  Maybe I should simply do the same.

We should be consistent throughout the codebase, one way or another.
If the bounds-checking matters for one case, it matters for the
others; if we don't care for umpteen cases, we shouldn't care any
more for one more case.  I've never been happy with the lossy bounds
checking, and tend to think we should fix it up all throughout.  But
that is quite unrelated to your current work.  It probably makes
sense to finish the new thing you're doing now just by following the
existing models, i.e. doing no better on bounds-checking but
matching the idioms we see elsewhere in the code.  Then later (right
away if you care to take it on, in the fullness of time if not) we
can figure out how to revamp the whole memory-access.h interface and
all its users to be fully robust.


Thanks,
Roland

             reply	other threads:[~2014-09-11 22:16 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-09-11 22:16 Roland McGrath [this message]
  -- strict thread matches above, loose matches on Subject: below --
2014-09-12 12:24 Petr Machata
2014-09-10 23:46 Petr Machata
2014-09-10 21:22 Roland McGrath
2014-09-10 21:09 Petr Machata

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20140911221619.A9AC62C3972@topped-with-meat.com \
    --to=roland@hack.frob.com \
    --cc=elfutils-devel@lists.fedorahosted.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).