[lttng-dev] [PATCH lttng-ust] Fix: make 'hello.cxx' compile with g++
Mathieu Desnoyers
mathieu.desnoyers at efficios.com
Sun Apr 21 17:36:22 EDT 2013
* Zifei Tong (soariez at gmail.com) wrote:
> Hi,
>
> Thanks for your review.
>
> > Why is this lttng_ust_lib_ring_buffer_config:: scoping needed in c++ ?
>
> C++ will hide enumeration defined inside class/struct.
>
> Quote from http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2011/n3242.pdf,
> page 158:
> > An enumerator declared in class scope can be referred to using the class
> member access
> > operators (::, . (dot) and -> (arrow)) [Example:
> > struct X {
> > enum direction { left=’l’, right=’r’ };
> > };
> > void g(X* p) {
> > int i;
> > i = p->f(left); // error: left not in scope
> > i = p->f(X::right); // OK
> > }
> > — end example ]
>
>
> > inlined precompiler directives like this make the function hard to
> > follow (mix of #if/#else/#endif and if/else).
>
> I agree inlined precompiler directives is bad style. Could you suggest a
> way to remove the mix of #if/#else/#endif and if/else?
Maybe we could simply left the numeration outside of the structure
declaration ? This would make its scope global in both c and c++.
Please try making this change, and make sure you keep this documentation
on the reasons why we need to do this change in the patch changelog. It
will be useful for future reference.
>
> > So each field need to be listed ? We usually don't put NULL
> > initialization for structures that are always in zero-initialized
> > memory. (coding style)
>
> This is related to a known issue of g++:
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55606 (Bug 55606 - sorry,
> unimplemented: non-trivial designated initializers not supported).
>
> g++'s 'trivial designated initializers' means no out-of-order
> initialization, no missing
> initialization (except the fields on the tail of a struct), and nested
> initialization should be done in the form {.foo = {.bar = 1}} instead of
> {.foo.bar = 1}. That's why I made such modification.
OK, this justifies the style to coding style. Please document in the
patch changelog.
>
> > Are those changes also compatible with the LLVM c++ compiler ?
> Actually, clang++ have designated initializers better supported than g++.
> All the modification about designated initializers are not required for
> clang++. No need to add NULL initialization, reorder initializations or
> change {.foo.bar = 1} into {.foo = {.bar = 1}}. These (ugly) hacks are just
> to make g++ happy.
ok, good to know.
>
> Cleaned-up patch attached.
More comments below,
>
> Thanks.
>
> From 36930fdfa25a6ea67de551300263dee353df43e5 Mon Sep 17 00:00:00 2001
> From: Zifei Tong <soariez at gmail.com>
> Date: Sat, 20 Apr 2013 19:38:39 +0800
> Subject: [PATCH lttng-ust] Fix: make 'hello.cxx' compile with g++
>
> Fixes #338
>
> Signed-off-by: Zifei Tong <soariez at gmail.com>
> ---
> include/lttng/ringbuffer-config.h | 5 +++++
> include/lttng/tracepoint-event.h | 12 +++++------
> include/lttng/ust-events.h | 42
> +++++++++++++++++++++++-------------
> include/lttng/ust-tracepoint-event.h | 32 +++++++++++++++++----------
> tests/hello.cxx/Makefile.am | 2 +-
> tests/hello.cxx/tp.c | 26 ----------------------
> tests/hello.cxx/tp.cpp | 26 ++++++++++++++++++++++
> 7 files changed, 86 insertions(+), 59 deletions(-)
> delete mode 100644 tests/hello.cxx/tp.c
> create mode 100644 tests/hello.cxx/tp.cpp
>
> diff --git a/include/lttng/ringbuffer-config.h
> b/include/lttng/ringbuffer-config.h
> index ca52fc7..bb01eab 100644
> --- a/include/lttng/ringbuffer-config.h
> +++ b/include/lttng/ringbuffer-config.h
> @@ -347,8 +347,13 @@ int lib_ring_buffer_check_config(const struct
> lttng_ust_lib_ring_buffer_config *
> unsigned int switch_timer_interval,
> unsigned int read_timer_interval)
> {
> + #ifdef __cplusplus
> + if (config->alloc ==
> lttng_ust_lib_ring_buffer_config::RING_BUFFER_ALLOC_GLOBAL
> + && config->sync ==
> lttng_ust_lib_ring_buffer_config::RING_BUFFER_SYNC_PER_CPU
> + #else
> if (config->alloc == RING_BUFFER_ALLOC_GLOBAL
> && config->sync == RING_BUFFER_SYNC_PER_CPU
> + #endif
> && switch_timer_interval)
> return -EINVAL;
> return 0;
> diff --git a/include/lttng/tracepoint-event.h
> b/include/lttng/tracepoint-event.h
> index 077eaa0..4630788 100644
> --- a/include/lttng/tracepoint-event.h
> +++ b/include/lttng/tracepoint-event.h
> @@ -20,12 +20,12 @@
> * SOFTWARE.
> */
>
> +#ifdef TRACEPOINT_CREATE_PROBES
> +
> #ifdef __cplusplus
> extern "C" {
> #endif
>
> -#ifdef TRACEPOINT_CREATE_PROBES
> -
> #define __tp_stringify1(x) #x
> #define __tp_stringify(x) __tp_stringify1(x)
>
> @@ -65,10 +65,10 @@ extern "C" {
> #undef TRACEPOINT_INCLUDE_FILE
> #undef TRACEPOINT_INCLUDE
>
> -#define TRACEPOINT_CREATE_PROBES
> -
> -#endif /* TRACEPOINT_CREATE_PROBES */
Hrm, I don't get what this is supposed to fix ?
Normall
> -
> #ifdef __cplusplus
> }
> #endif
> +
> +#define TRACEPOINT_CREATE_PROBES
> +
> +#endif /* TRACEPOINT_CREATE_PROBES */
> diff --git a/include/lttng/ust-events.h b/include/lttng/ust-events.h
> index 749478a..e97cc1b 100644
> --- a/include/lttng/ust-events.h
> +++ b/include/lttng/ust-events.h
> @@ -89,15 +89,21 @@ struct lttng_enum_entry {
>
> #define __type_integer(_type, _byte_order, _base, _encoding) \
> { \
> - .atype = atype_integer, \
> - .u.basic.integer = \
> + .atype = atype_integer, \
> + .u = \
> { \
> - .size = sizeof(_type) * CHAR_BIT, \
> - .alignment = lttng_alignof(_type) * CHAR_BIT, \
> - .signedness = lttng_is_signed_type(_type), \
> - .reverse_byte_order = _byte_order != BYTE_ORDER, \
> - .base = _base, \
> - .encoding = lttng_encode_##_encoding, \
> + .basic = \
> + { \
> + .integer = \
> + { \
Please ensure that each nested "{" has its own indent level.
I also think your mail client turned tabs into spaces.
Thanks,
Mathieu
> + .size = sizeof(_type) * CHAR_BIT, \
> + .alignment = lttng_alignof(_type) * CHAR_BIT, \
> + .signedness = lttng_is_signed_type(_type), \
> + .reverse_byte_order = _byte_order != BYTE_ORDER, \
> + .base = _base, \
> + .encoding = lttng_encode_##_encoding, \
> + } \
> + } \
> }, \
> } \
>
> @@ -123,14 +129,20 @@ struct lttng_integer_type {
>
> #define __type_float(_type) \
> { \
> - .atype = atype_float, \
> - .u.basic._float = \
> + .atype = atype_float, \
> + .u = \
> { \
> - .exp_dig = sizeof(_type) * CHAR_BIT \
> - - _float_mant_dig(_type), \
> - .mant_dig = _float_mant_dig(_type), \
> - .alignment = lttng_alignof(_type) * CHAR_BIT, \
> - .reverse_byte_order = BYTE_ORDER != FLOAT_WORD_ORDER, \
> + .basic = \
> + { \
> + ._float = \
> + { \
> + .exp_dig = sizeof(_type) * CHAR_BIT \
> + - _float_mant_dig(_type), \
> + .mant_dig = _float_mant_dig(_type), \
> + .alignment = lttng_alignof(_type) * CHAR_BIT, \
> + .reverse_byte_order = BYTE_ORDER != FLOAT_WORD_ORDER, \
> + }
> + }
> }, \
> } \
>
> diff --git a/include/lttng/ust-tracepoint-event.h
> b/include/lttng/ust-tracepoint-event.h
> index e46cc1a..3aa946d 100644
> --- a/include/lttng/ust-tracepoint-event.h
> +++ b/include/lttng/ust-tracepoint-event.h
> @@ -147,11 +147,14 @@ static const char \
> .type = \
> { \
> .atype = atype_array, \
> - .u.array = \
> + .u = \
> { \
> - .length = _length, \
> - .elem_type = __type_integer(_type, BYTE_ORDER, 10, _encoding), \
> - }, \
> + .array = \
> + { \
> + .elem_type = __type_integer(_type, BYTE_ORDER, 10, _encoding), \
> + .length = _length, \
> + } \
> + } \
> }, \
> .nowrite = _nowrite, \
> },
> @@ -164,10 +167,13 @@ static const char \
> .type = \
> { \
> .atype = atype_sequence, \
> - .u.sequence = \
> + .u = \
> { \
> - .length_type = __type_integer(_length_type, BYTE_ORDER, 10, none), \
> - .elem_type = __type_integer(_type, BYTE_ORDER, 10, _encoding), \
> + .sequence =
> + { \
> + .length_type = __type_integer(_length_type, BYTE_ORDER, 10, none), \
> + .elem_type = __type_integer(_type, BYTE_ORDER, 10, _encoding), \
> + }, \
> }, \
> }, \
> .nowrite = _nowrite, \
> @@ -180,7 +186,10 @@ static const char \
> .type = \
> { \
> .atype = atype_string, \
> - .u.basic.string.encoding = lttng_encode_UTF8, \
> + .u =
> + {
> + .basic = { .string = { .encoding = lttng_encode_UTF8 } } \
> + }, \
> }, \
> .nowrite = _nowrite, \
> },
> @@ -483,7 +492,7 @@ void
> __event_probe__##_provider##___##_name(_TP_ARGS_DATA_PROTO(_args)); \
> static \
> void __event_probe__##_provider##___##_name(_TP_ARGS_DATA_PROTO(_args))
> \
> { \
> - struct lttng_event *__event = __tp_data; \
> + struct lttng_event *__event = (struct lttng_event *) __tp_data; \
> struct lttng_channel *__chan = __event->chan; \
> struct lttng_ust_lib_ring_buffer_ctx __ctx; \
> size_t __event_len, __event_align; \
> @@ -612,13 +621,14 @@ static const char * \
> __ref_model_emf_uri___##_provider##___##_name \
> __attribute__((weakref ("_model_emf_uri___" #_provider "___" #_name)));\
> const struct lttng_event_desc __event_desc___##_provider##_##_name = {
> \
> - .fields = __event_fields___##_provider##___##_template, \
> .name = #_provider ":" #_name, \
> .probe_callback = (void (*)(void))
> &__event_probe__##_provider##___##_template,\
> + .ctx = NULL, \
> + .fields = __event_fields___##_provider##___##_template, \
> .nr_fields =
> _TP_ARRAY_SIZE(__event_fields___##_provider##___##_template), \
> .loglevel = &__ref_loglevel___##_provider##___##_name, \
> .signature = __tp_event_signature___##_provider##___##_template, \
> - .u.ext.model_emf_uri = &__ref_model_emf_uri___##_provider##___##_name, \
> + .u = { .ext = { .model_emf_uri =
> &__ref_model_emf_uri___##_provider##___##_name } }, \
> };
>
> #include TRACEPOINT_INCLUDE
> diff --git a/tests/hello.cxx/Makefile.am b/tests/hello.cxx/Makefile.am
> index 897416d..5f6c615 100644
> --- a/tests/hello.cxx/Makefile.am
> +++ b/tests/hello.cxx/Makefile.am
> @@ -1,7 +1,7 @@
> AM_CPPFLAGS = -I$(top_srcdir)/include -I$(top_builddir)/include
> -Wsystem-headers
>
> noinst_PROGRAMS = hello
> -hello_SOURCES = hello.cpp tp.c ust_tests_hello.h
> +hello_SOURCES = hello.cpp tp.cpp ust_tests_hello.h
> hello_LDADD = $(top_builddir)/liblttng-ust/liblttng-ust.la
>
> if LTTNG_UST_BUILD_WITH_LIBDL
> diff --git a/tests/hello.cxx/tp.c b/tests/hello.cxx/tp.c
> deleted file mode 100644
> index 4790965..0000000
> --- a/tests/hello.cxx/tp.c
> +++ /dev/null
> @@ -1,26 +0,0 @@
> -/*
> - * tp.c
> - *
> - * Copyright (c) 2011 Mathieu Desnoyers <mathieu.desnoyers at efficios.com>
> - *
> - * Permission is hereby granted, free of charge, to any person obtaining a
> copy
> - * of this software and associated documentation files (the "Software"),
> to deal
> - * in the Software without restriction, including without limitation the
> rights
> - * to use, copy, modify, merge, publish, distribute, sublicense, and/or
> sell
> - * copies of the Software, and to permit persons to whom the Software is
> - * furnished to do so, subject to the following conditions:
> - *
> - * The above copyright notice and this permission notice shall be included
> in
> - * all copies or substantial portions of the Software.
> - *
> - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
> OR
> - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> - * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> THE
> - * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> - * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> FROM,
> - * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> IN THE
> - * SOFTWARE.
> - */
> -
> -#define TRACEPOINT_CREATE_PROBES
> -#include "ust_tests_hello.h"
> diff --git a/tests/hello.cxx/tp.cpp b/tests/hello.cxx/tp.cpp
> new file mode 100644
> index 0000000..a2099ab
> --- /dev/null
> +++ b/tests/hello.cxx/tp.cpp
> @@ -0,0 +1,26 @@
> +/*
> + * tp.cpp
> + *
> + * Copyright (c) 2011 Mathieu Desnoyers <mathieu.desnoyers at efficios.com>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> copy
> + * of this software and associated documentation files (the "Software"),
> to deal
> + * in the Software without restriction, including without limitation the
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or
> sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included
> in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
> OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> IN THE
> + * SOFTWARE.
> + */
> +
> +#define TRACEPOINT_CREATE_PROBES
> +#include "ust_tests_hello.h"
> --
> 1.8.2.1
>
> --
> Best Regards,
> 仝子飞 (Zifei Tong)
> College of Computer Science and Technology, Zhejiang University
>
> soariez at gmail.com / tongzifei at zju.edu.cn
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
More information about the lttng-dev
mailing list