[PATCH v2 0/3] Fixes needed befor KS 1.0

[PATCH v2 0/3] Fixes needed befor KS 1.0

[Yordan Karadzhov]

The important one is the one fixing the visualization model
aborting on 32 bit system [PATCH 2].

Changes in v2:
 - [PATCH 2] includes a fix for void ksmodel_jump_to()
 - [PATCH 3] is new. 

Yordan Karadzhov (3):
  kernel-shark: Fix a bug when plotting the last trace record
  kernel-shark: Always use 64 bit variables for timestamps.
  kernel-shark: Fix all warnings when building on 32 bit systems

 kernel-shark/examples/datahisto.c  |  4 ++--
 kernel-shark/src/KsTraceGraph.cpp  | 10 +++++-----
 kernel-shark/src/libkshark-model.c | 27 +++++++++++++++++++--------
 kernel-shark/src/libkshark-model.h |  2 +-
 kernel-shark/src/libkshark.c       |  4 ++--
 5 files changed, 29 insertions(+), 18 deletions(-)

-- 
2.20.1

[PATCH v2 1/3] kernel-shark: Fix a bug when plotting the last trace record

[Yordan Karadzhov]

On a first glance this patch may looks like reverting commit
9336dd6bcd38 (kernel-shark: Fix a bug in ksmodel_set_next_bin_edge())

The point is that for the last bin we want to increment its upper edge
used  when checking if the bin is empty, but we do not want to touch
the lower edge time used by kshark_find_entry_by_time().

Fixes: 9336dd6bcd38 (kernel-shark: Fix a bug in ksmodel_set_next_bin_edge())
Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark/src/libkshark-model.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/kernel-shark/src/libkshark-model.c b/kernel-shark/src/libkshark-model.c
index 978cd70..0cac924 100644
--- a/kernel-shark/src/libkshark-model.c
+++ b/kernel-shark/src/libkshark-model.c
@@ -260,20 +260,30 @@ static size_t ksmodel_set_upper_edge(struct kshark_trace_histo *histo)
 static void ksmodel_set_next_bin_edge(struct kshark_trace_histo *histo,
 				      size_t bin, size_t last_row)
 {
-	size_t time, next_bin = bin + 1;
+	size_t time_min, time_max, next_bin = bin + 1;
 	ssize_t row;
 
-	/* Calculate the beginning of the next bin. */
-	time = histo->min + next_bin * histo->bin_size;
+	/* Calculate the beginning and the end of the next bin. */
+	time_min = histo->min + next_bin * histo->bin_size;
+	time_max = time_min + histo->bin_size;
+	/*
+	 * The timestamp of the very last entry of the dataset can be exactly
+	 * equal to the value of the upper edge of the range. This is very
+	 * likely to happen when we use ksmodel_set_in_range_bining(). In this
+	 * case we have to increase the size of the very last bin in order to
+	 * make sure that the last entry of the dataset will fall into it.
+	 */
+	if (next_bin == histo->n_bins - 1)
+		++time_max;
 
 	/*
 	 * Find the index of the first entry inside
-	 * the next bin (timestamp > time).
+	 * the next bin (timestamp > time_min).
 	 */
-	row = kshark_find_entry_by_time(time, histo->data, last_row,
+	row = kshark_find_entry_by_time(time_min, histo->data, last_row,
 					histo->data_size - 1);
 
-	if (row < 0 || histo->data[row]->ts >= time + histo->bin_size) {
+	if (row < 0 || histo->data[row]->ts >= time_max) {
 		/* The bin is empty. */
 		histo->map[next_bin] = KS_EMPTY_BIN;
 		return;
-- 
2.20.1

[PATCH v2 2/3] kernel-shark: Always use 64 bit variables for timestamps.

[Yordan Karadzhov]

All time stamps of the trace records are coded with 64 bits, however
on some systems the size_t type can be 32 bits.

Reported-by: Alan Mikhak <alanmikhak@gmail.com>
Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark/src/libkshark-model.c | 7 ++++---
 kernel-shark/src/libkshark-model.h | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/kernel-shark/src/libkshark-model.c b/kernel-shark/src/libkshark-model.c
index 0cac924..18f9c69 100644
--- a/kernel-shark/src/libkshark-model.c
+++ b/kernel-shark/src/libkshark-model.c
@@ -260,7 +260,8 @@ static size_t ksmodel_set_upper_edge(struct kshark_trace_histo *histo)
 static void ksmodel_set_next_bin_edge(struct kshark_trace_histo *histo,
 				      size_t bin, size_t last_row)
 {
-	size_t time_min, time_max, next_bin = bin + 1;
+	uint64_t time_min, time_max;
+	size_t next_bin = bin + 1;
 	ssize_t row;
 
 	/* Calculate the beginning and the end of the next bin. */
@@ -601,9 +602,9 @@ void ksmodel_shift_backward(struct kshark_trace_histo *histo, size_t n)
  * @param histo: Input location for the model descriptor.
  * @param ts: position in time to be visualized.
  */
-void ksmodel_jump_to(struct kshark_trace_histo *histo, size_t ts)
+void ksmodel_jump_to(struct kshark_trace_histo *histo, uint64_t ts)
 {
-	size_t min, max, range_min;
+	uint64_t min, max, range_min;
 
 	if (ts > histo->min && ts < histo->max) {
 		/*
diff --git a/kernel-shark/src/libkshark-model.h b/kernel-shark/src/libkshark-model.h
index 95c30b6..47793b1 100644
--- a/kernel-shark/src/libkshark-model.h
+++ b/kernel-shark/src/libkshark-model.h
@@ -89,7 +89,7 @@ void ksmodel_shift_forward(struct kshark_trace_histo *histo, size_t n);
 
 void ksmodel_shift_backward(struct kshark_trace_histo *histo, size_t n);
 
-void ksmodel_jump_to(struct kshark_trace_histo *histo, size_t ts);
+void ksmodel_jump_to(struct kshark_trace_histo *histo, uint64_t ts);
 
 void ksmodel_zoom_out(struct kshark_trace_histo *histo,
 		      double r, int mark);
-- 
2.20.1

[PATCH v2 3/3] kernel-shark: Fix all warnings when building on 32 bit systems

[Yordan Karadzhov]

Fixing warnings coming from:
../examples/datahisto.c
../src/KsTraceGraph.cpp
../src/libkshark.c

Reported-by: Alan Mikhak <alanmikhak@gmail.com>
Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark/examples/datahisto.c |  4 ++--
 kernel-shark/src/KsTraceGraph.cpp | 10 +++++-----
 kernel-shark/src/libkshark.c      |  4 ++--
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/kernel-shark/examples/datahisto.c b/kernel-shark/examples/datahisto.c
index 02c6285..b177b08 100644
--- a/kernel-shark/examples/datahisto.c
+++ b/kernel-shark/examples/datahisto.c
@@ -56,11 +56,11 @@ void dump_bin(struct kshark_trace_histo *histo, int bin,
 		puts ("EMPTY BIN");
 	} else {
 		entry_str = kshark_dump_entry(e_front);
-		printf("%li -> %s\n", i_front, entry_str);
+		printf("%zd -> %s\n", i_front, entry_str);
 		free(entry_str);
 
 		entry_str = kshark_dump_entry(e_back);
-		printf("%li -> %s\n", i_back, entry_str);
+		printf("%zd -> %s\n", i_back, entry_str);
 		free(entry_str);
 	}
 
diff --git a/kernel-shark/src/KsTraceGraph.cpp b/kernel-shark/src/KsTraceGraph.cpp
index da2c6aa..324f36e 100644
--- a/kernel-shark/src/KsTraceGraph.cpp
+++ b/kernel-shark/src/KsTraceGraph.cpp
@@ -277,7 +277,7 @@ void KsTraceGraph::_resetPointer(uint64_t ts, int cpu, int pid)
 	QString pointer;
 
 	kshark_convert_nano(ts, &sec, &usec);
-	pointer.sprintf("%lu.%06lu", sec, usec);
+	pointer.sprintf("%" PRIu64 ".%06" PRIu64 "", sec, usec);
 	_labelP2.setText(pointer);
 
 	if (pid > 0 && cpu >= 0) {
@@ -313,7 +313,7 @@ void KsTraceGraph::_setPointerInfo(size_t i)
 	uint64_t sec, usec;
 
 	kshark_convert_nano(e->ts, &sec, &usec);
-	pointer.sprintf("%lu.%06lu", sec, usec);
+	pointer.sprintf("%" PRIu64 ".%06" PRIu64 "", sec, usec);
 	_labelP2.setText(pointer);
 
 	comm.append("-");
@@ -601,17 +601,17 @@ void KsTraceGraph::_updateTimeLegends()
 	QString tMin, tMid, tMax;
 
 	kshark_convert_nano(_glWindow.model()->histo()->min, &sec, &usec);
-	tMin.sprintf("%lu.%06lu", sec, usec);
+	tMin.sprintf("%" PRIu64 ".%06" PRIu64 "", sec, usec);
 	_labelXMin.setText(tMin);
 
 	tsMid = (_glWindow.model()->histo()->min +
 		 _glWindow.model()->histo()->max) / 2;
 	kshark_convert_nano(tsMid, &sec, &usec);
-	tMid.sprintf("%lu.%06lu", sec, usec);
+	tMid.sprintf("%" PRIu64 ".%06" PRIu64 "", sec, usec);
 	_labelXMid.setText(tMid);
 
 	kshark_convert_nano(_glWindow.model()->histo()->max, &sec, &usec);
-	tMax.sprintf("%lu.%06lu", sec, usec);
+	tMax.sprintf("%" PRIu64 ".%06" PRIu64 "", sec, usec);
 	_labelXMax.setText(tMax);
 }
 
diff --git a/kernel-shark/src/libkshark.c b/kernel-shark/src/libkshark.c
index 0f0a1ba..9aaf1b7 100644
--- a/kernel-shark/src/libkshark.c
+++ b/kernel-shark/src/libkshark.c
@@ -1423,7 +1423,7 @@ char* kshark_dump_custom_entry(struct kshark_context *kshark_ctx,
 	event_name = info_func(kshark_ctx, entry, false);
 	info = info_func(kshark_ctx, entry, true);
 
-	size = asprintf(&entry_str, "%li; %s-%i; CPU %i; ; %s; %s",
+	size = asprintf(&entry_str, "%" PRIu64 "; %s-%i; CPU %i; ; %s; %s",
 			entry->ts,
 			task,
 			entry->pid,
@@ -1472,7 +1472,7 @@ char* kshark_dump_entry(const struct kshark_entry *entry)
 		event_name = event? event->name : "[UNKNOWN EVENT]";
 		lat = kshark_get_latency(kshark_ctx->pevent, data);
 
-		size = asprintf(&temp_str, "%li; %s-%i; CPU %i; %s;",
+		size = asprintf(&temp_str, "%" PRIu64 "; %s-%i; CPU %i; %s;",
 				entry->ts,
 				task,
 				entry->pid,
-- 
2.20.1

Re: [PATCH v2 3/3] kernel-shark: Fix all warnings when building on 32 bit systems

[Alan Mikhak]

On Fri, Jun 14, 2019 at 6:51 AM Yordan Karadzhov <ykaradzhov@vmware.com> wrote:
>
> Fixing warnings coming from:
> ../examples/datahisto.c
> ../src/KsTraceGraph.cpp
> ../src/libkshark.c
>
> Reported-by: Alan Mikhak <alanmikhak@gmail.com>
> Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
> ---
>  kernel-shark/examples/datahisto.c |  4 ++--
>  kernel-shark/src/KsTraceGraph.cpp | 10 +++++-----
>  kernel-shark/src/libkshark.c      |  4 ++--
>  3 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/kernel-shark/examples/datahisto.c b/kernel-shark/examples/datahisto.c
> index 02c6285..b177b08 100644
> --- a/kernel-shark/examples/datahisto.c
> +++ b/kernel-shark/examples/datahisto.c
> @@ -56,11 +56,11 @@ void dump_bin(struct kshark_trace_histo *histo, int bin,
>                 puts ("EMPTY BIN");
>         } else {
>                 entry_str = kshark_dump_entry(e_front);
> -               printf("%li -> %s\n", i_front, entry_str);
> +               printf("%zd -> %s\n", i_front, entry_str);
>                 free(entry_str);
>
>                 entry_str = kshark_dump_entry(e_back);
> -               printf("%li -> %s\n", i_back, entry_str);
> +               printf("%zd -> %s\n", i_back, entry_str);
>                 free(entry_str);
>         }
>
> diff --git a/kernel-shark/src/KsTraceGraph.cpp b/kernel-shark/src/KsTraceGraph.cpp
> index da2c6aa..324f36e 100644
> --- a/kernel-shark/src/KsTraceGraph.cpp
> +++ b/kernel-shark/src/KsTraceGraph.cpp
> @@ -277,7 +277,7 @@ void KsTraceGraph::_resetPointer(uint64_t ts, int cpu, int pid)
>         QString pointer;
>
>         kshark_convert_nano(ts, &sec, &usec);
> -       pointer.sprintf("%lu.%06lu", sec, usec);
> +       pointer.sprintf("%" PRIu64 ".%06" PRIu64 "", sec, usec);
>         _labelP2.setText(pointer);
>
>         if (pid > 0 && cpu >= 0) {
> @@ -313,7 +313,7 @@ void KsTraceGraph::_setPointerInfo(size_t i)
>         uint64_t sec, usec;
>
>         kshark_convert_nano(e->ts, &sec, &usec);
> -       pointer.sprintf("%lu.%06lu", sec, usec);
> +       pointer.sprintf("%" PRIu64 ".%06" PRIu64 "", sec, usec);
>         _labelP2.setText(pointer);
>
>         comm.append("-");
> @@ -601,17 +601,17 @@ void KsTraceGraph::_updateTimeLegends()
>         QString tMin, tMid, tMax;
>
>         kshark_convert_nano(_glWindow.model()->histo()->min, &sec, &usec);
> -       tMin.sprintf("%lu.%06lu", sec, usec);
> +       tMin.sprintf("%" PRIu64 ".%06" PRIu64 "", sec, usec);
>         _labelXMin.setText(tMin);
>
>         tsMid = (_glWindow.model()->histo()->min +
>                  _glWindow.model()->histo()->max) / 2;
>         kshark_convert_nano(tsMid, &sec, &usec);
> -       tMid.sprintf("%lu.%06lu", sec, usec);
> +       tMid.sprintf("%" PRIu64 ".%06" PRIu64 "", sec, usec);
>         _labelXMid.setText(tMid);
>
>         kshark_convert_nano(_glWindow.model()->histo()->max, &sec, &usec);
> -       tMax.sprintf("%lu.%06lu", sec, usec);
> +       tMax.sprintf("%" PRIu64 ".%06" PRIu64 "", sec, usec);
>         _labelXMax.setText(tMax);
>  }
>
> diff --git a/kernel-shark/src/libkshark.c b/kernel-shark/src/libkshark.c
> index 0f0a1ba..9aaf1b7 100644
> --- a/kernel-shark/src/libkshark.c
> +++ b/kernel-shark/src/libkshark.c
> @@ -1423,7 +1423,7 @@ char* kshark_dump_custom_entry(struct kshark_context *kshark_ctx,
>         event_name = info_func(kshark_ctx, entry, false);
>         info = info_func(kshark_ctx, entry, true);
>
> -       size = asprintf(&entry_str, "%li; %s-%i; CPU %i; ; %s; %s",
> +       size = asprintf(&entry_str, "%" PRIu64 "; %s-%i; CPU %i; ; %s; %s",
>                         entry->ts,
>                         task,
>                         entry->pid,
> @@ -1472,7 +1472,7 @@ char* kshark_dump_entry(const struct kshark_entry *entry)
>                 event_name = event? event->name : "[UNKNOWN EVENT]";
>                 lat = kshark_get_latency(kshark_ctx->pevent, data);
>
> -               size = asprintf(&temp_str, "%li; %s-%i; CPU %i; %s;",
> +               size = asprintf(&temp_str, "%" PRIu64 "; %s-%i; CPU %i; %s;",
>                                 entry->ts,
>                                 task,
>                                 entry->pid,
> --
> 2.20.1
>

Hi Yordan,

I had to manually apply your two patches from 2019-06-14 manually on top of your
first patch from 2019-06-12. I was able to build KernelShark on my
Raspberry Pi 3
model B+ and observe that your combined changes resolved the abort issue as
well as compiler warnings. I also observed the same good results on my
ODROID-XU3
from hardkernel.com which also runs a 32-bit armv7l kernel as well as
my 96Boards
ROCK960 model C which runs a 64-bit aarch64 kernel.

Please see bugzilla for an attached patch file which shows the
combined changes I
manually applied as your patches intended.

Regards,
Alan