Page MenuHomeFreeBSD

src.conf: Introduce WITHOUT_MACHDEP knob.
ClosedPublic

Authored by arrowd on Aug 8 2022, 3:24 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 14, 8:05 AM
Unknown Object (File)
Oct 4 2024, 9:40 PM
Unknown Object (File)
Oct 4 2024, 9:24 AM
Unknown Object (File)
Oct 3 2024, 9:57 PM
Unknown Object (File)
Sep 28 2024, 9:17 AM
Unknown Object (File)
Sep 27 2024, 10:18 PM
Unknown Object (File)
Sep 26 2024, 4:48 PM
Unknown Object (File)
Sep 25 2024, 8:22 PM

Details

Summary

This knob can be used to make buildsystem prefer generic C implentations of
various functions, instead of assembler ones.

Test Plan

make buildworld on amd64

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 46786
Build 43675: arc lint + arc unit

Event Timeline

arrowd requested review of this revision.Aug 8 2022, 3:24 PM
emaste requested changes to this revision.Aug 8 2022, 3:58 PM
emaste added a reviewer: imp.
emaste added a subscriber: emaste.
  • src.conf.5 is automatically generated from WITH_/WITHOUT_ descriptions in tools/build/options, it should not be modified directly
  • likewise WITH_ and WITHOUT_ options are controlled by share/mk/src.opts.mk and set MK_{OPTION}, if this option is going to be added it needs to be done there

I think this is useful functionality so that we can test the generic C implementations where optimized MD assembly exists, but I worry that we don't have a distinction between things that are MD optimizations with MI fallbacks, and things that are inherently MD like the lib/csu bits. Not an issue for the components modified in this review, but maybe the name should be WITHOUT_MACHDEP_OPTIMIZATIONS [or something similar, but shorter].

This revision now requires changes to proceed.Aug 8 2022, 3:58 PM
tools/build/options/WITHOUT_MACHDEP_OPTIMIZATIONS
2 ↗(On Diff #109008)

This patch doesn't even come close to doing that. We have dozens of assembler optimized routines, especially in compression, crypto, etc that this doesn't touch. Nor should it in many cases as the slowdown is quite substantial.

Please say that it just does libc and libm routines. That would be more accurate and honest.

tools/build/options/WITHOUT_MACHDEP_OPTIMIZATIONS
2 ↗(On Diff #109008)

I'm using this knob to run the compiled code under KLEE symbolic analyzer, which doesn't handle inline asm. Ideally, I'd like to analyze the whole FreeBSD source tree. Maybe instead of constraining this knob to libc and libm only we commit it as it is and then gradually add more and more libraries?

commit it as it is and then gradually add more and more libraries

I think that's reasonable, but @imp's point I think is just that the description should match what the option actually does. Maybe something like "in selected libraries" or similar rather than "across the code base."

commit it as it is and then gradually add more and more libraries

I think that's reasonable, but @imp's point I think is just that the description should match what the option actually does. Maybe something like "in selected libraries" or similar rather than "across the code base."

There's dozens of these things it doesn't do, it shouldn't claim to. If it was one or two out of 5 that would be one thing, but it's one or two out of dozens.

Add "At present, it just covers libc and libm." at the end, and we can expand that as we see fit as the scope increases.

I'm not sure I like the name, but I have nothing better that's not horribly long. Maybe WITHOUT_ASSEMBLER_OPTIMIZATIONS but I'm not sure that's much better. So I'm not objecting to the name, just that the description is too vague and gives such a misleading impression that it should be amended as suggested.

My only worry with mentioning that it applies only to libc and libm is we have many similar examples where lists in src.conf have become outdated -- I could easily see someone adding another library but forgetting to update the description. We could mention them as examples but not suggest they are an exhaustive list.

My only worry with mentioning that it applies only to libc and libm is we have many similar examples where lists in src.conf have become outdated -- I could easily see someone adding another library but forgetting to update the description. We could mention them as examples but not suggest they are an exhaustive list.

Hence my comments about reevaluating as we add more. I'll keep an eye out for reviews and/or commits and make sure things get updated. Once we get to a certain point, it will be burdensome to have a list and we can go with a more vague description...

Rephrase the knob description.

So, how do we move forward with this?

I think it's good to go now, it's was only the wording of the option description at issue and it is now addressed.

lib/msun/Makefile
154

Oh, except that for some reason it's typical to use != no rather than == yes

$ git ls-files | grep Makefile | xargs grep '== "yes"' | wc -l
      80
$ git ls-files | grep Makefile | xargs grep '== yes' | wc -l
       6
$ git ls-files | grep Makefile | xargs grep '!= "no"' | wc -l
     738
$ git ls-files | grep Makefile | xargs grep '!= no' | wc -l
       6

the most common case by a large margin is != "no" even though the quotes are not necessary and IMO == yes is easier to understand.
@imp do you know why it's this way?

I'd prefer != no, rather than == yes, but I'm not going to put a fuss up either way.
My big issue was with the wording, and that's been solved now.
You don't need the $FreeBSD$ either, so I'd prefer that not go in, but if it does it isn't going to cause an issue.

lib/msun/Makefile
154

Old-school FreeBSD make had issues, I think just for a short time, that required the quoting in some odd cases, so they were done 'religiously' by ru@ when he converted everything from the !define(NOFOO) -> MK_FOO != "no" and it was copied from there. I'm unaware of any even edge case transient issues in this area in the past decade though.

The reason for != no (with or without the quotes) was there was an older pattern for some of the options that was basically no, yes or something that meant some kind of subset (think the sendmail_enable wart we have today). All of those are long gone from the build system though, and I'd suggest, though, we use != no here for consistency. But it's just a suggestion.

Oh yeah, no need to put the $FreeBSD$ in.

Thanks @imp for the explanation. I'm indifferent, == yes is fine with me. It's already about 10% of cases so it should keep working.

This revision is now accepted and ready to land.Sep 8 2022, 4:01 PM
  • Remove $FreeBSD$.
  • Use != no instead of == yes.
This revision now requires review to proceed.Sep 9 2022, 6:51 AM
This revision was not accepted when it landed; it landed in state Needs Review.Sep 9 2022, 6:54 AM
This revision was automatically updated to reflect the committed changes.