| Message ID | 20210428134730.187533-4-y.karadz@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e489a3b247aae8a3a556504ce6bf0da37190a99b |
| Headers | show |
| Series | (Not so) Minor fixes toward KS 2.0 | expand |
On Wed, 28 Apr 2021 16:47:24 +0300 "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: > The KS_DEFINE_PLUGIN_CONTEXT macro implements methods that are used > to deal with plugin-specific context objects. However, when this macro > is used in multiple plugins and those plugins are loaded together > the symbol resolving fails, resulting in undefined behavior. Namely, > version of the function from one plugin, being called by another > plugin. Here we make sure that the methods defined in > KS_DEFINE_PLUGIN_CONTEXT are not visible outside of the corresponding > plugin. > > Fixing: 15df009 (kernel-shark: Add KS_DEFINE_PLUGIN_CONTEXT macro) > Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> > --- > src/libkshark-plugin.h | 22 ++++++++++++++++++---- > src/plugins/sched_events.c | 3 +++ > src/plugins/sched_events.h | 3 +-- > 3 files changed, 22 insertions(+), 6 deletions(-) > > diff --git a/src/libkshark-plugin.h b/src/libkshark-plugin.h > index c110616..752dbeb 100644 > --- a/src/libkshark-plugin.h > +++ b/src/libkshark-plugin.h > @@ -24,6 +24,8 @@ extern "C" { > /* Quiet warnings over documenting simple structures */ > //! @cond Doxygen_Suppress > > +#define __hidden __attribute__((visibility ("hidden"))) > + > #define _MAKE_STR(x) #x > > #define MAKE_STR(x) _MAKE_STR(x) > @@ -364,11 +366,14 @@ int kshark_handle_all_dpis(struct kshark_data_stream *stream, > __ok; \ > }) \ > > -/** General purpose macro defining methods for adding plugin context. */ > +/** > + * General purpose macro defining methods for adding plugin context. > + * Do not use this macro in header files. > + */ > #define KS_DEFINE_PLUGIN_CONTEXT(type) \ > static type **__context_handler; \ > static ssize_t __n_streams = -1; \ > -static inline type *__init(int sd) \ > +__hidden type *__init(int sd) \ This is strange, because static inline should never be a problem in this regard (otherwise things will break elsewhere too). static is never exported (it's stronger than "hidden"). Can you show me how you see this error, because this solution does not make any sense. -- Steve
On 6.05.21 г. 21:11, Steven Rostedt wrote: > On Wed, 28 Apr 2021 16:47:24 +0300 > "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: > >> The KS_DEFINE_PLUGIN_CONTEXT macro implements methods that are used >> to deal with plugin-specific context objects. However, when this macro >> is used in multiple plugins and those plugins are loaded together >> the symbol resolving fails, resulting in undefined behavior. Namely, >> version of the function from one plugin, being called by another >> plugin. Here we make sure that the methods defined in >> KS_DEFINE_PLUGIN_CONTEXT are not visible outside of the corresponding >> plugin. >> >> Fixing: 15df009 (kernel-shark: Add KS_DEFINE_PLUGIN_CONTEXT macro) >> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> >> --- >> src/libkshark-plugin.h | 22 ++++++++++++++++++---- >> src/plugins/sched_events.c | 3 +++ >> src/plugins/sched_events.h | 3 +-- >> 3 files changed, 22 insertions(+), 6 deletions(-) >> >> diff --git a/src/libkshark-plugin.h b/src/libkshark-plugin.h >> index c110616..752dbeb 100644 >> --- a/src/libkshark-plugin.h >> +++ b/src/libkshark-plugin.h >> @@ -24,6 +24,8 @@ extern "C" { >> /* Quiet warnings over documenting simple structures */ >> //! @cond Doxygen_Suppress >> >> +#define __hidden __attribute__((visibility ("hidden"))) >> + >> #define _MAKE_STR(x) #x >> >> #define MAKE_STR(x) _MAKE_STR(x) >> @@ -364,11 +366,14 @@ int kshark_handle_all_dpis(struct kshark_data_stream *stream, >> __ok; \ >> }) \ >> >> -/** General purpose macro defining methods for adding plugin context. */ >> +/** >> + * General purpose macro defining methods for adding plugin context. >> + * Do not use this macro in header files. >> + */ >> #define KS_DEFINE_PLUGIN_CONTEXT(type) \ >> static type **__context_handler; \ >> static ssize_t __n_streams = -1; \ >> -static inline type *__init(int sd) \ >> +__hidden type *__init(int sd) \ > > This is strange, because static inline should never be a problem in this > regard (otherwise things will break elsewhere too). > > static is never exported (it's stronger than "hidden"). > > Can you show me how you see this error, because this solution does not make > any sense. The problem is that some plugins can build from multiple source files. For example in the case when part of the plugin is written in C and another part in C++. In those cases we cannot have the functions being static. Thanks! Yordan > > -- Steve >
On Mon, 10 May 2021 14:53:08 +0300 Yordan Karadzhov <y.karadz@gmail.com> wrote: > > Can you show me how you see this error, because this solution does not make > > any sense. > > The problem is that some plugins can build from multiple source files. > For example in the case when part of the plugin is written in C and > another part in C++. In those cases we cannot have the functions being > static. So it's because its a mixture of C and C++ code? And you can't make them static? So this isn't a name resolution issue, it's a C to C++ issue where static doesn't work? If that is the case then the change log is misleading. -- Steve
On 10.05.21 г. 21:25, Steven Rostedt wrote: > On Mon, 10 May 2021 14:53:08 +0300 > Yordan Karadzhov <y.karadz@gmail.com> wrote: > >>> Can you show me how you see this error, because this solution does not make >>> any sense. >> >> The problem is that some plugins can build from multiple source files. >> For example in the case when part of the plugin is written in C and >> another part in C++. In those cases we cannot have the functions being >> static. > > So it's because its a mixture of C and C++ code? And you can't make them > static? No, this has no direct connection with C++. The static functions are the same in C++. It became an issue if you have multiple source files. If this is the case you have to put the macro in a header file, so that you can use the functions defined in the macro in all your source files. But this does not work, because the macro defines some global variables as well. To solve this I defined second macro to be used only in the header, but then the functions can't be static. Thanks! Yordan > > So this isn't a name resolution issue, it's a C to C++ issue where static > doesn't work? If that is the case then the change log is misleading. > > -- Steve >
On Mon, 10 May 2021 21:50:57 +0300 Yordan Karadzhov <y.karadz@gmail.com> wrote: > On 10.05.21 г. 21:25, Steven Rostedt wrote: > > On Mon, 10 May 2021 14:53:08 +0300 > > Yordan Karadzhov <y.karadz@gmail.com> wrote: > > > >>> Can you show me how you see this error, because this solution does not make > >>> any sense. > >> > >> The problem is that some plugins can build from multiple source files. > >> For example in the case when part of the plugin is written in C and > >> another part in C++. In those cases we cannot have the functions being > >> static. > > > > So it's because its a mixture of C and C++ code? And you can't make them > > static? > > No, this has no direct connection with C++. The static functions are the > same in C++. > It became an issue if you have multiple source files. If this is the > case you have to put the macro in a header file, so that you can use the > functions defined in the macro in all your source files. But this does > not work, because the macro defines some global variables as well. To > solve this I defined second macro to be used only in the header, but > then the functions can't be static. > Still does not make sense. Obviously, I'm missing something to connect the dots. Can you show me the error message that you are fixing. Thanks, -- Steve
On 10.05.21 г. 22:23, Steven Rostedt wrote: > On Mon, 10 May 2021 21:50:57 +0300 > Yordan Karadzhov <y.karadz@gmail.com> wrote: > >> On 10.05.21 г. 21:25, Steven Rostedt wrote: >>> On Mon, 10 May 2021 14:53:08 +0300 >>> Yordan Karadzhov <y.karadz@gmail.com> wrote: >>> >>>>> Can you show me how you see this error, because this solution does not make >>>>> any sense. >>>> >>>> The problem is that some plugins can build from multiple source files. >>>> For example in the case when part of the plugin is written in C and >>>> another part in C++. In those cases we cannot have the functions being >>>> static. >>> >>> So it's because its a mixture of C and C++ code? And you can't make them >>> static? >> >> No, this has no direct connection with C++. The static functions are the >> same in C++. >> It became an issue if you have multiple source files. If this is the >> case you have to put the macro in a header file, so that you can use the >> functions defined in the macro in all your source files. But this does >> not work, because the macro defines some global variables as well. To >> solve this I defined second macro to be used only in the header, but >> then the functions can't be static. >> > > Still does not make sense. Obviously, I'm missing something to connect the > dots. > > Can you show me the error message that you are fixing. It is not an error message. It is a malfunctioning. Say you have a macro that looks like this: #define KS_DEFINE_PLUGIN_CONTEXT(type) \ static type **__context_handler; \ static ssize_t __n_streams = -1; \ static inline type *__get_context(int sd) \ { \ if (sd < 0 || sd >= __n_streams) \ return NULL; \ return __context_handler[sd]; \ } \ and you want to use __get_context(int sd) in different source files. If you just place the macro in a header, each source file will have its own "static __context_handler" variable which is not what I would like. So this is a malfunctioning. I can solve this problem by having a second macro that only declares "type *__get_context(int sd)" and place this macro in the header, while keeping the original macro in one of the source files, but then __get_context() cannot be static. If I go this way and I have multiple plugins using the same macro I will have multiple versions of the same function and this time the dynamic linking will start to misbehave. Does that make sense? Thanks! Yordan > > Thanks, > > -- Steve >
On 10.05.21 г. 22:23, Steven Rostedt wrote: > Obviously, I'm missing something to connect the > dots. And I have to admit that the commit message is misleading. Thanks a lot! Yordan
On Mon, 10 May 2021 23:34:50 +0300 Yordan Karadzhov <y.karadz@gmail.com> wrote: > On 10.05.21 г. 22:23, Steven Rostedt wrote: > > Obviously, I'm missing something to connect the > > dots. > > And I have to admit that the commit message is misleading. :-) Yes, please rewrite the commit message to explain exactly what that patch is doing. That's what threw me for the loop. I read it, and was basing all my review on what the commit message said, and not what I saw in the patch. -- Steve
On 11.05.21 г. 0:02, Steven Rostedt wrote: > On Mon, 10 May 2021 23:34:50 +0300 > Yordan Karadzhov <y.karadz@gmail.com> wrote: > >> On 10.05.21 г. 22:23, Steven Rostedt wrote: >>> Obviously, I'm missing something to connect the >>> dots. >> >> And I have to admit that the commit message is misleading. > > :-) > > Yes, please rewrite the commit message to explain exactly what that patch > is doing. That's what threw me for the loop. I read it, and was basing all > my review on what the commit message said, and not what I saw in the patch. > Done: https://git.kernel.org/pub/scm/utils/trace-cmd/kernel-shark.git/commit/?id=e489a3b247aae8a3a556504ce6bf0da37190a99b and the link in the commit message points to this email thread. Thanks! Yordan > -- Steve >
diff --git a/src/libkshark-plugin.h b/src/libkshark-plugin.h index c110616..752dbeb 100644 --- a/src/libkshark-plugin.h +++ b/src/libkshark-plugin.h @@ -24,6 +24,8 @@ extern "C" { /* Quiet warnings over documenting simple structures */ //! @cond Doxygen_Suppress +#define __hidden __attribute__((visibility ("hidden"))) + #define _MAKE_STR(x) #x #define MAKE_STR(x) _MAKE_STR(x) @@ -364,11 +366,14 @@ int kshark_handle_all_dpis(struct kshark_data_stream *stream, __ok; \ }) \ -/** General purpose macro defining methods for adding plugin context. */ +/** + * General purpose macro defining methods for adding plugin context. + * Do not use this macro in header files. + */ #define KS_DEFINE_PLUGIN_CONTEXT(type) \ static type **__context_handler; \ static ssize_t __n_streams = -1; \ -static inline type *__init(int sd) \ +__hidden type *__init(int sd) \ { \ type *obj; \ if (__n_streams < 0 && sd < KS_DEFAULT_NUM_STREAMS) { \ @@ -388,7 +393,7 @@ static inline type *__init(int sd) \ __context_handler[sd] = obj; \ return obj; \ } \ -static inline void __close(int sd) \ +__hidden void __close(int sd) \ { \ if (sd < 0) { \ free(__context_handler); \ @@ -398,13 +403,22 @@ static inline void __close(int sd) \ free(__context_handler[sd]); \ __context_handler[sd] = NULL; \ } \ -static inline type *__get_context(int sd) \ +__hidden type *__get_context(int sd) \ { \ if (sd < 0 || sd >= __n_streams) \ return NULL; \ return __context_handler[sd]; \ } \ +/** + * General purpose macro declaring the methods for adding plugin context. + * To be used in header files. + */ +#define KS_DECLARE_PLUGIN_CONTEXT_METHODS(type) \ +type *__init(int sd); \ +void __close(int sd); \ +type *__get_context(int sd); \ + #ifdef __cplusplus } #endif // __cplusplus diff --git a/src/plugins/sched_events.c b/src/plugins/sched_events.c index ac4a7bf..5798322 100644 --- a/src/plugins/sched_events.c +++ b/src/plugins/sched_events.c @@ -73,6 +73,9 @@ int plugin_sched_get_prev_state(ks_num_field_t field) return (field & mask) >> PREV_STATE_SHIFT; } +/** A general purpose macro is used to define plugin context. */ +KS_DEFINE_PLUGIN_CONTEXT(struct plugin_sched_context); + static bool plugin_sched_init_context(struct kshark_data_stream *stream, struct plugin_sched_context *plugin_ctx) { diff --git a/src/plugins/sched_events.h b/src/plugins/sched_events.h index 78cfda0..2c540fd 100644 --- a/src/plugins/sched_events.h +++ b/src/plugins/sched_events.h @@ -53,8 +53,7 @@ struct plugin_sched_context { struct kshark_data_container *sw_data; }; -/** A general purpose macro is used to define plugin context. */ -KS_DEFINE_PLUGIN_CONTEXT(struct plugin_sched_context); +KS_DECLARE_PLUGIN_CONTEXT_METHODS(struct plugin_sched_context) /** The type of the data field stored in the kshark_data_container object. */ typedef int64_t ks_num_field_t;
The KS_DEFINE_PLUGIN_CONTEXT macro implements methods that are used to deal with plugin-specific context objects. However, when this macro is used in multiple plugins and those plugins are loaded together the symbol resolving fails, resulting in undefined behavior. Namely, version of the function from one plugin, being called by another plugin. Here we make sure that the methods defined in KS_DEFINE_PLUGIN_CONTEXT are not visible outside of the corresponding plugin. Fixing: 15df009 (kernel-shark: Add KS_DEFINE_PLUGIN_CONTEXT macro) Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> --- src/libkshark-plugin.h | 22 ++++++++++++++++++---- src/plugins/sched_events.c | 3 +++ src/plugins/sched_events.h | 3 +-- 3 files changed, 22 insertions(+), 6 deletions(-)