Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib: fix build error on MacOS #15890

Merged
merged 1 commit into from
May 1, 2024

Conversation

httpstorm
Copy link
Contributor

@httpstorm httpstorm commented Apr 30, 2024

Sections use a different syntax for Mach-O executables.

The frr package has a host build step in OpenWRT, which can be cross-compiled on macOS. This commit fixes errors while building OpenWRT on macOS host.

Fixes:

# OpenWRT
make package/feeds/packages/frr/host/{clean,compile} -j 1 V=s
…
lib/command_graph.c:16:1: error: argument to 'section' attribute is not valid for this target: mach-o section specifier requires a segment and section separated by a comma DEFINE_MTYPE_STATIC(LIB, CMD_TOKENS, "Command Tokens"); ^
./lib/memory.h:139:2: note: expanded from macro 'DEFINE_MTYPE_STATIC'
        DEFINE_MTYPE_ATTR(group, name, static, desc)                           \
        ^
./lib/memory.h:109:26: note: expanded from macro 'DEFINE_MTYPE_ATTR'
                __attribute__((section(".data.mtypes"))) = { {                 \

Compile tested as part of OpenWRT buildroot: macOS 14.4.1, Ubuntu 22.04.4 LTS.
Changes from the original version [1]: rebased on master, fixed formatting style. Please let me know if any changes are needed.

[1] #6032

Maintainer: @eqvinox
@polychaeta @qlyoung @donaldsharp @rzalamena @rubenk @robimarko

@eqvinox
Copy link
Contributor

eqvinox commented Apr 30, 2024

This is only the tip of the iceberg that you would need to tackle to get FRR to work on macOS. The FRR community has decided it's not worth the effort. If you really want, I suppose you could try to fix all of the entire iceberg, but — again, this is only the tip.

Take a look at clippy and what it does with libelf, that's not gonna work on macOS full stop…

@httpstorm
Copy link
Contributor Author

@eqvinox
The frr package has a host build step in OpenWRT, which can be cross-compiled on macOS. I'm not sure where it is used and why. This commit fixes errors while building OpenWRT on macOS host. I hope this clarifies the reason behind my contribution.
openwrt/packages#24053

@eqvinox
Copy link
Contributor

eqvinox commented Apr 30, 2024

host build step in OpenWRT, which can be cross-compiled on macOS

Aaaaah that makes much more sense. Please mention that the fix is for that ;)

@httpstorm
Copy link
Contributor Author

@eqvinox

Aaaaah that makes much more sense. Please mention that the fix is for that ;)

Ha hah, done. Sorry for the confusion. I changed the description. 😆

@eqvinox
Copy link
Contributor

eqvinox commented Apr 30, 2024

Hmm, since this affects only clippy, the easier fix would be to just remove the section attribute. It's not needed there.

(The primary purpose of this is to place the MTYPE variables close to each other to get [page] cache locality, so technically the entire thing is optional—nothing will break if you remove those specific bits. To be clear, this applies only here for MTYPE/MGROUP. There are other section attributes in xref_* and those are absolutely needed. Just to note in case someone is looking at porting FRR to macOS.)

@eqvinox
Copy link
Contributor

eqvinox commented Apr 30, 2024

To be clear, the xref stuff is not used for clippy (which is the host build step.)

lib/memory.h Outdated
@@ -120,7 +117,7 @@ struct memgroup {
_mg_##group.insert = &_mg_##group.types; \
MTYPE_##mname->ref = _mg_##group.insert; \
*_mg_##group.insert = MTYPE_##mname; \
_mg_##group.insert = &MTYPE_##mname->next; \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

↑ Unrelated whitespace change

@eqvinox
Copy link
Contributor

eqvinox commented Apr 30, 2024

the easier fix would be to just remove the section attribute.

Hmm. I guess there's no benefit to change it to remove the attribute, this should be fine as is. Please remove that added whitespace though 😃

Sections use a different syntax for Mach-O executables.

Fixes:

lib/bfd.c:35:1: error: argument to 'section' attribute is not valid for this target: mach-o section specifier requires a segment and section separated by a
      comma
DEFINE_MTYPE_STATIC(LIB, BFD_INFO, "BFD info")
^
./lib/memory.h:140:2: note: expanded from macro 'DEFINE_MTYPE_STATIC'
        DEFINE_MTYPE_ATTR(group, name, static, desc)                           \
        ^
./lib/memory.h:110:26: note: expanded from macro 'DEFINE_MTYPE_ATTR'
                __attribute__((section(".data.mtypes"))) = { {                 \
                                       ^
1 error generated.

Signed-off-by: Ruben Kerkhof <ruben@rubenkerkhof.com>
Signed-off-by: Georgi Valkov <gvalkov@gmail.com>
@httpstorm httpstorm force-pushed the frr-macOS-section.2024-04-30 branch from adf18c9 to 9824f07 Compare April 30, 2024 17:24
@httpstorm
Copy link
Contributor Author

httpstorm commented Apr 30, 2024

@eqvinox

Hmm. I guess there's no benefit to change it to remove the attribute, this should be fine as is. Please remove that added whitespace though 😃

Ready.

Copy link
Contributor

@eqvinox eqvinox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@eqvinox
Copy link
Contributor

eqvinox commented Apr 30, 2024

Merging when CI completes (which will take a bit because it's Tuesday which is "CI overloaded" day for FRR)

@httpstorm
Copy link
Contributor Author

@eqvinox
Thank you for your help!

httpstorm added a commit to httpstorm/openwrt.packages that referenced this pull request Apr 30, 2024
Fixes:
lib/command_graph.c:16:1: error: argument to 'section' attribute is not valid for this target: mach-o section specifier requires a segment and section separated by a comma DEFINE_MTYPE_STATIC(LIB, CMD_TOKENS, "Command Tokens"); ^
./lib/memory.h:139:2: note: expanded from macro 'DEFINE_MTYPE_STATIC'
        DEFINE_MTYPE_ATTR(group, name, static, desc)                           \
        ^
./lib/memory.h:109:26: note: expanded from macro 'DEFINE_MTYPE_ATTR'
                __attribute__((section(".data.mtypes"))) = { {                 \

[1] FRRouting/frr#6032
[2] FRRouting/frr#15890

Signed-off-by: Georgi Valkov <gvalkov@gmail.com>
robimarko pushed a commit to openwrt/packages that referenced this pull request Apr 30, 2024
Fixes:
lib/command_graph.c:16:1: error: argument to 'section' attribute is not valid for this target: mach-o section specifier requires a segment and section separated by a comma DEFINE_MTYPE_STATIC(LIB, CMD_TOKENS, "Command Tokens"); ^
./lib/memory.h:139:2: note: expanded from macro 'DEFINE_MTYPE_STATIC'
        DEFINE_MTYPE_ATTR(group, name, static, desc)                           \
        ^
./lib/memory.h:109:26: note: expanded from macro 'DEFINE_MTYPE_ATTR'
                __attribute__((section(".data.mtypes"))) = { {                 \

[1] FRRouting/frr#6032
[2] FRRouting/frr#15890

Signed-off-by: Georgi Valkov <gvalkov@gmail.com>
@eqvinox eqvinox merged commit 2130575 into FRRouting:master May 1, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants