Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

script: Support native C/C++ shared object scripting #1205

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

honggyukim
Copy link
Collaborator

@honggyukim honggyukim commented Sep 29, 2020

This patch adds a native script support that can use a shared object as
a script plugin.

The basic interface is same as python and luajit, but the benefit of
this native script is that it doesn't add any overhead from the
scripting engines. It just bypasses the execution flow into the
dynamically loaded shared object.

The target shared object has to be compiled from C/C++ programs that
provides the following functions.

  void uftrace_begin(struct uftrace_script_info *sc_info);
  void uftrace_entry(struct uftrace_script_context *sc_ctx);
  void uftrace_exit(struct uftrace_script_context *sc_ctx);
  void uftrace_end(void);

The 'struct script_context' is same as the struct used inside uftrace so
it's just passed without any manipulation.

The example C/C++ files are in sctipts directory and the shared objects
can be compiled as follows.

  $ make -C scripts/
  make: Entering directory '/home/honggyu/work/uftrace/scripts'
  gcc -fPIC -shared -o simple_c.so simple.c
  gcc -fPIC -shared -o simple_cpp.so simple.cpp
  make: Leaving directory '/home/honggyu/work/uftrace/scripts'

The basic usage is as follows.

  $ uftrace record -S scripts/simple_c.so --no-libcall t-abc
  program begins...
  entry : main()
  entry : a()
  entry : b()
  entry : c()
  exit  : c()
  exit  : b()
  exit  : a()
  exit  : main()
  program is finished

The output looks exactly same as python and luajit scripts.

Signed-off-by: Honggyu Kim [email protected]

@namhyung
Copy link
Owner

Yes, I like it. Actually I thought about this some time ago, but didn't find real use cases. And we should consider argument and other info for proper usage.

scripts/simple.c Outdated
#include <stdio.h>
#include <stdint.h>

struct script_context {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth putting this into a header that can be easily included in a plugin?

Would we need any versioning of the interface?

Copy link
Collaborator Author

@honggyukim honggyukim Sep 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we can think about it. I've just put the struct as it looks very simple to copy and paste.

My concern is about having struct list_head for argument info because it adds a dependency on the internal list implementation. It's not always required so user can decide not to use and remove the dependency. I will think more about it. Thanks for the comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks very simple to copy and paste.

Depends if we're ever going to change the structure. I guess as long as we put new members at the end we will be fine.

Depends if you think it would be useful for plugins to know the argument types? Can always follow up with that later.

In the future we may want to have a uftrace/plugin.h (or similar) that includes all the necessary headers for parsing the argument and this structure. For now, it's likely fine to ignore the arguments.

Copy link
Collaborator Author

@honggyukim honggyukim Sep 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. uftrace/plugin.h looks good. We have to decide how to provide the header to users. I also prototyped another feature that also needs to provide a header to user, but it's also pending now at #636.

I will think more about how to provide those headers.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that we should provide a header for API/ABI compatibility and also it's good to use a versioning. Then we should consider installing/loading the header and plugin binaries as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made a draft change at c2c22b8 regarding versioning and providing a header uftrace-script.h.

The C/C++ scripts can be written by including uftrace-script.h and be compiled. The version can be checked before using the struct. We can change the major version when the layout is change and the minor version can be changed when we added a new members at the end of struct.

It's just a draft version, but would like to hear how you think about this approach.

@honggyukim
Copy link
Collaborator Author

I will also think about providing a way to disable the uftrace shmem record behaviour when we only need to use this native script interface.

@namhyung
Copy link
Owner

namhyung commented Oct 1, 2020

Yes, it's reasonable when we run it during record and want to reduce the overhead. It might be handled in uftrace_begin or using --disable option.

@honggyukim honggyukim force-pushed the review/native-script branch from 803bc45 to c2c22b8 Compare October 1, 2020 23:27
@honggyukim
Copy link
Collaborator Author

And we should consider argument and other info for proper usage.

I didn't use struct script_info yet in this implementation because it introduces another internal data structure struct strv so it will make the user to have more complicated header.

How about replace struct strv cmds to just const char *cmds as a concatenated string? I think we don't have to make it complicated unnecessarily and the users can parse the info themselves. I've applied this change at fe80d3f.

cmds/record.c Outdated Show resolved Hide resolved
@@ -35,6 +35,8 @@ ifneq ($(wildcard $(srcdir)/check-deps/have_libluajit),)
COMMON_CFLAGS += $(shell pkg-config --cflags luajit)
endif

COMMON_CFLAGS += -DHAVE_LIBNATIVE

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's needed as it turns on unconditionally.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I will remove this.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please..

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's now removed, but also we have to always enable script feature so added ca0c6e1.

utils/script-native.c Outdated Show resolved Hide resolved
scripts/Makefile Outdated Show resolved Hide resolved
scripts/info.c Outdated
struct list_head *argspec;
#endif
};
#include "utils/uftrace-script.h"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe "script-api.h"? And this should be installed in the target directory (probably "/usr/include/uftrace/") by make install. We can make a "include" directory in the source and put this file there. Users (or plugin developers) can include "uftrace/script-api.h" then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will make include directory and put the headers that will be installed together. If you want to put the headers under /usr/include/uftrace directory, the name can be simpler like /usr/include/uftrace/script.h.

utils/uftrace-script.h Outdated Show resolved Hide resolved
struct si_version {
uint8_t major;
uint8_t minor;
} ver;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we will have complex versioning here. Let's simply use an integer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about handling the major version change doesn't allow the previously compiled shared objects cannot be used. But minor version change allows that the shared objects can be used in a higher version of uftrace.

In summary

  • If major version is different, then uftrace rejects loading the shared object.
  • If uftrace minor version is higher than the shared object, then it can use the shared object without a problem.
  • If uftrace minor version is lower than the shared object, then uftrace rejects loading the shared object.

But I also think that we can simply use a single integer version because the layout will not be changed often and we don't actually distribute the plugin shared object binaries.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just made it to use a single uftrace version for both struct.

} ver;
char *name;
char *version;
bool record;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to have an extensible interface. Also maybe we can have an interface that script also can ask or control the behavior. For example whether it's called from libmcount (record = true), or the script needs argument info, or the script wants to disable shmem buffer saving and so on.

Maybe we can have 2 integers as bitmasks for them. One indicates current status - only the record bit is available for now. The other can control the behavior like disable, or skip arguments. I think we should keep original behavior unless script changes the bitmasks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's also possible. I also thought that it's wasteful to use a bool type here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can have 2 integers as bitmasks for them. One indicates current status - only the record bit is available for now.

For now, I've just made the existing record variable can be disabled by the given native script. If it's disabled by script, then uftrace do not actually record the data.

The other can control the behavior like disable, or skip arguments. I think we should keep original behavior unless script changes the bitmasks.

I don't see any use case for disabling argument record because we can already do this with uftrace command. I will just leave it as is, but can bump up the script version when it's needed.

* struct list_head internally uses 'new' as a variable name so
* it's not possible to use in C++.
*/
#endif
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let's not use it. We can use a simple linked list for this like:

struct sciprt_arg {
   struct script_arg *next;
   int type;  /* INT, FLOAT, DOUBLE, STR, OTHER(?) */
   char data[];
};

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may have to convert the struct list_head to this type. Could you explain why you used char data[] here?

Otherwise, how about modifying utils/list.h that can be compilable in C++, then distribute it together because uftrace also uses GPLv2 same as list.h anyway.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain why you used char data[] here?

I wanted to handle all types of arguments include strings which have variable length. But maybe we can just use a pointer to the string wrapped in an union for other types. How about this?

struct uft_script_arg {
   struct uft_script_arg *next;
   int type;
   int size;
   union {
      int si;
      unsigned int ui;
      long sl;
      ...
      char *str;
   } data;
};

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion. I see that if we introduce another way of handling arguments then we better use this in script-python.c and script-luajit.c as well without using the current struct list_head.

It may take more time to refine them all so I didn't update it for now.

@honggyukim honggyukim force-pushed the review/native-script branch 2 times, most recently from f7a2024 to cb05efc Compare October 3, 2020 22:58
@honggyukim
Copy link
Collaborator Author

I've updated this PR again as follows.

  • aabc21d uftrace: Do not use C++ keywords as variable names
  • b1d3ff2 script: Change script_info.cmds from struct strv to char*
  • 306b5fd script: Move script_info and script_context out to include/script.h
  • edeb132 script: Support native C/C++ shared object scripting
  • ab13871 script: Add version info to script_info
  • cb05efc script: Make native script disable uftrace record

Copy link
Owner

@namhyung namhyung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work!

include/script.h Outdated

#include <stdint.h>
#include <stdbool.h>
#include "utils/list.h"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to add list.h here.

include/script.h Outdated
/* for arguments and return value */
int arglen;
void *argbuf;
struct list_head *argspec;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should think about how to pass argument info. Currently it needs to understand the argspec and calculate position in the argbuf. That's why I suggested something in the previous version.

include/script.h Outdated Show resolved Hide resolved
include/script.h Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
utils/script-native.h Outdated Show resolved Hide resolved
include/script.h Outdated
int uftrace_script_version(void)
{
return SCRIPT_VERSION;
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A function definition in the header? I'm afraid that a plugin can consist of multiple files.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.. Otherwise, the script writer has to implement this. Could you recommend if you have a better idea?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's script writer's responsibility to check the version number.

cmds/script.c Outdated
@@ -133,6 +133,7 @@ int command_script(int argc, char *argv[], struct opts *opts)
struct uftrace_data handle;
struct uftrace_task_reader *task;
struct script_info info = {
.script_version = SCRIPT_VERSION,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API version? I don't want to repeat "script" in the script_info..

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed it to api_version.

INIT_NATIVE_API_FUNC(uftrace_script_version);
if (SCRIPT_VERSION != __uftrace_script_version()) {
pr_warn("incompatible script versions (uftrace %d != module %d)\n",
SCRIPT_VERSION, __uftrace_script_version());
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well.. I think we should have backward compatibility so old version of script can still be run.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's right. But that's why I made the major and minor version to handle this before. If we want to keep the backward compatibility then we must not change the layout of struct, but we can only append the data to the struct in include/script.h.

@@ -9,6 +9,9 @@ void uftrace_begin(struct script_info *sc_info)
printf("%s\n", sc_info->version);
printf("%s\n", sc_info->name);
printf("%s\n", sc_info->cmds);

/* disable actual data record by uftrace */
sc_info->record = false;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said before, I'd like to have two bitmasks to control the behavior.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can make it to use two bitmasks, but I don't see if it's useful to control argument recording so left it for now. I can update it in the next update.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do this in a separate PR. I think it's better to focus on enabling the native script here. Also other scripting like Python might use this feature too.

@honggyukim honggyukim force-pushed the review/native-script branch from cb05efc to 6fab0c8 Compare October 4, 2020 08:27
@honggyukim
Copy link
Collaborator Author

I've just pushed only after rebase to make the "force-pushed" to be clearer in the next update. I will update this PR a bit later.

@honggyukim
Copy link
Collaborator Author

@namhyung I've updated this patch again with the following changes.

  • Split uftrace_script_context into uftrace_script_base_ctx and uftrace_script_args.
  • Remove list.h dependency by passing uftrace_script_base_ctx to native script APIs without argument info.

In addition, I've already made comments for API version handling and record bits so please have a look again. I actually don't have a better idea yet so please let me know if you have better ideas.

utils/script.h Outdated
int arglen;
void *argbuf;
struct list_head *argspec;
};

/* context and args information passed to script */
struct uftrace_script_context {
struct uftrace_script_base_ctx ctx;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to rename it to 'base' so that I can refer to 'ctx->base.tid' or so.

Makefile Outdated
@@ -338,6 +340,9 @@ endif
$(Q)$(INSTALL) $(objdir)/libmcount/libmcount-fast.so $(DESTDIR)$(libdir)/libmcount-fast.so
$(Q)$(INSTALL) $(objdir)/libmcount/libmcount-single.so $(DESTDIR)$(libdir)/libmcount-single.so
$(Q)$(INSTALL) $(objdir)/libmcount/libmcount-fast-single.so $(DESTDIR)$(libdir)/libmcount-fast-single.so
$(call QUIET_INSTALL, headers)
$(Q)$(INSTALL) -m 644 $(srcdir)/include/script.h $(DESTDIR)$(incdir)/script.h
$(Q)$(INSTALL) -m 644 $(srcdir)/utils/list.h $(DESTDIR)$(incdir)/list.h
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use it anymore, right?

@@ -14,8 +14,6 @@
#include "utils/script-luajit.h"
#include "utils/utils.h"

#define SCRIPT_ENABLED (SCRIPT_LUAJIT_ENABLED || SCRIPT_PYTHON_ENABLED)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can just make this 'true', then no need to change other code.

scripts/info.c Outdated
#include <stdio.h>
#include <stdint.h>
#include <stdbool.h>
#include "include/script.h"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok to put the header file there. This script is an example that users can compile even without the uftrace source IMHO. So it should work with the system-installed header.

scripts/Makefile Outdated
@@ -0,0 +1,7 @@
script:
gcc -I.. -fPIC -shared -g -o simple_c.so simple.c
g++ -I.. -fPIC -shared -g -o simple_cpp.so simple.cpp
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to implement same one twice. Please add other one like 'count.so'?

include/script.h Outdated
char *name;
};

#endif /* UFTRACE_INCLUDE_SCRIPT_H */
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need 'extern "C"' here?

include/script.h Outdated
int uftrace_script_version(void)
{
return SCRIPT_VERSION;
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's script writer's responsibility to check the version number.

@@ -74,6 +74,9 @@ static int __maybe_unused mcount_depth = MCOUNT_DEFAULT_DEPTH;
/* boolean flag to turn on/off recording */
static bool __maybe_unused mcount_enabled = true;

/* boolean flag to turn on/off recording, but keep script working */
bool __maybe_unused record_enabled = true;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, mcount_enable can be used for this (hmm.. but it'd need this too)

diff --git a/libmcount/mcount.c b/libmcount/mcount.c
index a2310798..60b10945 100644
--- a/libmcount/mcount.c
+++ b/libmcount/mcount.c
@@ -1232,7 +1232,7 @@ void mcount_exit_filter_record(struct mcount_thread_data *mtdp,
                        mtdp->record_idx--;
 
                if (!mcount_enabled)
-                       return;
+                       goto out;
 
                if (!(rstack->flags & MCOUNT_FL_RETVAL))
                        retval = NULL;
@@ -1276,6 +1276,7 @@ void mcount_exit_filter_record(struct mcount_thread_data *mtdp,
                                mtdp->nr_events = k;  /* invalidate sync events */
                }
 
+out:
                /* script hooking for function exit */
                if (SCRIPT_ENABLED && script_str)
                        script_hook_exit(mtdp, rstack);

honggyukim added 5 commits May 6, 2023 07:31
The struct script_info and script_context can be exposed to the
external users so it'd be better to have uftrace_ prefix for their
names.

Signed-off-by: Honggyu Kim <[email protected]>
It doesn't look like a good idea to expose 'struct strv' type in
'struct script_info' when it's exposed to external users.

Rather than using the internal data structure to represent commands,
it'd be much simpler to use a concatenated char* string.

So this patch changes the type of script_info.cmds from 'struct strv' to
'const char*' type as follows.

 struct script_info {
        char                    *name;
        char                    *version;
        bool                    record;
-       struct strv             cmds;
+       const char              *cmds;
 };

Signed-off-by: Honggyu Kim <[email protected]>
The current script context is as follows.

  /* base context information passed to script */
  struct uftrace_script_context {
         int                     tid;
         int                     depth;
         uint64_t                timestamp;
         uint64_t                duration;       /* exit only */
         unsigned long           address;
         char                    *name;
         /* for arguments and return value */
         int                     arglen;
         void                    *argbuf;
         struct list_head        *argspec;
  };

This patch splits it into the following two parts so that the additional
script APIs can selectively support them.  For example, native script
API will not support arguments for now.

  /* base context information passed to script */
  struct uftrace_script_base_ctx {
         int                     tid;
         int                     depth;
         uint64_t                timestamp;
         uint64_t                duration;       /* exit only */
         unsigned long           address;
         char                    *name;
  };

  /* arguments and return value passed to script */
  struct uftrace_script_args {
         int                     arglen;
         void                    *argbuf;
         struct list_head        *argspec;
  };

  /* context and args information passed to script */
  struct uftrace_script_context {
         struct uftrace_script_base_ctx base;
         struct uftrace_script_args     args;
  };

Signed-off-by: Honggyu Kim <[email protected]>
This patch factors out uftrace_script_info and uftrace_script_base_ctx
from utils/script.h to include/script.h, which can be included in C/C++
native scripts.

The header will be installed into $DESTDIR/include/uftrace/script.h
to be used by native script writers.

Signed-off-by: Honggyu Kim <[email protected]>
Currently script can be diabled when both python and luajit is not
enabled during configure, but need to prepare to always enable it to
have native script feature.

Signed-off-by: Honggyu Kim <[email protected]>
@honggyukim honggyukim force-pushed the review/native-script branch from 6494fc0 to eaab48c Compare May 6, 2023 11:49
honggyukim added 3 commits May 6, 2023 20:55
This patch adds a native script support that can use a shared object as
a script plugin.

The basic interface is same as python and luajit, but the benefit of
this native script is that it doesn't add any overhead from the
scripting engines.  It just bypasses the execution flow into the
dynamically loaded shared object.

The target shared object has to be compiled from C/C++ programs that
provides the following functions after including "include/script.h".

  void uftrace_begin(struct uftrace_script_info *sc_info);
  void uftrace_entry(struct uftrace_script_base_ctx *sc_ctx);
  void uftrace_exit(struct uftrace_script_base_ctx *sc_ctx);
  void uftrace_end(void);

The 'struct uftrace_script_base_ctx' is same as the struct used inside
uftrace so it's just passed without any manipulation.

The example C files are in sctipts directory and the shared objects can
be compiled as follows.

  $ make -C scripts/
  make: Entering directory '/home/honggyu/work/uftrace/scripts'
  gcc -fPIC -shared -g -o simple.so simple.c
  gcc -fPIC -shared -g -o info_c.so info.c
  gcc -fPIC -shared -g -o count.so count.c
  gcc -fPIC -shared -g -o dump.so dump.c
  make: Leaving directory '/home/honggyu/work/uftrace/scripts'

The basic usage is as follows.

  $ uftrace record -S scripts/simple_c.so --no-libcall t-abc
  program begins...
  entry : main()
  entry : a()
  entry : b()
  entry : c()
  exit  : c()
  exit  : b()
  exit  : a()
  exit  : main()
  program is finished

The output looks exactly same as python and luajit scripts.

Signed-off-by: Honggyu Kim <[email protected]>
This patch adds script version info to uftrace_script_info and the
script version can be changed when the layout of the script related
struct types are changed.

The script writer has a reponsibility to check the version. The API
version of the script is SCRIPT_API_VERSION and the API version of
uftrace engine is sc_info->api_version.

Signed-off-by: Honggyu Kim <[email protected]>
This patch makes the given script to disable the actual uftrace record
to shared memory.  It allows the script control the behavior of uftrace.

Signed-off-by: Honggyu Kim <[email protected]>
@honggyukim honggyukim force-pushed the review/native-script branch from eaab48c to 5cfe799 Compare May 6, 2023 11:56
Copy link
Owner

@namhyung namhyung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this.

@@ -9,6 +9,9 @@ void uftrace_begin(struct script_info *sc_info)
printf("%s\n", sc_info->version);
printf("%s\n", sc_info->name);
printf("%s\n", sc_info->cmds);

/* disable actual data record by uftrace */
sc_info->record = false;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do this in a separate PR. I think it's better to focus on enabling the native script here. Also other scripting like Python might use this feature too.

/* context and args information passed to script */
struct uftrace_script_context {
struct uftrace_script_base_ctx base;
struct uftrace_script_args args;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is needed. Maybe we can add a separate script context definition for native script and leave this as is.

utils/script.h Outdated
@@ -29,7 +29,7 @@ struct uftrace_script_info {
char *name;
char *version;
bool record;
struct strv cmds;
const char *cmds;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not expose this to users. Native scripts can have a separate header with your change. Maybe argc + argv[] are more appropriate for native scripts.

@@ -223,6 +223,9 @@ static int luajit_uftrace_begin(struct uftrace_script_info *info)
{
int i;
char *s;
struct strv sv = {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use STRV_INIT.

@@ -572,6 +572,9 @@ int python_uftrace_begin(struct uftrace_script_info *info)
PyObject *ctx;
int i;
char *s;
struct strv sv = {
0,
};
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

"",
"#",
"--",
"//",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the trailing comma makes line break.

{
std::cout << "program is finished\n";
}
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be confusing. Please add a comment like // extern "C".

CSO = $(SRCS:%.c=%_c.so)
CPPSO = $(SRCS:%.cpp=%_cpp.so)

all: $(CSO) $(CPPSO)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be called from the top level Makefile. But I think you should split the addition of examples to a separate commit.

@honggyukim honggyukim added the script related to script execution label Jul 23, 2023
@paranlee
Copy link
Contributor

paranlee commented Mar 8, 2024

I thought it would be very useful to write a script to view CPU register information, and I'll work on that feature if it needs to be enhanced.

@paranlee
Copy link
Contributor

paranlee commented Mar 9, 2024

I've also rebased @honggyukim branch and test the rust plugin works well!

$ uftrace record -S  scripts/simple_rs.so --no-libcall tests/t-abc
uftrace_begin called scripts/simple_rs.so
uftrace_entry called with name: main
uftrace_entry called with name: a
uftrace_entry called with name: b
uftrace_entry called with name: c
uftrace_exit called with name: c
uftrace_exit called with name: b
uftrace_exit called with name: a
uftrace_exit called with name: main
uftrace_end called
use std::os::raw::c_char;
use std::ffi::CStr;

// Information passed during initialization
#[repr(C)] // Ensure C-compatible layout
pub struct UftraceScriptInfo {
	pub api_version: i32,
	pub name: *const c_char, // Use raw pointer for name and version
	pub version: *const c_char,
	pub record: bool,
	pub cmds: *const c_char,  // Use raw pointer for commands string
}

// Base context information passed to script
#[repr(C)] // Ensure C-compatible layout
pub struct UftraceScriptBaseCtx {
	pub tid: i32,
	pub depth: i32,
	pub timestamp: u64,
	pub duration: u64,
	pub address: usize, // Use usize for address on most platforms
	pub name: *const c_char, // Use raw pointer for name
}

#[no_mangle]
pub extern "C" fn uftrace_begin(info: &UftraceScriptInfo) {
	println!("uftrace_begin called {}", unsafe { CStr::from_ptr(info.name) }.to_string_lossy());
}

#[no_mangle]
pub extern "C" fn uftrace_entry(ctx: &UftraceScriptBaseCtx) {
	println!("uftrace_entry called with name: {}", unsafe { CStr::from_ptr(ctx.name) }.to_string_lossy());
}

#[no_mangle]
pub extern "C" fn uftrace_exit(ctx: &UftraceScriptBaseCtx) {
	println!("uftrace_exit called with name: {}", unsafe { CStr::from_ptr(ctx.name) }.to_string_lossy());
}

#[no_mangle]
pub extern "C" fn uftrace_end() {
	println!("uftrace_end called");
}

@namhyung
Copy link
Owner

I thought it would be very useful to write a script to view CPU register information

I'm not sure if it's useful. The register content is volatile - IOW it's changed whenever you call a function (at least for some registers).

@honggyukim
Copy link
Collaborator Author

I've also rebased @honggyukim branch and test the rust plugin works well!

Thanks!

I'll work on that feature if it needs to be enhanced.

I think current status is good but this PR is pending because we have many things to consider to keep the backward compatibility especially for argument handling. Maybe we better drop argument handling for now and let this merged without it.

@namhyung
Copy link
Owner

Maybe we better drop argument handling for now and let this merged without it.

Then it needs to be specified with API version. I thought converting all data into a string and pass them once. This would reduce the complexity to handle different types and number of arguments. But the script should parse the argument (which may be fine since it can control the argument when recording).

@yskelg
Copy link
Contributor

yskelg commented Oct 1, 2024

This is especially great for uncertain post-mortem analysis or when we are unsure about which filtering parameters to narrow down.

In my opinion, the strength of this feature lies in the fact that uftrace currently cannot dynamically observe information at runtime, but with this, we can script specific parts as needed. Additionally, if we can implement such modules using Rust, C, or C++, which have lower performance overhead compared to other scripting languages, it allows for more flexible observation of the program's behavior at runtime. And once the source code is written as a module, I think it's great that anyone can share and reuse it.

@honggyukim's slides were a great inspiration for me. Thank you so much!

image image image image

I believe that people will find better use cases than the ones I come up with, and that's how it will be.

@honggyukim
Copy link
Collaborator Author

Thanks @yskelg. I would like to complete this feature but I haven't had much time for working on this. It's in my TODO list for a long time but didn't get a strong request as of yet.

I can't promise I will continue in the near future but will come back to this when I can find some time. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
script related to script execution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants