From 1fba7c7b9ff8f36fde916dfb2317855549f0eb5b Mon Sep 17 00:00:00 2001 From: Philippe Proulx Date: Fri, 5 Jul 2019 18:28:56 -0400 Subject: [PATCH] Fix: bt_ctfser_write_float64(): use `double` in union, not `float` This bug made both CTF writer and `sink.ctf.fs` write wrong 64-bit floating point fields. This patch also adds two `sink.ctf.fs` tests to catch this. Both tests run a CTF generator which uses CTF writer, then run babeltrace2 /path/to/generated/trace -o ctf -w /path/to/converted/trace and then pass the converted trace into a `sink.text.details` sink to verify that the content is expected. Signed-off-by: Philippe Proulx Change-Id: If69ee9a0e1d4038dd28a72860ecc11f18bb6b50e Reviewed-on: https://review.lttng.org/c/babeltrace/+/1645 Tested-by: jenkins Reviewed-by: Simon Marchi --- .gitignore | 2 + configure.ac | 2 + src/ctfser/ctfser.h | 4 +- tests/Makefile.am | 3 +- .../sink.ctf.fs/succeed/trace-double.expect | 48 +++++++ .../sink.ctf.fs/succeed/trace-float.expect | 48 +++++++ tests/plugins/Makefile.am | 1 + tests/plugins/sink.ctf.fs/Makefile.am | 1 + tests/plugins/sink.ctf.fs/succeed/Makefile.am | 13 ++ .../sink.ctf.fs/succeed/gen-trace-double.c | 131 ++++++++++++++++++ .../sink.ctf.fs/succeed/gen-trace-float.c | 131 ++++++++++++++++++ .../plugins/sink.ctf.fs/succeed/test_succeed | 77 ++++++++++ 12 files changed, 458 insertions(+), 3 deletions(-) create mode 100644 tests/data/plugins/sink.ctf.fs/succeed/trace-double.expect create mode 100644 tests/data/plugins/sink.ctf.fs/succeed/trace-float.expect create mode 100644 tests/plugins/sink.ctf.fs/Makefile.am create mode 100644 tests/plugins/sink.ctf.fs/succeed/Makefile.am create mode 100644 tests/plugins/sink.ctf.fs/succeed/gen-trace-double.c create mode 100644 tests/plugins/sink.ctf.fs/succeed/gen-trace-float.c create mode 100755 tests/plugins/sink.ctf.fs/succeed/test_succeed diff --git a/.gitignore b/.gitignore index e302428e..a00cd499 100644 --- a/.gitignore +++ b/.gitignore @@ -7,6 +7,8 @@ /tests/plugins/flt.lttng-utils.debug-info/test_bin_info /tests/plugins/flt.lttng-utils.debug-info/test_dwarf /tests/plugins/src.ctf.fs/succeed/gen-trace-simple +/tests/plugins/sink.ctf.fs/succeed/gen-trace-float +/tests/plugins/sink.ctf.fs/succeed/gen-trace-double *~ *.o *.a diff --git a/configure.ac b/configure.ac index 4a81e5c1..35bf4238 100644 --- a/configure.ac +++ b/configure.ac @@ -778,6 +778,8 @@ AC_CONFIG_FILES([ tests/plugins/Makefile tests/plugins/src.ctf.fs/Makefile tests/plugins/src.ctf.fs/succeed/Makefile + tests/plugins/sink.ctf.fs/Makefile + tests/plugins/sink.ctf.fs/succeed/Makefile tests/plugins/flt.lttng-utils.debug-info/Makefile tests/utils/Makefile tests/utils/tap/Makefile diff --git a/src/ctfser/ctfser.h b/src/ctfser/ctfser.h index f8bbfae2..cf9941bc 100644 --- a/src/ctfser/ctfser.h +++ b/src/ctfser/ctfser.h @@ -507,10 +507,10 @@ int bt_ctfser_write_float64(struct bt_ctfser *ctfser, double value, { union u64f { uint64_t u; - float f; + double d; } u64f; - u64f.f = value; + u64f.d = value; return bt_ctfser_write_unsigned_int(ctfser, u64f.u, alignment_bits, 64, byte_order); } diff --git a/tests/Makefile.am b/tests/Makefile.am index 5ffd0e0c..36b0ab93 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -68,7 +68,8 @@ TESTS_LIB += lib/test_plugin endif TESTS_PLUGINS = \ - plugins/src.ctf.fs/succeed/test_succeed + plugins/src.ctf.fs/succeed/test_succeed \ + plugins/sink.ctf.fs/succeed/test_succeed if !ENABLE_BUILT_IN_PLUGINS if ENABLE_PYTHON_BINDINGS diff --git a/tests/data/plugins/sink.ctf.fs/succeed/trace-double.expect b/tests/data/plugins/sink.ctf.fs/succeed/trace-double.expect new file mode 100644 index 00000000..3db72bb2 --- /dev/null +++ b/tests/data/plugins/sink.ctf.fs/succeed/trace-double.expect @@ -0,0 +1,48 @@ +Trace class: + Stream class (ID 0): + Packets have beginning default clock snapshot: Yes + Packets have end default clock snapshot: Yes + Supports discarded events: Yes + Discarded events have default clock snapshots: Yes + Supports discarded packets: Yes + Discarded packets have default clock snapshots: Yes + Default clock class: + Name: _default + Frequency (Hz): 1,000,000,000 + Precision (cycles): 1 + Offset (s): 0 + Offset (cycles): 0 + Origin is Unix epoch: No + Event class `ev` (ID 0): + Payload field class: Structure (1 member): + dbl: Real (Double precision) + +{Trace 0, Stream class ID 0, Stream ID 0} +Stream beginning: + Trace: + Stream (ID 0, Class ID 0) + +[Unknown] +{Trace 0, Stream class ID 0, Stream ID 0} +Stream activity beginning + +[0 cycles, 0 ns from origin] +{Trace 0, Stream class ID 0, Stream ID 0} +Packet beginning: + +[0 cycles, 0 ns from origin] +{Trace 0, Stream class ID 0, Stream ID 0} +Event `ev` (Class ID 0): + Payload: + dbl: 17283.388100 + +[0 cycles, 0 ns from origin] +{Trace 0, Stream class ID 0, Stream ID 0} +Packet end + +[Unknown] +{Trace 0, Stream class ID 0, Stream ID 0} +Stream activity end + +{Trace 0, Stream class ID 0, Stream ID 0} +Stream end diff --git a/tests/data/plugins/sink.ctf.fs/succeed/trace-float.expect b/tests/data/plugins/sink.ctf.fs/succeed/trace-float.expect new file mode 100644 index 00000000..33f8f18c --- /dev/null +++ b/tests/data/plugins/sink.ctf.fs/succeed/trace-float.expect @@ -0,0 +1,48 @@ +Trace class: + Stream class (ID 0): + Packets have beginning default clock snapshot: Yes + Packets have end default clock snapshot: Yes + Supports discarded events: Yes + Discarded events have default clock snapshots: Yes + Supports discarded packets: Yes + Discarded packets have default clock snapshots: Yes + Default clock class: + Name: _default + Frequency (Hz): 1,000,000,000 + Precision (cycles): 1 + Offset (s): 0 + Offset (cycles): 0 + Origin is Unix epoch: No + Event class `ev` (ID 0): + Payload field class: Structure (1 member): + flt: Real (Single precision) + +{Trace 0, Stream class ID 0, Stream ID 0} +Stream beginning: + Trace: + Stream (ID 0, Class ID 0) + +[Unknown] +{Trace 0, Stream class ID 0, Stream ID 0} +Stream activity beginning + +[0 cycles, 0 ns from origin] +{Trace 0, Stream class ID 0, Stream ID 0} +Packet beginning: + +[0 cycles, 0 ns from origin] +{Trace 0, Stream class ID 0, Stream ID 0} +Event `ev` (Class ID 0): + Payload: + flt: -28.475100 + +[0 cycles, 0 ns from origin] +{Trace 0, Stream class ID 0, Stream ID 0} +Packet end + +[Unknown] +{Trace 0, Stream class ID 0, Stream ID 0} +Stream activity end + +{Trace 0, Stream class ID 0, Stream ID 0} +Stream end diff --git a/tests/plugins/Makefile.am b/tests/plugins/Makefile.am index a44f0695..073766f1 100644 --- a/tests/plugins/Makefile.am +++ b/tests/plugins/Makefile.am @@ -1,3 +1,4 @@ SUBDIRS = \ src.ctf.fs \ + sink.ctf.fs \ flt.lttng-utils.debug-info diff --git a/tests/plugins/sink.ctf.fs/Makefile.am b/tests/plugins/sink.ctf.fs/Makefile.am new file mode 100644 index 00000000..7daaeedb --- /dev/null +++ b/tests/plugins/sink.ctf.fs/Makefile.am @@ -0,0 +1 @@ +SUBDIRS = succeed diff --git a/tests/plugins/sink.ctf.fs/succeed/Makefile.am b/tests/plugins/sink.ctf.fs/succeed/Makefile.am new file mode 100644 index 00000000..4663157a --- /dev/null +++ b/tests/plugins/sink.ctf.fs/succeed/Makefile.am @@ -0,0 +1,13 @@ +dist_check_SCRIPTS = test_succeed + +# CTF trace generators +GEN_TRACE_LDADD = $(top_builddir)/src/ctf-writer/libbabeltrace2-ctf-writer.la + +gen_trace_float_SOURCES = gen-trace-float.c +gen_trace_float_LDADD = $(GEN_TRACE_LDADD) +gen_trace_double_SOURCES = gen-trace-double.c +gen_trace_double_LDADD = $(GEN_TRACE_LDADD) + +noinst_PROGRAMS = \ + gen-trace-float \ + gen-trace-double diff --git a/tests/plugins/sink.ctf.fs/succeed/gen-trace-double.c b/tests/plugins/sink.ctf.fs/succeed/gen-trace-double.c new file mode 100644 index 00000000..3c187ee3 --- /dev/null +++ b/tests/plugins/sink.ctf.fs/succeed/gen-trace-double.c @@ -0,0 +1,131 @@ +/* + * Copyright 2019 Philippe Proulx + * + * 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. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "common/assert.h" + +struct config { + struct bt_ctf_writer *writer; + struct bt_ctf_trace *trace; + struct bt_ctf_clock *clock; + struct bt_ctf_stream_class *sc; + struct bt_ctf_stream *stream; + struct bt_ctf_event_class *ec; +}; + +static +void fini_config(struct config *cfg) +{ + bt_ctf_object_put_ref(cfg->stream); + bt_ctf_object_put_ref(cfg->sc); + bt_ctf_object_put_ref(cfg->ec); + bt_ctf_object_put_ref(cfg->clock); + bt_ctf_object_put_ref(cfg->trace); + bt_ctf_object_put_ref(cfg->writer); +} + +static +void configure_writer(struct config *cfg, const char *path) +{ + struct bt_ctf_field_type *ft; + int ret; + + cfg->writer = bt_ctf_writer_create(path); + BT_ASSERT(cfg->writer); + cfg->trace = bt_ctf_writer_get_trace(cfg->writer); + BT_ASSERT(cfg->trace); + cfg->clock = bt_ctf_clock_create("default"); + BT_ASSERT(cfg->clock); + ret = bt_ctf_writer_add_clock(cfg->writer, cfg->clock); + BT_ASSERT(ret == 0); + ret = bt_ctf_writer_set_byte_order(cfg->writer, + BT_CTF_BYTE_ORDER_BIG_ENDIAN); + BT_ASSERT(ret == 0); + cfg->sc = bt_ctf_stream_class_create("hello"); + BT_ASSERT(cfg->sc); + ret = bt_ctf_stream_class_set_clock(cfg->sc, cfg->clock); + BT_ASSERT(ret == 0); + cfg->ec = bt_ctf_event_class_create("ev"); + BT_ASSERT(cfg->ec); + ft = bt_ctf_field_type_floating_point_create(); + BT_ASSERT(ft); + ret = bt_ctf_field_type_floating_point_set_exponent_digits(ft, 11); + BT_ASSERT(ret == 0); + ret = bt_ctf_field_type_floating_point_set_mantissa_digits(ft, 53); + BT_ASSERT(ret == 0); + ret = bt_ctf_event_class_add_field(cfg->ec, ft, "dbl"); + BT_ASSERT(ret == 0); + bt_ctf_object_put_ref(ft); + ret = bt_ctf_stream_class_add_event_class(cfg->sc, cfg->ec); + BT_ASSERT(ret == 0); + cfg->stream = bt_ctf_writer_create_stream(cfg->writer, cfg->sc); + BT_ASSERT(cfg->stream); +} + +static +void write_stream(struct config *cfg) +{ + struct bt_ctf_event *ev; + struct bt_ctf_field *field; + int ret; + + /* Create event and fill fields */ + ev = bt_ctf_event_create(cfg->ec); + BT_ASSERT(ev); + field = bt_ctf_event_get_payload(ev, "dbl"); + BT_ASSERT(field); + ret = bt_ctf_field_floating_point_set_value(field, 17283.3881); + BT_ASSERT(ret == 0); + bt_ctf_object_put_ref(field); + ret = bt_ctf_clock_set_time(cfg->clock, 0); + BT_ASSERT(ret == 0); + + /* Append event */ + ret = bt_ctf_stream_append_event(cfg->stream, ev); + BT_ASSERT(ret == 0); + bt_ctf_object_put_ref(ev); + + /* Create packet */ + ret = bt_ctf_stream_flush(cfg->stream); + BT_ASSERT(ret == 0); +} + +int main(int argc, char **argv) +{ + struct config cfg = {0}; + + BT_ASSERT(argc >= 2); + configure_writer(&cfg, argv[1]); + write_stream(&cfg); + fini_config(&cfg); + return 0; +} diff --git a/tests/plugins/sink.ctf.fs/succeed/gen-trace-float.c b/tests/plugins/sink.ctf.fs/succeed/gen-trace-float.c new file mode 100644 index 00000000..34444b2e --- /dev/null +++ b/tests/plugins/sink.ctf.fs/succeed/gen-trace-float.c @@ -0,0 +1,131 @@ +/* + * Copyright 2019 Philippe Proulx + * + * 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. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "common/assert.h" + +struct config { + struct bt_ctf_writer *writer; + struct bt_ctf_trace *trace; + struct bt_ctf_clock *clock; + struct bt_ctf_stream_class *sc; + struct bt_ctf_stream *stream; + struct bt_ctf_event_class *ec; +}; + +static +void fini_config(struct config *cfg) +{ + bt_ctf_object_put_ref(cfg->stream); + bt_ctf_object_put_ref(cfg->sc); + bt_ctf_object_put_ref(cfg->ec); + bt_ctf_object_put_ref(cfg->clock); + bt_ctf_object_put_ref(cfg->trace); + bt_ctf_object_put_ref(cfg->writer); +} + +static +void configure_writer(struct config *cfg, const char *path) +{ + struct bt_ctf_field_type *ft; + int ret; + + cfg->writer = bt_ctf_writer_create(path); + BT_ASSERT(cfg->writer); + cfg->trace = bt_ctf_writer_get_trace(cfg->writer); + BT_ASSERT(cfg->trace); + cfg->clock = bt_ctf_clock_create("default"); + BT_ASSERT(cfg->clock); + ret = bt_ctf_writer_add_clock(cfg->writer, cfg->clock); + BT_ASSERT(ret == 0); + ret = bt_ctf_writer_set_byte_order(cfg->writer, + BT_CTF_BYTE_ORDER_BIG_ENDIAN); + BT_ASSERT(ret == 0); + cfg->sc = bt_ctf_stream_class_create("hello"); + BT_ASSERT(cfg->sc); + ret = bt_ctf_stream_class_set_clock(cfg->sc, cfg->clock); + BT_ASSERT(ret == 0); + cfg->ec = bt_ctf_event_class_create("ev"); + BT_ASSERT(cfg->ec); + ft = bt_ctf_field_type_floating_point_create(); + BT_ASSERT(ft); + ret = bt_ctf_field_type_floating_point_set_exponent_digits(ft, 8); + BT_ASSERT(ret == 0); + ret = bt_ctf_field_type_floating_point_set_mantissa_digits(ft, 24); + BT_ASSERT(ret == 0); + ret = bt_ctf_event_class_add_field(cfg->ec, ft, "flt"); + BT_ASSERT(ret == 0); + bt_ctf_object_put_ref(ft); + ret = bt_ctf_stream_class_add_event_class(cfg->sc, cfg->ec); + BT_ASSERT(ret == 0); + cfg->stream = bt_ctf_writer_create_stream(cfg->writer, cfg->sc); + BT_ASSERT(cfg->stream); +} + +static +void write_stream(struct config *cfg) +{ + struct bt_ctf_event *ev; + struct bt_ctf_field *field; + int ret; + + /* Create event and fill fields */ + ev = bt_ctf_event_create(cfg->ec); + BT_ASSERT(ev); + field = bt_ctf_event_get_payload(ev, "flt"); + BT_ASSERT(field); + ret = bt_ctf_field_floating_point_set_value(field, -28.4751); + BT_ASSERT(ret == 0); + bt_ctf_object_put_ref(field); + ret = bt_ctf_clock_set_time(cfg->clock, 0); + BT_ASSERT(ret == 0); + + /* Append event */ + ret = bt_ctf_stream_append_event(cfg->stream, ev); + BT_ASSERT(ret == 0); + bt_ctf_object_put_ref(ev); + + /* Create packet */ + ret = bt_ctf_stream_flush(cfg->stream); + BT_ASSERT(ret == 0); +} + +int main(int argc, char **argv) +{ + struct config cfg = {0}; + + BT_ASSERT(argc >= 2); + configure_writer(&cfg, argv[1]); + write_stream(&cfg); + fini_config(&cfg); + return 0; +} diff --git a/tests/plugins/sink.ctf.fs/succeed/test_succeed b/tests/plugins/sink.ctf.fs/succeed/test_succeed new file mode 100755 index 00000000..c04522ac --- /dev/null +++ b/tests/plugins/sink.ctf.fs/succeed/test_succeed @@ -0,0 +1,77 @@ +#!/bin/bash +# +# Copyright (C) 2019 Philippe Proulx +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; only version 2 +# of the License. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. + +# This test validates that a `src.ctf.fs` component successfully reads +# specific CTF traces and creates the expected messages. +# +# Such CTF traces to open either exist (in `tests/ctf-traces/succeed`) +# or are generated by this test using local trace generators. + +SH_TAP=1 + +if [ "x${BT_TESTS_SRCDIR:-}" != "x" ]; then + UTILSSH="$BT_TESTS_SRCDIR/utils/utils.sh" +else + UTILSSH="$(dirname "$0")/../../../utils/utils.sh" +fi + +# shellcheck source=../../../utils/utils.sh +source "$UTILSSH" + +this_dir_relative="plugins/sink.ctf.fs/succeed" +this_dir_build="$BT_TESTS_BUILDDIR/$this_dir_relative" +expect_dir="$BT_TESTS_DATADIR/$this_dir_relative" + +test_ctf_gen_single() { + name="$1" + temp_gen_trace_dir="$(mktemp -d)" + temp_out_trace_dir="$(mktemp -d)" + + diag "Generating trace '$name'" + + if ! "$this_dir_build/gen-trace-$name" "$temp_gen_trace_dir"; then + # this is not part of the test itself; it must not fail + echo "ERROR: \"$this_dir_build/gen-trace-$name" "$temp_gen_trace_dir\" failed" >&2 + rm -rf "$temp_gen_trace_dir" + rm -rf "$temp_out_trace_dir" + exit 1 + fi + + diag "Converting trace '$name' to CTF through 'sink.ctf.fs'" + "$BT_TESTS_BT2_BIN" >/dev/null "$temp_gen_trace_dir" -o ctf -w "$temp_out_trace_dir" + ret=$? + ok $ret "'sink.ctf.fs' component succeeds with input trace '$name'" + converted_test_name="Converted trace '$name' gives the expected output" + + if [ $ret -eq 0 ]; then + bt_diff_details_ctf_single "$temp_out_trace_dir" \ + "$expect_dir/trace-$name.expect" \ + '-p with-uuid=no,with-trace-name=no,with-stream-name=no' + ok $? "$converted_test_name" + else + fail "$converted_test_name" + fi + + rm -rf "$temp_gen_trace_dir" + rm -rf "$temp_out_trace_dir" +} + +plan_tests 4 + +test_ctf_gen_single float +test_ctf_gen_single double -- 2.34.1