Created attachment 228325 [details] devel/dyncall: unbreak on arm Greetings! devel/dyncall has two minor issues that prevent it from building on armv6/armv7: - the pre-UAL mnemonics FLDMIAD and STMIAD are used - % sigils are used for registers in one file This patch makes the port build by fixing these two problems. While we are at it, I've hooked up TEST_TARGET and verified that the test suite passes. I was however unable to test on FreeBSD 12 or armv6. Tested with Poudriere on armv7 FreeBSD 13. Please MFH to 2021Q4 if possible.
Thanks! Since I'm also upstream, I think I should integrate parts of this directly into dyncall, and not just patch up here. I'm surprised to see the % issue, I build it on clang and don't have this issue... but maybe I'm looking at the wrong place. I think it'll break builds with GNU as, though? Did you test that? I'm not sure about the use of hooking up the test suite *in its current state*, as some test executables do not return any meaningful status codes. This is also on my list to fix. Of course this should be hooked up eventually, so you did the right thing, I just think the suite needs some changes, first. Just in case: I'll need a couple of days before I can look at this in detail, life is stressful at the moment. Thanks, I'll report back asap
(In reply to Tassilo Philipp from comment #1) Hi Tassilo, ARM does not normally use % sigils for registers and I think gas just accepts them out of courtesy. Clang's assembler on the other hand is a lot stricter. I none of your other arm assembly source files in your projects have them, so I wonder why this one file diverges. Also consider migrating to UAL (.syntax unified) if possible. This'll avoid a lot of headache in the future. It is possible that your clang is configured to use the GNU assembler for assembly files. As for the patches, I can confirm that the resulting code also assembles when using gcc/GNU as. > I'm not sure about the use of hooking up the test suite *in its current state*, as some test executables do not return any meaningful status codes. This is also on my list to fix. Of course this should be hooked up eventually, so you did the right thing, I just think the suite needs some changes, first. The test suite is not ran automatically by any part of the package building process. Setting TEST_TARGET simply means that it is possible to run "make test" on the port. However, if you don't feel comfortable with that, there's no problem leaving it out.
(In reply to Robert Clausecker from comment #2) > ... so I wonder why this one file diverges. Good question... no idea. Maybe there was a reason, maybe there wasn't, this all started a long time ago in 2008... Of course this should work everywhere, so will make it work everywhere. :) > Also consider migrating to UAL (.syntax unified) if possible. Absolutely. I'll try and if that works on the platforms we plan to still support, yes, of course. > As for the patches, I can confirm that the resulting code also assembles when using gcc/GNU as. Thanks! > The test suite is not ran automatically by any part of the package building process. Setting TEST_TARGET simply means that it is possible to run "make test" on the port. However, if you don't feel comfortable with that, there's no problem leaving it out. I want to have that in, eventually, for sure, so I'll play around with it and see if it makes sense at this point. Thanks again, I'll get back to you asap. I'll integrate the changes into dyncall directly, for an upcoming 1.3 release. That release might still be a little ways out, though, so I consider getting your patches into the ports tree, as-is, for v1.2. Thanks!
(In reply to Tassilo Philipp from comment #3) > Thanks again, I'll get back to you asap. I'll integrate the changes into > dyncall directly, for an upcoming 1.3 release. That release might still be a > little ways out, though, so I consider getting your patches into the ports > tree, as-is, for v1.2. If you want to have that, the easiest way is to simply give maintainer approval for the patch by setting the maintainer-approval flag on the patch. This permits ports committers to commit the patch. Also make sure to set the maintainer-feedback flag to "+" indicating that you gave feedback. I meanwhile had the chance to test the patch on armv6 FreeBSD 13 where it builds fine with Poudriere.
(In reply to Robert Clausecker from comment #4) I know, but as a maintainer I don't feel comfortable simply looking at a patch, and saying that it's fine. It's a bit my responsibility to test it and make sure this works. Also I happen to run a fbsd12.2 which is still supported. As said, give me a couple of days, I'll take care of this as soon as possible. I would do it now, but there are too many spinning plates at the moment. Thank you for your patience.
Alright, I looked at it and ran some tests on 4 different arm platforms, and I cannot accept it as-is, but it's close. The main issue is it only unbreaks arm w/ the clang integrated assembler when not using THUMB (in other words, the file you pointed out is not the only one to patch). I'll come up with a new patch. Details and/or other things while looking at it: - the %-prefixes were probably used b/c of x86 habits, they should be removed indeed, thanks! - this is not the first time we have issues with clang's integrated assembler, we needed to change some PPC code also for v1.2 to make it happy - about the UAL/.syntax unified: this doesn't actually help for dyncall, b/c the ARM and THUMB calling conventions differ enough to need their own implementations, anyways.... yes, it would probably also be harmless to use those instruction names, but pre-UAL toolchains might break (and we do actually use dyncall on some of those); I'm surprised that you listed it under "issues that prevent it from building on armv6/armv7", b/c I cannot reproduce that... - depending on further tests, until v1.3 is out with the real fix, I might make use of clang's -no-integrated-as to unbreak the build for the FreeBSD port... I'll have a clearer picture, soon - I don't think I'll add TEST_TARGET, for already pointed out reasons; I understand that this isn't run automatically for the pkg building process, but there is no point in running `make test` to only build (but not run) the tests - the target you probably wanted isn't 'tests' but 'run-tests', and for those I should fix some exit codes, first; as said, this is on my to do list for dyncall 1.3 More to come, soon, thanks again for bringing this to my attention :)
(In reply to Tassilo Philipp from comment #6) Thank you for your investigations. Feel free to fix this how you like. > I'm surprised that you listed it under "issues that prevent it from building on armv6/armv7", b/c I cannot reproduce that... I tested on armv7 FreeBSD 13.0 and got a build failure. Try it! There are also dozens of port fallouts for your ports indicating build failures (see freshports; click on the trifoil icon on your port's page to get to the fallouts). > depending on further tests, until v1.3 is out with the real fix, I might make use of clang's -no-integrated-as to unbreak the build for the FreeBSD port... I'll have a clearer picture, soon You can do that, but then you need to add a binutils dependency, e.g. BUILD_DEPENDS= as:devel/binutils@native FreeBSD no longer ships binutils in base on most platforms.
(In reply to Robert Clausecker from comment #7) Thanks for the additional info and pointing out the removal of binutils, which isn't the case on 12.2, yet. Will see what I can put together.
Alright, I have a 12.2 armv7 w/ optional binutils and 13.0 with only clang's as now, and I can repro it all. I'm a bit disappointed that clang's integrated AS disallows the use of pre-UAL mnemonics, b/c given dyncall is used and supported in some cases on very old setups, this means that there will be more toolchain checks and more #ifs to keep supporting them (for the upstream fix). But yeah, that's life. I'll have something ready by tomorrow, if nothing unexpected comes up. For the FreeBSD port it'll be pragmatic and likely be very close to your patch (basically the same but with the mentioned thumb support). For the actual upstream fix this will be a bit more involved b/c of those old platforms I mentioned, and will be part of v1.3. Thanks again for bringing this to my intention
Created attachment 228402 [details] syntax fixes to unbreak arm32 when using clang's integrated assembler New patch: - handling also thumb builds - everything done via post-patch target, b/c it's simple replacements, also making future port update to upcoming 1.3 a bit simpler - not integrating tests for now, as mentioned Tested with clang's integrated assembler on 12.2 and 13.0, as well as with binutils/gas on 12.2. Thanks again!
(In reply to Tassilo Philipp from comment #10) Can you use patch files instead, see porters handbook section 4.4.3. Simple Automatic Replacements https://docs.freebsd.org/en/books/porters-handbook/book/
As mentioned, this was intentional: I'm also upstream, the same patches have been applied upstream, already, so this simply makes my life easier until v1.3 is out, as the entire post-patch: target will be removed. And as a matter of fact, this is simple replacements.
(In reply to Tassilo Philipp from comment #12) Yes, I agree. However, the Porter's Handbook explicitly states that ${REINPLACE_CMD} must not be used for static replacements: > Only use sed(1) to replace variable content. You must use patch files instead of sed(1) to replace static content. So a patch file must be used.
(In reply to Robert Clausecker from comment #13) Your original patch did the same... so pardon me but I have a hard time seeing the benefit replacing 5 lines that will be removed in a couple of months, and instead using over a hundred lines in 4 patches that replace 5 characters of pure syntax changes. There is zero benefit of patches here, no context needed, etc.. But sure, if you insist... I won't get around this, today, though.
(In reply to Tassilo Philipp from comment #14) I don't insist, but it's policy to do it that way. While I do not know why this policy has been made, I am unfortunately not in a position to negotiate it. That's why my patches did it this way, too.
(In reply to Robert Clausecker from comment #15) Sorry if my message came across as I did attack you, which was not my intention. And the "if you insist" was more broadly aimed at the policy. Getting back to this, I unfortunately have a lot of stress at the moment after a loss in the family, daily grind, etc. and thus little time at hand. Also, by grepping through the entire ports tree, I find thousands of examples that do the same simple replacements as suggested here. I fully understand that there is such policy, and that many of those examples might be historic. I'm not even against such a policy, at all, I just ask for reason. My plan was to simply make my life easier, given that there won't be a freebsd specific patchset to maintain for some 3rd party software, as I am the maintainer and upstream. That said, I explained my case and I would appreciate if this will be accepted as-is, as it'll make my life easier now, and in a few months when I'll release 1.3 and update the port, which will remove those exact lines again. Please, @Mikael, let me know if you want me to convert this to patches after all. Thank you
(In reply to Robert Clausecker from comment #15) The community and contributors in particular are always in a position, and welcome, and encouraged to bring up questions and issues with respect to improving policy. I would encourage a post on freebsd-ports seeking clarification (and subsequent documentation) of any policy guidelines Policy for policy sake is not in the spirit of how we do things, and I see no issue with the patch as is. Sorry for your loss Tassilo
Created attachment 228477 [details] v0 (In reply to Tassilo Philipp from comment #16) This is a relatively new policy and most "old" ports still use REINPLACE_CMD. Sometimes REINPLACE_CMD stays forever in a port Makefile and does nothing and can introduce subtle bugs. So it's better to fail in the patch phase. I can push the attached patch if you agree.
Comment on attachment 228477 [details] v0 Approved, thanks to both of you for helping out here, and doing this work, I appreciate it! I really do have too many things to care of, I didn't even get around to check my inbox, yesterday, at all. Thanks again, patch replacement approved
(and btw... I can't get the "maintainer-approval +" to stick on your patch, I can set it, submit, but it won't stick, tried multiple times now)
(In reply to Kubilay Kocak from comment #17) Thanks Kubilay!
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=17631aa22f13fa4cecfed6cf92bd3ac15334454b commit 17631aa22f13fa4cecfed6cf92bd3ac15334454b Author: Tassilo Philipp <tphilipp@potion-studios.com> AuthorDate: 2021-10-08 10:46:15 +0000 Commit: Mikael Urankar <mikael@FreeBSD.org> CommitDate: 2021-10-08 10:46:15 +0000 devel/dyncall: Fix build on arm PR: 258846 Reported by: Robert Clausecker devel/dyncall/files/patch-arm (new) | 176 ++++++++++++++++++++++++++++++++++++ 1 file changed, 176 insertions(+)
A commit in branch 2021Q4 references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=e0d3ad189b0a9beac091154261296745f765a731 commit e0d3ad189b0a9beac091154261296745f765a731 Author: Tassilo Philipp <tphilipp@potion-studios.com> AuthorDate: 2021-10-08 10:46:15 +0000 Commit: Mikael Urankar <mikael@FreeBSD.org> CommitDate: 2021-10-08 12:47:10 +0000 devel/dyncall: Fix build on arm PR: 258846 Reported by: Robert Clausecker (cherry picked from commit 17631aa22f13fa4cecfed6cf92bd3ac15334454b) devel/dyncall/files/patch-arm (new) | 176 ++++++++++++++++++++++++++++++++++++ 1 file changed, 176 insertions(+)