3246251196 / adtools_testing

1 stars 1 forks source link

[ADTOOLS/BINUTILS] Port over the patches to 2.36.1 #7

Closed 3246251196 closed 1 year ago

3246251196 commented 1 year ago

(committed 2.36.1 to the binutils/series file and copied over the patches to the binutils/2.36.1 folder; of course, the patches do not yet cleanly apply; https://github.com/sba1/adtools/commit/ce128557f0aaf26974ad2aac08b87e80b4a5e45d)

My plan is to go through each patch that exists in 2.23.1 and apply that to 2.36.1.

Question: Should we duplicate the patches from 2.23.1 in the same style/order or should we have have one large overall patch?

At the minute, I am thinking about duplicating the patches with the necessary changes to location/file.

migthymax commented 1 year ago

I was that route down to, but did something differently. I took the binutils from sba1 with all patches, a clean checkout of the binutils version 2.23.1 and a clean version check out of (now not anymore) current version 2.39. Than did a manual "three-way" diff to get all required changes to the binutils 2.39. The result (still work in progress) can be found here https://github.com/migthymax/binutils-gdb. It does not compile. Time went by , because of other time consuming happings in life. But just yesterday i started again to investigate into binutils.

Personally i think a re-porting with eyeing how adaptions have been made in the old version will be easier. That's because i have the feeling that not all modification made by sba1 are really needed. Mainly stuff works differently today/isn't used anymore. What comes to mind here is the amiga specific relocations for an addressing mode via a special register, which as long i understood isn't used anymore. Additional things will change, or needs to be changed, mainly the handling of mechanism to support constructors and destructors. See the work of elfpipe on the thread on amigans.net. Even parts of TLS is handled/prepared by the linker stuff.

So my personal approach would be to first document how the ELF is actually used by the AmigaOS4, and there it differs from the common usage pattern from linux etc. Having such a documentation, would probably ease the porting of binutils. I have digged a lot around binutils and various documentation. If you have question I'm willing to help out. Maybe i can answer them, or helping you finding the answer.

3246251196 commented 1 year ago

@migthymax Thank you for the insight. I recall you starting it. All I care about is that it is done. If you feel that you are able to progress from your starting point then that would be great. If you do not have the time then that is fine.

I was planning on porting each patch over manually - as I think you alluded to with "re-porting with eyeing".

What I do not want to do is to have to wait for discussions and conclusions with people since that will take forever. By this I mean - discussions on forums involving lots of people. I just feel that it will never come to a conclusion.

Do you think there is value is porting over each all changes to 2.36.1? Nothing more, nothing less. Then... going from there?

Of course, this involves porting over some unnecessary changes too, but is that a bad thing?

Regards,

josefwegner commented 1 year ago

In my humble opinion, I would try to split the patches even smaller.

This would first make the reason for the patch clearer, and second will help to port the patches to a newer version because you will have fewer merge conflicts for each patch.

And multiple people can work on porting different patches at the same time.

migthymax commented 1 year ago

Doing you way, is fine too. At least least i would try to remove which has to to with the amiga hunk format. Because most patched is for supporting that. Out of my head:

0001-Changes-for-various-Amiga-targets.patch is the bigger one containing a lot of the hunk support 0002-Fixed-errors-occuring-with-more-recent-versions-of-t.patch Can be skipped, is probably fixed in upstream, as long i remember only texi files are patched. 0003-Disabled-some-stuff-such-that-68k-vtarget-builds-aga.patch Can be skipped if hunk is removed 0004-Print-symbol-name-when-an-unexpected-type-was-encoun.patch Can be applied but position has moved about 2000 lines (regarding binutils 2.39), don't know is really needed 0005-Bind-in-the-ld-unwind-options.patch Needed 0006-Introduced-strip-unneeded-rel-relocs.patch Needed 0007-Keep-symbols-for-stripped-sections.-This-is-importan.patch As the name implies 0008-Fix-undefined-behaviour.patch This one could already have been fix upstream, is not really amiga specific 0009-Fix-handling-of-brel-stuff.patch This is a fix for the amiga specific relocations introduced with patch 0001, for your way needed 0010-Fix-Wimplicit-fallthrough-warnings.patch This one could already have been present upstream, is not really amiga specific 0011-Fix-bad-function-cast-warnings.patch This one could already have been present upstream, is not really amiga 0012-Fix-pointer-comparision-bug.patch I think this one is even in upstream (as least 2.39), not amiga specific 0013-Don-t-emit-dynamic-relocations-for-data-symbols.patch That one is needed 0014-Added-newer-config.guess-and-config.sub.patch This one can be obsolete if you start with a newer binutils and patch 0001 is successful applied.

The fun part begins after applying the critical patches and the internal structures API of binutils changes and they don't compile anymore. In that you have to figure out what the changes actually want to accomplished. here we can work together, to figure it out.

3246251196 commented 1 year ago

@josefwegner I like the idea of breaking the patches down even more atomically. Are we on the same wavelength when I guess that what you mean is - for example - in patch 0001 creating a patch for each individual file changed in there?

I was thinking today that perhaps the best thing to do here is to follow such an approach (a patch per file) per day and then asking for a review.

I think it would go quicker than it first sounds!

It could also be then done by anyone. But trying to do it sequentially would be good. I believe I can create PRs on my own branch and ask for review.

josefwegner commented 1 year ago

Yes, having a patch for each file, or maybe a handful of files if the changes are similar (like adding additional configuration to the makefiles) would greatly help getting this done.

3246251196 commented 1 year ago

@migthymax Hey, before I start doing this: I have been looking at your branch and I wanted to ask: are the changes you applied what you believe to be the non-HUNK relevant things among the entire collection of patches for 2.23.2?

It is important to discuss this first since it could be that your changes are all that are needed + fixing a few things. For example, I see the error message you are hopefully also seeing:

/home/rjd/projects/adtools_binutils/binutils/repo/binutils/objcopy.c:4408:36: error: ‘bfd_target_amiga_flavour’ undeclared (first use in this function); did you mean ‘bfd_target_mmo_flavour’?
 4408 |           bfd_get_flavour(obfd) != bfd_target_amiga_flavour)
      |                                    ^~~~~~~~~~~~~~~~~~~~~~~~
      |                                    bfd_target_mmo_flavour

That enumeration is used, but it is not defined.

Anyway, the error there is not the point of this comment: I am just wondering how confident you are in continuing with what you have done before I start this.

migthymax commented 1 year ago

Ahm. Yes I'm pretty sure that i get rid of all hunk stuff. But I even tried/trying to optimizing the port, so that as little changes are needed. And the current port just uses the custom introduce bfd_target_amiga_flavour as a marker (if check) if the linking etc is targeting AmigaOS. I think this can be better handled in introducing an additional osTarget. Additional the bfd_target_amiga_flavour even introduce duplicate/unneeded code. I have the check in objcopy.c:4408 already changed to use osTarget but not committed yet. And I'm currently not at my devel machine. But if using osTarget it still needs to be checked if it is set correctly if linking for AmigaOS. Out of my head the "new" code is somethings like this. You need to get access to the elf private data with a method starting with bfd_get.... With the result you can use the elf API to get the targeting os. As soon I have access (earliest Fr in a week) i will post my solution for objcopy.c BTW reading the texi doc in bfd/docs helps to understand the bfd elf APIs

migthymax commented 1 year ago

Here is how the if statement can be written using not an amiga specific flavor, but a targeting os. ! ( bfd_get_flavour(obfd) == bfd_target_elf_flavour && get_elf_backend_data(obfd)->target_os == is_amigaos )

3246251196 commented 1 year ago

@migthymax Thanks for the update. Let's see how you progress first. If you need any help let us know. Let's continue to discuss on discord. We can evaluate how we get on with your branch first, before attempting something else.

3246251196 commented 1 year ago

Binutils is being handled over on: https://github.com/migthymax/binutils-gdb