-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 error on MacOS #6032
lib: fix error on MacOS #6032
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution to FRR!
Click for style suggestions
To apply these suggestions:
curl -s https://gist.githubusercontent.com/polychaeta/3f3ea50f2f5f9bd11fd2ea4fbdc7dcc9/raw/07a8e684d5bc430e1b31623df4d2a1b23f720dcd/cr_6032_1584543107.diff | git apply
diff --git a/lib/memory.h b/lib/memory.h
index cf3390af4..f89a5c299 100644
--- a/lib/memory.h
+++ b/lib/memory.h
@@ -83,13 +83,12 @@ struct memgroup {
#define DECLARE_MGROUP(name) extern struct memgroup _mg_##name;
#define DEFINE_MGROUP(mname, desc) \
- struct memgroup _mg_##mname \
- ATTRIBUTE_MGROUPS = { \
- .name = desc, \
- .types = NULL, \
- .next = NULL, \
- .insert = NULL, \
- .ref = NULL, \
+ struct memgroup _mg_##mname ATTRIBUTE_MGROUPS = { \
+ .name = desc, \
+ .types = NULL, \
+ .next = NULL, \
+ .insert = NULL, \
+ .ref = NULL, \
}; \
static void _mginit_##mname(void) __attribute__((_CONSTRUCTOR(1000))); \
static void _mginit_##mname(void) \
@@ -119,14 +118,13 @@ struct memgroup {
#endif
#define DEFINE_MTYPE_ATTR(group, mname, attr, desc) \
- attr struct memtype MTYPE_##mname[1] \
- ATTRIBUTE_MTYPES = { { \
- .name = desc, \
- .next = NULL, \
- .n_alloc = 0, \
- .size = 0, \
- .ref = NULL, \
- } }; \
+ attr struct memtype MTYPE_##mname[1] ATTRIBUTE_MTYPES = {{ \
+ .name = desc, \
+ .next = NULL, \
+ .n_alloc = 0, \
+ .size = 0, \
+ .ref = NULL, \
+ }}; \
static void _mtinit_##mname(void) __attribute__((_CONSTRUCTOR(1001))); \
static void _mtinit_##mname(void) \
{ \
@@ -134,7 +132,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; \
+ _mg_##group.insert = &MTYPE_##mname->next; \
} \
static void _mtfini_##mname(void) __attribute__((_DESTRUCTOR(1001))); \
static void _mtfini_##mname(void) \
If you are a new contributor to FRR, please see our contributing guidelines.
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: FailedCheckout code: Failed (click for details)Checkout code: No useful log found |
Please note I tried to verify that these attributes actually add new segments into the binary, but they don't show up on Linux in the output of readelf -a, before or after this change. I can't find where we actually use this functionality (separate segment per MTYPE), so perhaps the better change here would be to remove the attributes altogether? @eqvinox what do you think? |
ci:rerun |
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: FailedCheckout code: Failed (click for details)Checkout code: No useful log found |
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotest tests on Ubuntu 16.04 i386: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOI386-11266/test Topology Tests failed for Topotest tests on Ubuntu 16.04 i386:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-11266/artifact/TOPOI386/ErrorLog/log_topotests.txt Successful on other platforms/tests
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
|
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-11265/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
|
ci:rerun |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-11277/ This is a comment from an automated CI system. |
973ec7c
to
a56ce75
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution to FRR!
Click for style suggestions
To apply these suggestions:
curl -s https://gist.githubusercontent.com/polychaeta/3d4d29a827ac1ab8ac227cd8a8a771d7/raw/07a8e684d5bc430e1b31623df4d2a1b23f720dcd/cr_6032_1584639330.diff | git apply
diff --git a/lib/memory.h b/lib/memory.h
index cf3390af4..f89a5c299 100644
--- a/lib/memory.h
+++ b/lib/memory.h
@@ -83,13 +83,12 @@ struct memgroup {
#define DECLARE_MGROUP(name) extern struct memgroup _mg_##name;
#define DEFINE_MGROUP(mname, desc) \
- struct memgroup _mg_##mname \
- ATTRIBUTE_MGROUPS = { \
- .name = desc, \
- .types = NULL, \
- .next = NULL, \
- .insert = NULL, \
- .ref = NULL, \
+ struct memgroup _mg_##mname ATTRIBUTE_MGROUPS = { \
+ .name = desc, \
+ .types = NULL, \
+ .next = NULL, \
+ .insert = NULL, \
+ .ref = NULL, \
}; \
static void _mginit_##mname(void) __attribute__((_CONSTRUCTOR(1000))); \
static void _mginit_##mname(void) \
@@ -119,14 +118,13 @@ struct memgroup {
#endif
#define DEFINE_MTYPE_ATTR(group, mname, attr, desc) \
- attr struct memtype MTYPE_##mname[1] \
- ATTRIBUTE_MTYPES = { { \
- .name = desc, \
- .next = NULL, \
- .n_alloc = 0, \
- .size = 0, \
- .ref = NULL, \
- } }; \
+ attr struct memtype MTYPE_##mname[1] ATTRIBUTE_MTYPES = {{ \
+ .name = desc, \
+ .next = NULL, \
+ .n_alloc = 0, \
+ .size = 0, \
+ .ref = NULL, \
+ }}; \
static void _mtinit_##mname(void) __attribute__((_CONSTRUCTOR(1001))); \
static void _mtinit_##mname(void) \
{ \
@@ -134,7 +132,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; \
+ _mg_##group.insert = &MTYPE_##mname->next; \
} \
static void _mtfini_##mname(void) __attribute__((_DESTRUCTOR(1001))); \
static void _mtfini_##mname(void) \
If you are a new contributor to FRR, please see our contributing guidelines.
please apply the style suggestions |
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>
a56ce75
to
ad26e09
Compare
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-11309/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
|
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-11315/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
|
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK on change, but needs polish.
#define ATTRIBUTE_MGROUPS __attribute__((section("__DATA,mgroups"))) | ||
#else | ||
#define ATTRIBUTE_MGROUPS __attribute__((section(".data.mgroups"))) | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK, but
- this belongs in
lib/compiler.h
- please make it one macro with a parameterized section name
#ifdef __MACH__
#define _DATA_SECTION(name) __attribute__((section("__DATA," name)))
#else
#define _DATA_SECTION(name) __attribute__((section(".data." name)))
#endif
(Note consecutive string literals in C are automatically concatenated, "foo" "bar"
is the same as "foobar"
. This does work correctly for GCC attribute parameters.)
would be used like this:
struct memgroup _mg_##mname _DATA_SECTION("mgroups")
(we have some macros with the __attribute__
included in the macro, and some without the __attribute__
- I don't really care much either way on that... maybe we should clean that up and make it consistent one way or another.)
@rubenk are you planning to address the comments and update this PR? If not please let us know so we can give the work to someone else to finish up |
@polychaeta autoclose in 1 week |
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 Signed-off-by: Georgi Valkov <gvalkov@gmail.com>
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 Signed-off-by: Georgi Valkov <gvalkov@gmail.com>
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 Signed-off-by: Georgi Valkov <gvalkov@gmail.com>
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>
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>
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