[1/7] Fix size of external_reloc for pe-aarch64
Checks
Commit Message
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(-)
Comments
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
@@ -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. */
@@ -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. */