SPI and FPVA fixes #25

Ethane98 · 2022-02-21T20:29:14Z

Merge in various bug fixes and improvements to SPI and the FPVA plugin from forked repo.

Addresses issues #9 #23 #21

…d_freebufs_ and big_freebufs_

kupsch

The debug macro changes could be made after merging this

kupsch · 2022-02-21T22:42:03Z

src/agent/context.cc

    char** syms = backtrace_symbols(buffer, num);
    for (int i = 0; i < num; i++) {
-      sp_debug("%lx - %s", *(unsigned long*)buffer[i], syms[i]);
+      sp_debug("agent", "%lx - %s", *(unsigned long*)buffer[i], syms[i]);


use %p instead of %lx and cast to (unsigned long*)

kupsch · 2022-02-21T22:53:42Z

src/agent/inst_workers/inst_worker_delegate.h

          size += 20; // the worse case, we emulate pc insn
        }
      }
-      sp_debug("EST SIZE - %lu", (unsigned long)size);


use %zu instead of %lu and the cast to (unsigned long)

kupsch · 2022-02-22T05:46:16Z

src/agent/parser.cc

           name.c_str(), obj->name().c_str());

-  if ((strcmp(name.c_str(), "recv") != 0) && (strcmp(name.c_str(), "read") != 0) && obj->name().find("libc-") != std::string::npos &&
+if ((strcmp(name.c_str(), "recv") != 0) && (strcmp(name.c_str(), "read") != 0) && obj->name().find("libc-") != std::string::npos &&


fix indentation

kupsch · 2022-02-22T05:52:31Z

src/agent/parser.cc

-// Find function by name.
+// Find function by mangled name.
+bool
+SpParser::FindFunctionByMangledName(string name, FuncSet* found_funcs) {


pass found_funcs by reference (or return it w/o passing as a parameter) as the value should not be null.

kupsch · 2022-02-22T06:07:10Z

src/agent/patchapi/object.h

@@ -70,7 +68,9 @@ namespace sp {
 						 sb::Symtab* symtab)
 			: ph::PatchObject(o, a, cm, cb),
 			load_addr_(la),
-			symtab_(symtab) {}
+			symtab_(symtab),
+	 		small_freebufs_ {} {}


use default initialization for small_freebufs_ and remove from constructor initializer list.
place function body on its own line.

kupsch · 2022-02-22T06:46:23Z

src/agent/propeller.cc

+    //cur_func = g_parser->FindFunction(func->GetMangledName());
+    g_parser->FindFunctionByMangledName(func->GetMangledName(), &found_funcs);
+
+    for (FuncSet::iterator i = found_funcs.begin(); i != found_funcs.end(); i++) {


In a future revision switch for loops on containers to range-based for loops for (auto i: found_funcs) {. Simpler and less error prone.

kupsch · 2022-02-22T06:50:04Z

src/agent/propeller.cc

+    g_parser->FindFunctionByMangledName(func->GetMangledName(), &found_funcs);
+
+    for (FuncSet::iterator i = found_funcs.begin(); i != found_funcs.end(); i++) {
+      if (strcmp(FUNC_CAST(*i)->GetObject()->name().c_str(), func->GetObject()->name().c_str()) == 0)


Compare using operator == directly on the strings. It is faster and easier to understand.

kupsch · 2022-02-22T07:29:17Z

src/common/common.h

+  if ((strcmp(debug_type, "injector") == 0 && getenv("SP_DEBUG_INJECTOR")) || \
+      (strcmp(debug_type, "common") == 0 && getenv("SP_DEBUG_COMMON")) || \
+      (strcmp(debug_type, "patchapi") == 0 && getenv("SP_DEBUG_PATCHAPI")) || \
+      (strcmp(debug_type, "ipc") == 0 && getenv("SP_DEBUG_IPC")) || \
+      (strcmp(debug_type, "worker") == 0 && getenv("SP_DEBUG_WORKER")) || \
+      (strcmp(debug_type, "sigtrap") == 0 && getenv("SP_DEBUG_SIGTRAP")) || \
+      (strcmp(debug_type, "agent") == 0 && getenv("SP_DEBUG_AGENT"))) { \


This is very inefficient in time and code size for something that get expanded and called a lot. strcmp and getenv are not fast functions and strcmp would be call 1-7 times and getenv once for each sp_debug. Change this at make an enum value for each debug_type enum DebugType {injectorDebug, commonDebug, ..., unknownDebug, numDebugTypes}, then make an array of of bool debugTypeEnabled[numDebugTypes] (initialized once using getenv), char *debugTypeToEnvVar[numDebugTypes], char *debugTypeToName[numDebugTypes]. Now the macro body's definition becomes

do { if (debugType >= numDebugTypes) { /* call a function that logs the provided out of range debugType? */ debugType = unknownDebugType; } if (debugTypeEnabled[debugType]) { char* nodir = basename((char*)__FILE__); fprintf(g_debug_fp, "%s [%d]: ", nodir, __LINE__); fprintf(g_debug_fp, __VA_ARGS__); fprintf(g_debug_fp, "\n"); fflush(g_debug_fp); } } while (0)

Then I would make a macro for each of the debug types

#define sp_debug_injector(...) sp_debug(injectorDebug, __VA_ARGS__) #define sp_debug_common(...) sp_debug(commonDebug, __VA_ARGS__) ...

kupsch · 2022-02-22T07:35:48Z

src/common/common.h

+    if (getenv("SP_DEBUG")) {   \
+        char* nodir = basename((char*)__FILE__);				 \
+        fprintf(stderr, "%s [%d]: ", nodir, __LINE__); \
+        fprintf(stderr, __VA_ARGS__); \
+        fprintf(stderr, "\n");  \
+        fflush(stderr); \
+      } \
+    else if (getenv("SP_FDEBUG")) {               \
+        char* nodir = basename((char*)__FILE__);				 \
+        fprintf(g_debug_fp, "%s [%d]: ", nodir, __LINE__); \
+        fprintf(g_debug_fp, __VA_ARGS__); \
+        fprintf(g_debug_fp, "\n");  \
+        fflush(g_debug_fp); \
+    }\
+  } \


set g_debug_fp to stderr if SP_DEBUG environment variable is set, or FILE* returned by fopen if SP_FDEBUG is environment variable is set. Only set debugTypeEnabled values is either is set, and only fclose file is g_debug_fp is not nullptr or stderr.

kupsch · 2022-02-22T07:41:53Z

src/common/utils.cc

+std::string
+GetExeObjName() {
+  std::string path = GetExeName();
+  return path.substr(path.find_last_of("//") + 1);


"/" instead of "//"

kupsch

There a couple of comments that you should look at. After fixing those that are problems I approve.

Also you could consider replacing getenv("X") with a function getenv_bool("X") that returned a bool. This function then uniformly decides on the truth. Just calling and returning getenv would define truth as the environment variable being defined (as it is currently), but you could also define it as not (undefined v || v is "" || v is "0" || v is "false") which might be less error prone for users.

kupsch · 2022-03-23T18:51:58Z

src/agent/agent.cc

@@ -67,7 +69,7 @@ namespace sp {
      core_limit.rlim_cur = RLIM_INFINITY;
      core_limit.rlim_max = RLIM_INFINITY;
      if (setrlimit(RLIMIT_CORE, &core_limit) < 0) {
-        sp_perror("ERROR: failed to setup core dump ability\n");
+        sp_perror("RROR: failed to setup core dump ability\n");


kupsch · 2022-03-23T19:12:34Z

src/common/common.h

+  if (debugType >= numDebugTypes) { \
+    debugType = unknownDebugType; \
+  } \
+  if (debugTypeEnabled[debugType]) { \
    if (getenv("SP_DEBUG")) {   \


Put the output FILE in a variable and use it. Then the two branches are identical. The output FILE variable can then be computed once eliminating the getenv on every sp_debug.

kupsch · 2022-03-23T19:18:34Z

src/agent/parser.cc

           name.c_str());
-
+
+  FuncSet* found_funcs = new FuncSet();  


This is a memory leak. Nothing every frees this memory. Change to FuncSet found_funcs; and return found_funcs;.

Ethane98 added 11 commits September 15, 2021 14:28

Fixes to get spi to build correctly again

8ac2582

Test suite fix and README update

5f2ab47

Added SPI trampguard fix and removed skipping instrumenting libstdc++

0efeb7f

Properly initialize small_freebufs_ to fix allocator issue, remove mi…

bfac326

…d_freebufs_ and big_freebufs_

Fpva build and python xml parsing fixes

63057c1

Exclude libclassad to instrument condor_master correctly

bcd1b66

Begin SIGILL support and sigaction changes

46e3de3

Support duplicate function names across objects

c00b39a

Clean up outdated code

edb8b49

Trampguard revision

a06489a

SP_DEBUG improvements and trampguard fix

8052b06

Ethane98 requested a review from kupsch February 21, 2022 20:29

Ethane98 self-assigned this Feb 21, 2022

This was linked to issues Feb 21, 2022

TrampGuard to prevent recursive instrumentation #9

Closed

Need to reduce the amount of debug log #21

Closed

Checking for libc++ functions is artificial in propeller #23

Closed

kupsch requested changes Feb 22, 2022

View reviewed changes

Pull request changes

594a0f1

kupsch reviewed Mar 23, 2022

View reviewed changes

Debug/parser pull request changes

ede3641

Ethane98 merged commit ffe7370 into dyninst:master Mar 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SPI and FPVA fixes #25

SPI and FPVA fixes #25

Ethane98 commented Feb 21, 2022 •

edited

Loading

kupsch left a comment

kupsch Feb 21, 2022

kupsch Feb 21, 2022

kupsch Feb 22, 2022

kupsch Feb 22, 2022

kupsch Feb 22, 2022

kupsch Feb 22, 2022

kupsch Feb 22, 2022

kupsch Feb 22, 2022

kupsch Feb 22, 2022

kupsch Feb 22, 2022

kupsch left a comment

kupsch Mar 23, 2022

kupsch Mar 23, 2022

kupsch Mar 23, 2022

SPI and FPVA fixes #25

SPI and FPVA fixes #25

Conversation

Ethane98 commented Feb 21, 2022 • edited Loading

kupsch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kupsch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ethane98 commented Feb 21, 2022 •

edited

Loading