Clean-up: run format-cpp on the tree The original re-format commit missed a number of files that were caught by format-cpp. Hopefully this is the last large reformat commit for a while. Change-Id: I493ee6d9fe6187e0bd087c68ed346af69c929c1e Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
clang-tidy: add Chrome-inspired checks Add the checks used by the Chrome project. Most of these changes were produced and applied by clang-tidy. Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com> Change-Id: I4058dafbdef00e81ac04f202fdfe377384a99e6b
Fix: sessiond: size-based rotations never trigger Issue observed ============== Size-based scheduled rotations have no effect. Cause ===== Since c08136a3f, the rotation thread's handle_condition() checks that the notification received matches the trigger that was registered. As part of the equality check, the triggers' credentials are compared. This checks fails systematically since the group id of a trigger's credentials is not transported by the serialize/create_from functions. The trigger that is received through the notification thus has an unset group id, while the rotation trigger of the `ltt_session` has a group id set; it was not stripped by the communication layer. The check also fails since the trigger registered for the size-based rotation is "hidden". This internal attribute is not propagated through the communication layer, which causes the comparison to fail. Solution ======== Since triggers only use the 'uid' part of lttng_credentials, we ensure that lttng_trigger_set_credentials only sets this part of the structure. Also, the `is_hidden` attribute of a trigger is now propagated through the communication layer. This has no effect for external applications since this attribute is not exposed through the API. However, it is useful for internal triggers which use the same communication facilities. This allows the equality check in rotation-thread.cpp to go through as expected. Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com> Change-Id: I216f5cb9297ecd1a867dc292c10b8da595efce34
Rename C++ header files to .hpp Rename all C++ header files (include/**/*-internal.h, src/**/*.h except argpar and msgpack, some headers in tests) to have the .hpp extension. Doing so highlights that we include some C++ header files in some test files still compiled as C. This is ok for now, as the files they include don't actually contain C++ code incompatible with C yet, but they could eventually. This is something we can fix later. Change-Id: I8bf326b6b2946a3e26704f3ef3ac5831bbe9bc26 Signed-off-by: Simon Marchi <simon.marchi@efficios.com> Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
Remove extern "C" from internal headers All internal code is now compiled as C++, we can now remove all 'extern "C"' declarations from internal headers. This means files will see each other's declarations as C++, and we can now use things in headers. Remove the min/min_t/max/max_t macros from macros.h as well. Change-Id: I5a6b7ef60be5f46160c6d5ca39f082d2137d5a07 Signed-off-by: Simon Marchi <simon.marchi@efficios.com> Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
lib: compile liblttng-ctl as C++ Same as the previous commits, but compile the liblttng-ctl library as C++ code (while still offering a C interface). Some exported global variables (for example in deprecated-symbols.cpp) have to be made non-const, otherwise we get: CXX deprecated-symbols.lo /home/simark/src/lttng-tools/src/lib/lttng-ctl/deprecated-symbols.cpp:21:33: error: ‘visibility’ attribute ignored [-Werror=attributes] 21 | LTTNG_EXPORT const char * const config_element_pid_tracker; | ^~~~~~~~~~~~~~~~~~~~~~~~~~ I think this is related to the fact that const global variables automatically have internal linkage in C++. Despite using -export-symbols in src/lib/lttng-ctl/Makefile.am, some new ELF symbols become exposed. It could be related to this, in the ld man page: --retain-symbols-file does not discard undefined symbols, or symbols needed for relocations. One new symbol I see, for example, is `_Z16connect_sessiondv`. Looking at liblttng-ctl.so, I indeed see a relocation for this symbol: 000000000010b778 0000053e00000007 R_X86_64_JUMP_SLOT 00000000000314a2 _Z16connect_sessiondv + 0 And that would explain why the linker keeps that symbol visible. This is related to the entry for this function in the procedure linkage table. I'm not entirely sure why these functions didn't generate a PLT entry in C, but do in C++. To avoid these new symbols, build everything with -fvisibility=hidden and -fvisibility-inlines-hidden, and tag functions we really want to export with LTTNG_EXPORT, a new macro defined to __attribute__((visibility("default"))). This macro is publicly visible, because it has to be used in distributed header files (although it's of no use for users of liblttng-ctl). Change-Id: Ie51bf0a2edfb87e5f46f9c39eed5309d9f8c41d6 Signed-off-by: Simon Marchi <simon.marchi@efficios.com> Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
bin: compile lttng-sessiond as C++ Same as commit 48a400056134 ("bin: compile lttng as C++"), but change lttng-sessiond to be a C++ program. In addition to the categories of changes already mentioned in that commit's message, here are some interesting changes: - Add an include in trigger.h, an exported header, to fix: CXX notification-thread.lo In file included from /home/simark/src/lttng-tools/src/bin/lttng-sessiond/notification-thread.cpp:9: /home/simark/src/lttng-tools/include/lttng/trigger/trigger.h:142:13: error: use of enum ‘lttng_error_code’ without previous declaration 142 | extern enum lttng_error_code lttng_register_trigger_with_name( | ^~~~~~~~~~~~~~~~ - We get this with clang: CXX lttng-conf.o In file included from /home/simark/src/lttng-tools/src/bin/lttng/conf.cpp:18: In file included from /home/simark/src/lttng-tools/src/common/common.h:14: In file included from /home/simark/src/lttng-tools/src/common/runas.h:17: In file included from /home/simark/src/lttng-tools/src/common/sessiond-comm/sessiond-comm.h:38: In file included from /home/simark/src/lttng-tools/src/common/unix.h:17: /home/simark/src/lttng-tools/src/common/payload-view.h:82:27: error: 'lttng_payload_view_from_payload' has C-linkage specified, but returns user-defined type 'struct lttng_payload_view' which is incompatible with C [-Werror,-Wreturn-type-c-linkage] struct lttng_payload_view lttng_payload_view_from_payload( ^ Turns out that because of the "const" field in lttng_payload_view, clang doesn't consider that type incompatible with C. I don't really want to remove the "const" for C code using that API, so conditionally remove it if we are compiling with clang in C++. - clang gives: CXX event.lo In file included from /home/simark/src/lttng-tools/src/bin/lttng-sessiond/event.cpp:19: /home/simark/src/lttng-tools/src/common/bytecode/bytecode.h:50:1: error: struct has size 0 in C, size 1 in C++ [-Werror,-Wextern-c-compat] struct literal_string { ^ It looks like that type isn't even used? Remove it. - it's not possible to initialize some union members, for example with lttcomm_consumer_msg, in consumer.cpp. Initialize it in a separate statement. - It's not possible to use the transparent union trick when calling urcu function, for example in thread_application_registration, in register.cpp. We need to instantiate a cds_wfcq_head_ptr_t object, assign the appropriate field, and pass that object to the function. - the ALIGNED_CONST_PTR trick does not work in C++: CXX consumer.lo In file included from /home/simark/src/lttng-tools/src/common/error.h:19, from /home/simark/src/lttng-tools/src/common/common.h:12, from /home/simark/src/lttng-tools/src/bin/lttng-sessiond/consumer.cpp:19: /home/simark/src/lttng-tools/src/bin/lttng-sessiond/consumer.cpp: In function ‘int consumer_send_relayd_socket(consumer_socket*, lttcomm_relayd_sock*, consumer_output*, lttng_stream_type, uint64_t, const char*, const char*, const char*, int, const uint64_t*, time_t, bool)’: /home/simark/src/lttng-tools/src/common/macros.h:116:58: error: expected primary-expression before ‘]’ token 116 | #define ALIGNED_CONST_PTR(value) (((const typeof(value) []) { value })) | ^ /home/simark/src/lttng-tools/src/bin/lttng-sessiond/consumer.cpp:1192:48: note: in expansion of macro ‘ALIGNED_CONST_PTR’ 1192 | ret = consumer_send_fds(consumer_sock, ALIGNED_CONST_PTR(rsock->sock.fd), 1); | ^~~~~~~~~~~~~~~~~ Replace uses with copying the data in a local variable (which is properly aligned), and pass the address to that variable to the function. - In consumer.h, an array field in a structure is defined using the max macro. It can't be replaced with std::max, since std::max isn't constexpr in C++11. Define a max_constexpr function locally and use it. - g++ 7 doesn't support non-trivial designated initializers, leading to errors like: CXX globals.lo /home/smarchi/src/lttng-tools/src/bin/lttng-sessiond/globals.cpp:44:1: sorry, unimplemented: non-trivial designated initializers not supported }; ^ Change consumer_data to have a constructor instead. Change initializations of some structures, such as lttcomm_lttng_msg, to initialize the fields separate from the variable declaration. This requires making these variable non-const which is not ideal. But once everything is C++, these types could get a fancy constructor, and then they can be made const again. - When compiling without UST support the stub versions of functions ust_app_rotate_session & co, in ust-app.h, are used. Some of them have the return type "enum lttng_error_code", but return 0, an invalid value, causing: CXX main.o In file included from /home/smarchi/src/lttng-tools/src/bin/lttng-sessiond/lttng-sessiond.h:22:0, from /home/smarchi/src/lttng-tools/src/bin/lttng-sessiond/main.cpp:45: /home/smarchi/src/lttng-tools/src/bin/lttng-sessiond/ust-app.h: In function ‘lttng_error_code ust_app_snapshot_record(ltt_ust_session*, const consumer_output*, int, uint64_t)’: /home/smarchi/src/lttng-tools/src/bin/lttng-sessiond/ust-app.h:575:9: error: invalid conversion from ‘int’ to ‘lttng_error_code’ [-fpermissive] return 0; ^ Change these functions to return LTTNG_ERR_UNK. These functions are not supposed to be called if UST support is not included. But even if they were: all their callers check that the return value is not LTTNG_OK. The value 0 would be considered an error, so will be LTTNG_ERR_UNK. Change-Id: I2cdd34459a54b1943087b43843ef20b35b7bf7d8 Signed-off-by: Simon Marchi <simon.marchi@efficios.com> Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
bin: compile lttng as C++ Compile the code of the lttng binary as C++ source. - start by renaming all files under src/bin/lttng to have the .cpp extension, adjust Makefile.am accordingly - apply the sophisticated algorithm: while does_not_build(): fix_error() until completion Fixes fall in these categories: - add extern "C" to headers of functions implemented in C. This is likely temporary: at some point some of these things will be implemented in C++, at which point we'll remove the extern "C". - rename mi_lttng_version to mi_lttng_version_data, to avoid a -Wshadow warning about the mi_lttng_version function hiding the mi_lttng_version's struct constructor - we have the same warning about lttng_calibrate, but we can't rename it, it's exposed in a public header. Add some pragmas to disable the warning around there. We will need more macro smartness in case we need to support a compiler that doesn't understand these pragmas. - in filter-ast.h, add a dummy field to the empty struct, to avoid a -Wextern-c-compat warning with clang++ (it warns us that the struct has size 0 in C but size 1 in C++). - in add_context.cpp, we can't initialize ctx_opts' union field like we did in C. Fix that by adding a ctx_opts constructor for each kind of context and implement the PERF_* macros to use them. - need to explicitly cast void pointer to type of the destination, for example the eturn value of allocation functions, or parameter of "destroy" functions - need to explicitly cast when passing an int to an enum parameter, for example an lttng_error_code parameter - remove use of designated array initializers, for example for schedule_type_str in disable_rotation.cpp - fix order of struct initializers to match order of field declarations, for example in list_triggers.cpp, function cmd_list_triggers - rename some things to avoid clashing with keywords, for example in runas.h Change-Id: Id743b141552a412b4104af4dda8969eef5032388 Signed-off-by: Simon Marchi <simon.marchi@efficios.com> Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
liblttng-ctl: use export list to define exported symbols Symbols are currently exported by default by liblttng-ctl.so (usable by other shared libraries / programs using liblttng-ctl.so), so we must use LTTNG_HIDDEN on all symbols that are meant to be internal to liblttng-ctl.so. Of course, this is easy to forget, so over the years many symbols that were not meant to be exported were exported, and must now stay exported to avoid breaking the ABI. As explained here [1], a better method is to make symbols hidden by default, and mark those we want to be exported as such. I have tried to use this, but when subsequently converting the code to C++, I have noticed that some symbols related to the STL were exported anyway, which is bad. The other alternative, implemented in this patch, is to use an explicit symbol export list [2], using libtool's -export-symbols (which uses the linker's -version-script option). Only the symbols listed here are exported. So, in practice, this patch: - Adds an liblttng-ctl.sym file with the list of exported symbols and adjusts the Makefile to use the -export-symbol option - Removes LTTNG_HIDDEN and all its uses abidiff shows no changes for liblttng-ctl.so between before and after this patch. [1] https://gcc.gnu.org/wiki/Visibility [2] https://www.gnu.org/software/libtool/manual/libtool.html#Link-mode Change-Id: I5d8c558303894b0ad8113c6e52f79a053bb580e1 Signed-off-by: Simon Marchi <simon.marchi@efficios.com> Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
Fix: sessiond: list-triggers: don't return internal triggers The session daemon uses triggers internally. For instance, the trigger and notification subsystem is used to implement the automatic rotation of sessions based on a size threshold. Currently, a user of the C API will see those internal triggers if it is running as the same user as the session daemon. This can be unexpected by user code that assumes it will be alone in creating triggers. Moreover, it is possible for external users to unregister those triggers which would cause bugs. As the triggers gain more capabilities, it is likely that the session daemon will keep using them to implement features internally. Thus, an internal "is_hidden" property is introduced in lttng_trigger. A "hidden" trigger is a trigger that is not returned by the listings. It is used to hide triggers that are used internally by the session daemon so that they can't be listed nor unregistered by external clients. This is a property that can only be set internally by the session daemon. As such, it is not serialized nor set by a "create_from_buffer" constructor. The hidden property is preserved by copies. Note that notifications originating from an "hidden" trigger will not be sent to clients that are not within the session daemon's process. Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com> Change-Id: I61b7949075172fcd428289e2eb670d03c19bdf71
Add condition-targeting error query Notifications discarded by the tracers are reported at the level of a trigger. As those errors are specific to triggers with an "event-rule matches" condition, they should be reported through a condition-specific error query. Note that a condition error query is created from a trigger: there is no ambiguity since, unlike actions, conditions cannot be nested. Given the proximity of the final 2.13 release, the code which populated trigger error query results is simply used to populate the condition error query results when the condition is of type "event-rule matches". No trigger-scope errors can be reported for the moment. However, such error reports will be added in the future. Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com> Change-Id: Ie1ac3668142041beb6fd61574ccef506707c55b2
Fix: lttng-ctl: assertion failure during unregistration of trigger Issue observed ============== lt-trigger_name: trigger.c:302: int lttng_trigger_serialize(const struct lttng_trigger *, struct lttng_payload *): Assertion `(creds->uid).is_set' failed. Program terminated with signal SIGABRT, Aborted. #0 0x00007fb74129eef5 in raise () from /usr/lib/libc.so.6 #1 0x00007fb741288862 in abort () from /usr/lib/libc.so.6 #2 0x00007fb741288747 in __assert_fail_base.cold () from /usr/lib/libc.so.6 #3 0x00007fb741297646 in __assert_fail () from /usr/lib/libc.so.6 #4 0x00007fb74169bab7 in lttng_trigger_serialize (trigger=0x5616f6f70060, payload=0x7ffe5819d140) at trigger.c:302 #5 0x00007fb74169cef0 in lttng_trigger_copy (trigger=0x5616f6f70060) at trigger.c:859 #6 0x00007fb74164302e in lttng_unregister_trigger (trigger=0x5616f6f70060) at lttng-ctl.c:3350 #7 0x00005616f50c675f in register_named_trigger () at trigger_name.c:295 #8 0x00005616f50c6879 in main (argc=1, argv=0x7ffe581a07d8) at trigger_name.c:343 Cause ===== When creating a trigger instance and using it to unregister an existing trigger, its credentials are unset (meaning 'default'). Expecting this, lttng_unregister_trigger() copies the source trigger to change its credentials to those of the caller. Unfortunately, the trigger copy operation expects credentials to be set. We don't run into this situation typically since the trigger instance used to perform the unregistration is sourced from a listing or is the same instance that was used to perform the registration (which sets the credentials before serializing). Solution ======== A proper implementation of "copy" is provided for the trigger object itself. For its condition and action, we still use the same "trick" of leveraging the serdes code to perform a deep-copy, keeping the change small Drawbacks ========= None really, except that we lose some of the code sharing between copy and serdes. Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com> Change-Id: I71b7b075c959bc4935621543c4d379f62b7dabdf
trigger: keep state of if a trigger is currently registered Since a trigger can be referenced even when is was "unregistered" in other part of lttng-sessiond, namely the action executor queue, we must keep track of the registration state. This will allows us to easily skip any actions to be executed if the associated trigger is "unregistered" at the moment of execution. This is implemented in a following patch. Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com> Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com> Change-Id: I06c9d437fce975c9c8393e8d1be3e66a24618af3
Move firing policy from lttng_trigger to lttng_action After some reflection on the future of the importance of the trigger feature and the centralization we wish to carry around it, it is required that the notion of firing policy be moved from the trigger object to each action object of a trigger. This is necessary since we plan on introducing tracer side actions, such as increment value of map. Controlling the firing policy on the tracer side is not an easy thing to do since for UST tracing a lot a synchronizations must take place and also we must define the behaviour when multiple apps are present. Hence, we need a way to ensure that we are not painting ourself in a corner. The middle ground that was chosen was to move the notion of firing policy to the action object level. This allows us to scope the concept to the action and decide for each type if firing policy can be supported and, as needed, define the behaviour per action type. Essentially this patch perform the complete transition. It removes the notion of firing policy at the trigger level and exposes the firing policy of each action type if applicable. CLI ====== For the `add-trigger` command the change essentially boils down to moving the `--fire-every` and `--fire-once-after` from a top-level parsing to the parsing of each actions. Yes, for now all actions supports the `--fire-*` options but it will not be the case in the future. A side effect of this is that a user could decide to have different firing policy for each actions, but this also mean that if a user want to apply the same firing policy across actions, the user needs to specify it for each actions. This could be solved in the future as the trigger feature matures and that common ground are found in the behaviour or implementation of the actions (tracer side action, async action, sync actions etc.) such that "syntactic sugar" options emerge. As for the `list-trigger`, we move the firing policy printing to each actions. Tests have been updated to reflect the changes. Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com> Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com> Change-Id: Ib29d4319922096c0c4b3f00782f3bbefb17e2f40
event-notifier: implement `lttng_trigger_needs_tracer_notifier()` function This function is used to tell if a trigger needs a tracer notifier. This depends on its condition and its actions. Signed-off-by: Francis Deslauriers <francis.deslauriers@efficios.com> Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com> Change-Id: Ie3c928ba1c705c32e4c2e4c6b6b22aa8c10837a9