[1/7] Fix size of external_reloc for pe-aarch64

  Resubmission of aarch64-w64-mingw32 patches against Nick's arm64pe patch.

---
 bfd/coff-aarch64.c     | 4 ----
 include/coff/aarch64.h | 3 +--
 2 files changed, 1 insertion(+), 6 deletions(-)
  

Hi!

I am not a maintainer, but would you mind adding proper commit messages 
describing what each patch does (or intends to)?

Thanks,

Christophe


On 1/6/23 02:25, Mark Harmstone wrote:
> Resubmission of aarch64-w64-mingw32 patches against Nick's arm64pe patch.
> 
> ---
>   bfd/coff-aarch64.c     | 4 ----
>   include/coff/aarch64.h | 3 +--
>   2 files changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/bfd/coff-aarch64.c b/bfd/coff-aarch64.c
> index 8a514b278ee..236cbb79ffb 100644
> --- a/bfd/coff-aarch64.c
> +++ b/bfd/coff-aarch64.c
> @@ -188,10 +188,6 @@ coff_aarch64_rtype_lookup (unsigned int code)
>   #define bfd_pe_print_pdata      NULL
>   #endif
>   
> -/* Handle include/coff/aarch64.h external_reloc.  */
> -#define SWAP_IN_RELOC_OFFSET	H_GET_32
> -#define SWAP_OUT_RELOC_OFFSET	H_PUT_32
> -
>   /* Return TRUE if this relocation should
>      appear in the output .reloc section.  */
>   
> diff --git a/include/coff/aarch64.h b/include/coff/aarch64.h
> index 7592661553f..4616cfef2b8 100644
> --- a/include/coff/aarch64.h
> +++ b/include/coff/aarch64.h
> @@ -54,11 +54,10 @@ struct external_reloc
>     char r_vaddr[4];
>     char r_symndx[4];
>     char r_type[2];
> -  char r_offset[4];
>   };
>   
>   #define RELOC struct external_reloc
> -#define RELSZ 14
> +#define RELSZ 10
>   
>   /* ARM64 relocations types. */
>
  

On 6/1/23 09:47, Christophe Lyon wrote:
> Hi!
>
> I am not a maintainer, but would you mind adding proper commit messages describing what each patch does (or intends to)?
>
> Thanks,
>
> Christophe

Hi Christophe,

This is a resubmission of a patch set from a few days ago, because of a change that Tamar wanted. If you're interested in the discussion behind each patch, it's available in the mailing list archives.

Mark
  

On 06.01.2023 18:51, Mark Harmstone wrote:
> On 6/1/23 09:47, Christophe Lyon wrote:
>> Hi!
>>
>> I am not a maintainer, but would you mind adding proper commit messages describing what each patch does (or intends to)?
>>
>> Thanks,
>>
>> Christophe
> 
> Hi Christophe,
> 
> This is a resubmission of a patch set from a few days ago, because of a change that Tamar wanted. If you're interested in the discussion behind each patch, it's available in the mailing list archives.

I'm afraid pointing to list archives for explanations on patches isn't a good
approach. Once committed, such links will not be easy to (re-)establish.
Other projects are quite a bit more demanding towards the content of commit
messages, but I guess some minimal level of explanation wants to be in the
average binutils patch as well.

Jan
  

On 1/9/23 09:11, Jan Beulich wrote:
> On 06.01.2023 18:51, Mark Harmstone wrote:
>> On 6/1/23 09:47, Christophe Lyon wrote:
>>> Hi!
>>>
>>> I am not a maintainer, but would you mind adding proper commit messages describing what each patch does (or intends to)?
>>>
>>> Thanks,
>>>
>>> Christophe
>>
>> Hi Christophe,
>>
>> This is a resubmission of a patch set from a few days ago, because of a change that Tamar wanted. If you're interested in the discussion behind each patch, it's available in the mailing list archives.
> 
> I'm afraid pointing to list archives for explanations on patches isn't a good
> approach. Once committed, such links will not be easy to (re-)establish.
> Other projects are quite a bit more demanding towards the content of commit
> messages, but I guess some minimal level of explanation wants to be in the
> average binutils patch as well.
> 

That's what I meant I think, thanks for rephrasing :-)

I can't find guidelines on how to contribute patches for binutils, but 
they are similar to GCC's and GDB's. If you just run 'git log' in a 
binutils clone, you'll see what we mean: in addition to short a summary 
(title), commit messages include a description of what the commit does 
and why this is the right change.

It's great if all your changes are obvious for Nick, but they are not 
for others like me ;-)

For instance this patch 1/7 only says "Fix size of external_reloc for 
pe-aarch64", so why is the removal of 
SWAP_IN_RELOC_OFFSET/SWAP_OUT_RELOC_OFFSET and r_offset needed?

I did check the list archives, so if I'm not mistaken this is third 
iteration of this patch series? (I saw 1/5 and 1/8 in December, then 
this one in January). The first iteration had an introduction message 
which led to a discussion with Jan and Tamar, and I think an updated 
version of that message would help here, when others will have to try to 
understand these patches in whatever future time ;-)

Also as Jan mentioned on your testsuite patches, can you describe why we 
have to skip so many of them? Jan seems to think that they could be 
adjusted to cope with both formats intead.

Thanks,

Christophe

> Jan
  

On 9/1/23 09:22, Christophe Lyon wrote:
>
>
> On 1/9/23 09:11, Jan Beulich wrote:
>> On 06.01.2023 18:51, Mark Harmstone wrote:
>>> On 6/1/23 09:47, Christophe Lyon wrote:
>>>> Hi!
>>>>
>>>> I am not a maintainer, but would you mind adding proper commit messages describing what each patch does (or intends to)?
>>>>
>>>> Thanks,
>>>>
>>>> Christophe
>>>
>>> Hi Christophe,
>>>
>>> This is a resubmission of a patch set from a few days ago, because of a change that Tamar wanted. If you're interested in the discussion behind each patch, it's available in the mailing list archives.
>>
>> I'm afraid pointing to list archives for explanations on patches isn't a good
>> approach. Once committed, such links will not be easy to (re-)establish.
>> Other projects are quite a bit more demanding towards the content of commit
>> messages, but I guess some minimal level of explanation wants to be in the
>> average binutils patch as well.
>>
>
> That's what I meant I think, thanks for rephrasing :-)
>
> I can't find guidelines on how to contribute patches for binutils, but they are similar to GCC's and GDB's. If you just run 'git log' in a binutils clone, you'll see what we mean: in addition to short a summary (title), commit messages include a description of what the commit does and why this is the right change.
>
> It's great if all your changes are obvious for Nick, but they are not for others like me ;-)
>
> For instance this patch 1/7 only says "Fix size of external_reloc for pe-aarch64", so why is the removal of SWAP_IN_RELOC_OFFSET/SWAP_OUT_RELOC_OFFSET and r_offset needed?
>
> I did check the list archives, so if I'm not mistaken this is third iteration of this patch series? (I saw 1/5 and 1/8 in December, then this one in January). The first iteration had an introduction message which led to a discussion with Jan and Tamar, and I think an updated version of that message would help here, when others will have to try to understand these patches in whatever future time ;-)
>
> Also as Jan mentioned on your testsuite patches, can you describe why we have to skip so many of them? Jan seems to think that they could be adjusted to cope with both formats intead.
>
> Thanks,
>
> Christophe
>
>> Jan

You both raise good points. Sorry, I still find mailing list culture very confusing, compared to something like GitHub... it's not at all clear what you're expected to do, who has what authority, etc. I'm not at all certain that I was even expected to resubmit the patches the third time, given it was more or less a trivial rebase.

Jan, I suggest that I push patches 1 and 4-7 with the original messages, and save the two test patches for a later date. Would that be acceptable? Patches 2 and 3 probably should have been submitted separately anyway, as the later patches don't depend on them.

Mark
  

On 09.01.2023 19:03, Mark Harmstone wrote:
> You both raise good points. Sorry, I still find mailing list culture very confusing, compared to something like GitHub... it's not at all clear what you're expected to do, who has what authority, etc. I'm not at all certain that I was even expected to resubmit the patches the third time, given it was more or less a trivial rebase.
> 
> Jan, I suggest that I push patches 1 and 4-7 with the original messages, and save the two test patches for a later date. Would that be acceptable?

I think so, yes. Looking at just patch 1, its original description was first
talking about the series as a whole. That part would better be omitted. Each
patch's description should speak just for itself.

Jan
  

On 10/1/23 08:58, Jan Beulich wrote:
> On 09.01.2023 19:03, Mark Harmstone wrote:
>> You both raise good points. Sorry, I still find mailing list culture very confusing, compared to something like GitHub... it's not at all clear what you're expected to do, who has what authority, etc. I'm not at all certain that I was even expected to resubmit the patches the third time, given it was more or less a trivial rebase.
>>
>> Jan, I suggest that I push patches 1 and 4-7 with the original messages, and save the two test patches for a later date. Would that be acceptable?
> I think so, yes. Looking at just patch 1, its original description was first
> talking about the series as a whole. That part would better be omitted. Each
> patch's description should speak just for itself.
>
> Jan

Thanks Jan, it's in now.

Mark