This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 2/3] Fix copy_bitwise()
On Mon, Nov 14 2016, Luis Machado wrote:
> On 11/14/2016 09:02 AM, Andreas Arnez wrote:
[...]
>> + dest += dest_offset / 8;
>> + dest_offset %= 8;
>> + source += source_offset / 8;
>> + source_offset %= 8;
>
> Are you sure you will always have non-zero source_offset and dest_offset
> when explicitly dividing them by 8? If i were to feed (or GDB, in some
> erroneous state) invalid data to the function, this would likely crash?
>
> There are other cases of explicit / operations.
No, copy_bitwise should work fine with source_offset and dest_offset set
to zero. Where do you think it would crash?
[...]
>> + /* Fill BUF with DEST_OFFSET bits from the destination and 8 -
>> + SOURCE_OFFSET bits from the source. */
>> + buf = *(bits_big_endian ? source-- : source++) >> source_offset;
>
> Maybe it's just me, but having constructs like the above don't help much
> performance-wise and make the code slightly less readable. Should we
> expand this further? There are multiple occurrences of this.
Well, I've tried a few different ways and found this approach actually
the easiest to read, for my taste. For instance, it makes the multiple
occurrences easy to recognize -- as you pointed out ;-)
Of course, if people feel that this post-decrement/increment pattern
really hurts readability, I can provide a more "stretched" form instead.
> Also, should we harden the method to prevent dereferencing NULL source or
> dest?
I wouldn't consider that necessary, but I'm open for other opinions.
[...]
> Otherwise it looks good to me.
Thanks!
--
Andreas