Re: Flash memory size not aligned to address

  On Sat, Oct 7, 2017 at 3:42 PM, Kevin Buettner <kevinb@redhat.com> wrote:
>
> Hi Mark,
>
> I did notice, however, that you added [PATCH] to the subject line,
> so perhaps you were just getting the subject line formatted in
> such a way so that it might be noticed?


Indeed, that was my plan.

>
> Do you have an FSF Copyright Assignment?
>

No.

> ChangeLog entries are generally not posted as patches.  It is unlikely
> that this portion of the patch will cleanly apply once it's approved.

Ok.  Thank you for pointing me to the wiki.  I was going off of
gdb/CONTRIBUTE.  Which one is authoritative?

> It occurs to me that address_in_region might be better named
> offset_in_region.

Updated patch follows.

> It seems to me that there will be no difference in behavior for
> targets whose region boundaries are already aligned to the block size.
> I do wonder, however, about behavior on other targets that don't meet
> this criteria.  I'm hoping that someone else who has more experience in
> this area will comment.

The only situation I can think of would be a device that begins a
section of flash unaligned, but requires an aligned address for the
erase command.  I'm not aware of such, but it's a big world.

Anyway, thanks for looking it over. Here goes second try:

gdb/ChangeLog:

        * target-memory.c (block_boundaries): Fix for block address not
        aligned on block size.
  

On Mon, 9 Oct 2017 10:28:13 -0600
Mark Rages <markrages@gmail.com> wrote:

> > Do you have an FSF Copyright Assignment?
> >  
> 
> No.

According to this link...

https://www.gnu.org/prep/maintain/maintain.html#Legally-Significant

...contributions that consist of more than about 15 lines of code or
text are significant for copyright purposes.

Your patch is well under that amount, so I do not think that you
*need* a copyright assignment for this particular change.

However, if you plan to make other contributions to GDB, I encourage
you to have a copyright assignment on file with the FSF.

Please let us know if you intend to pursue a copyright assignment.
If not, I can commit/push this change for you.

> > ChangeLog entries are generally not posted as patches.  It is unlikely
> > that this portion of the patch will cleanly apply once it's approved.  
> 
> Ok.  Thank you for pointing me to the wiki.  I was going off of
> gdb/CONTRIBUTE.  Which one is authoritative?

I think that gdb/CONTRIBUTE is meant to be authoritative.

But I find that the wiki document provides some really useful nuts
and bolts type of stuff for contributing to GDB.  Personally, I find
it to be a lot more useful than gdb/CONTRIBUTE.

For example, the wiki contains some really useful info on how to
format your commit comment when using git.  It also contains some
good information on formatting ChangeLog entries.

> > It occurs to me that address_in_region might be better named
> > offset_in_region.  
> 
> Updated patch follows.
> 
> > It seems to me that there will be no difference in behavior for
> > targets whose region boundaries are already aligned to the block size.
> > I do wonder, however, about behavior on other targets that don't meet
> > this criteria.  I'm hoping that someone else who has more experience in
> > this area will comment.  
> 
> The only situation I can think of would be a device that begins a
> section of flash unaligned, but requires an aligned address for the
> erase command.  I'm not aware of such, but it's a big world.
> 
> Anyway, thanks for looking it over. Here goes second try:
> 
> gdb/ChangeLog:
> 
>         * target-memory.c (block_boundaries): Fix for block address not
>         aligned on block size.
> 
> diff --git a/gdb/target-memory.c b/gdb/target-memory.c
> index 1c8faa8..7f048de 100644
> --- a/gdb/target-memory.c
> +++ b/gdb/target-memory.c
> @@ -138,14 +138,18 @@ block_boundaries (CORE_ADDR address, CORE_ADDR
> *begin, CORE_ADDR *end)
>  {
>    struct mem_region *region;
>    unsigned blocksize;
> +  CORE_ADDR offset_in_region;
> 
>    region = lookup_mem_region (address);
>    gdb_assert (region->attrib.mode == MEM_FLASH);
>    blocksize = region->attrib.blocksize;
> +
> +  offset_in_region = address - region->lo;
> +
>    if (begin)
> -    *begin = address / blocksize * blocksize;
> +    *begin = region->lo + offset_in_region / blocksize * blocksize;
>    if (end)
> -    *end = (address + blocksize - 1) / blocksize * blocksize;
> +    *end = region->lo + (offset_in_region + blocksize - 1) /
> blocksize * blocksize;
>  }
> 

This is okay.

As noted earlier, please let us know if you intend to pursue an FSF
copyright assignment.  If so, in order have write access to the git
repository, you'll need a sourceware account.  You should also submit
a patch adding yourself to the "Write After Approval" section of
gdb/MAINTAINERS.  (We can go into how all of that is accomplished
after the copyright assignment process is finished.)

Kevin
  

On Mon, Oct 9, 2017 at 11:51 AM, Kevin Buettner <kevinb@redhat.com> wrote:
>
> Please let us know if you intend to pursue a copyright assignment.
> If not, I can commit/push this change for you.

I am not pursuing copyright assignment right now.   Please push this
patch for me.

I am not opposed to assignment, but I will wait until I have a more
substantial contribution to make.

Regards,
Mark
markrages@gmail
  

On Mon, 9 Oct 2017 13:08:44 -0600
Mark Rages <markrages@gmail.com> wrote:

> On Mon, Oct 9, 2017 at 11:51 AM, Kevin Buettner <kevinb@redhat.com> wrote:
> >
> > Please let us know if you intend to pursue a copyright assignment.
> > If not, I can commit/push this change for you.  
> 
> I am not pursuing copyright assignment right now.   Please push this
> patch for me.

I've pushed it for you.  The ChangeLog entry is attributed to you.  The
git commit has my name on it, but I indicate where the patch came
from in the commit comment.

(I hope that's the correct procedure...)

Kevin
  

On 10/11/2017 08:56 AM, Kevin Buettner wrote:
> On Mon, 9 Oct 2017 13:08:44 -0600
> Mark Rages <markrages@gmail.com> wrote:
> 
>> On Mon, Oct 9, 2017 at 11:51 AM, Kevin Buettner <kevinb@redhat.com> wrote:
>>>
>>> Please let us know if you intend to pursue a copyright assignment.
>>> If not, I can commit/push this change for you.  
>>
>> I am not pursuing copyright assignment right now.   Please push this
>> patch for me.
> 
> I've pushed it for you.  The ChangeLog entry is attributed to you.  The
> git commit has my name on it, but I indicate where the patch came
> from in the commit comment.
> 
> (I hope that's the correct procedure...)

Actually, the right procedure is to amend to commit's authorship too.
There are useful instructions here:

 https://sourceware.org/glibc/wiki/GlibcGit#Author_Attribution

Thanks,
Pedro Alves
  

On Wed, 11 Oct 2017 10:31:08 +0100
Pedro Alves <palves@redhat.com> wrote:

> On 10/11/2017 08:56 AM, Kevin Buettner wrote:
> > On Mon, 9 Oct 2017 13:08:44 -0600
> > Mark Rages <markrages@gmail.com> wrote:
> >   
> >> On Mon, Oct 9, 2017 at 11:51 AM, Kevin Buettner <kevinb@redhat.com> wrote:  
> >>>
> >>> Please let us know if you intend to pursue a copyright assignment.
> >>> If not, I can commit/push this change for you.    
> >>
> >> I am not pursuing copyright assignment right now.   Please push this
> >> patch for me.  
> > 
> > I've pushed it for you.  The ChangeLog entry is attributed to you.  The
> > git commit has my name on it, but I indicate where the patch came
> > from in the commit comment.
> > 
> > (I hope that's the correct procedure...)  
> 
> Actually, the right procedure is to amend to commit's authorship too.
> There are useful instructions here:
> 
>  https://sourceware.org/glibc/wiki/GlibcGit#Author_Attribution

Okay, thanks.

I gather it too late to amend it now, right?  (Since git commit --amend
only works on the most recent commit.)

Kevin
  

On 10/11/2017 04:53 PM, Kevin Buettner wrote:

> I gather it too late to amend it now, right?  (Since git commit --amend
> only works on the most recent commit.)

Right, it's too late, but the reason is we don't allow history
rewrites of the upstream repo; we only allow fast-forward merges
to keep history linear -- even if it were the most recent commit,
you wouldn't be able to push it.  (And it's possible to amend
more than one commit.)  The closest would be to revert the patch, and
reapply the original patch with author fixed.  (Not sure it's
worth complicating git history a bit, but it's easy enough to
do if desired.)

Thanks,
Pedro Alves
  

On Wed, 11 Oct 2017 17:11:17 +0100
Pedro Alves <palves@redhat.com> wrote:

> On 10/11/2017 04:53 PM, Kevin Buettner wrote:
> 
> > I gather it too late to amend it now, right?  (Since git commit --amend
> > only works on the most recent commit.)  
> 
> Right, it's too late, but the reason is we don't allow history
> rewrites of the upstream repo; we only allow fast-forward merges
> to keep history linear -- even if it were the most recent commit,
> you wouldn't be able to push it.  (And it's possible to amend
> more than one commit.)  The closest would be to revert the patch, and
> reapply the original patch with author fixed.  (Not sure it's
> worth complicating git history a bit, but it's easy enough to
> do if desired.)

Thanks for the explanation.

Unless someone strenuously objects, I'll leave it as it is.  As noted
earlier, Mark is credited in both the ChangeLog as well as in the
commit comment.

Thanks again,

Kevin