SD-4452 Gimli on ARM #2

kkelley1 · 2024-11-06T14:16:33Z

No description provided.

kkelley1 · 2024-11-06T14:21:40Z

proc_service.c

+  if (!sym && !strncmp(obj, "libpthread.", strlen("libpthread."))) {
+    /* not found in libpthread */
+    /* glibc 2.34 (e.g. RHEL9 and above) moved libpthread symbols to libc */
+    sym = gimli_sym_lookup(ph, "libc.so.6", name);
+  }


This was the patch in gimli.spec.

Gimli is a public repo on github. Are we sure we want to make this change also public?
A safer option is to create a patch file, but keep the source code intact. Then conditionally apply the patch in gimli.spec. This could help us avoiding pushing the change into the public repo, and rebuild/alter the code for other platforms.

Do we know why it's public? Could we make it private?

Gimli itself is public because we open sourced it long ago (maybe in the omniti days? but definitely by the message systems days) for others to use and benefit from, because that's how we did things back then.

This repo is different from mine, which I think was a fork of the one that got published at OmniTI Labs (now also defunct), and yours is missing some (admittedly NOP) activity from 2012. You can find my version of the repo at:

https://github.com/wez/gimli/

There are also a number of changes I made to my fork in 2013 for darwin/intel in there that you probably don't care about. Those changes enabled the use of gimli in https://github.com/facebook/watchman back then although that project no longer uses gimli any more.

kkelley1 · 2024-11-06T14:22:37Z

symbols.c

-      printf("sym_lookup: %s => " PTRFMT "\n", name, sym ? sym->addr : 0);
+      printf("sym_lookup: %s => " PTRFMT "\n", name, find.sym ? find.sym->addr : 0);


This was a bug in the original code.

Thanks; I've applied this to my fork!

dkoerichbird · 2024-11-06T16:55:22Z

configure.ac

@@ -11,7 +11,7 @@ AC_C_BIGENDIAN
 CFLAGS=`echo $CFLAGS | sed -e 's/-O2//;'`
 CFLAGS="-D_REENTRANT $CFLAGS"
 if test "$GCC" == "yes" ; then
-  CFLAGS="$CFLAGS -Wmissing-prototypes -Wformat"
+  CFLAGS="$CFLAGS -Wmissing-prototypes -Wformat -I/opt/msys/3rdParty/include"


In the same context of @juliebin's comment below: https://github.com/SparkPost/gimli/pull/2/files#r1831155126, this -I option would mean only Momo dev boxes or those installed with msys-*-devel packages would be able to configure-and-build Gimli, did I get it right?

This is actually "safe" for machines without those packages installed: -I bogus has no practical effect. This sort of change is however generally a bad thing to put into something like configure.ac though, because it can have surprising or unexpected effects on systems that might have the packages installed.

For example, if you still have or use the GRUP stuff to build your rpm distribution for ecelerity, hard coding this sort of path can cause the build to fail if you have an older version of ecelerity installed while trying to build a new/current version of your package distribution.

This sort of thing is generally better for the build environment to put into $CPPFLAGS when it calls configure than to hard code directly into configure here.

dkoerichbird · 2024-11-06T16:56:31Z

configure.ac

@@ -35,6 +35,10 @@ case $host in
    RLDPATH="-R"
    DARWIN_DSYMUTIL="dsymutil -o .libs/wedgie.dSYM .libs/wedgie ; dsymutil -o .libs/libgimli.0.dylib.dSYM .libs/libgimli.0.dylib"
    ;;
+  *aarch64*linux*)
+    CORONER_LDFLAGS="-lthread_db -lpthread -L/opt/msys/3rdParty/lib64 -lunwind-ptrace -lunwind-generic"


Same as above for the -L option

dkoerichbird

If I am right and the baseline of this PR isn't synchronized with the 1.2.6 tag in the Mercurial repo, then I'd propose either (a) to rebase this GitHub with the tip of that foreign repo, or (b) to move these changes on top of there.

reported in SparkPost/gimli#2 which may not remain public, so I'm applying it here.

wez

I'm not entirely sure how or why GitHub notified me about this particular version of the gimli repo, but it did, and it was a nice distraction this particular evening.

I have mostly conceptual comments: you almost certainly do not care about them and do not need to listen to me about them.

They are from the perspective and concept of "gimli is an open source and generally applicable piece of software".

It was true that it was used in at least two or maybe three projects from companies other than Message Systems for a while, but I suspect that only you are using it in earnest anywhere now, so really no one other than you will be practically impacted by any changes you make here.

(and: Hi @juliebin! I hope you are well!)

wez · 2024-11-07T04:22:12Z

proc.c

+{
+  char cmd[100];
+  sprintf(cmd, "ps -L -o lwp=,comm= -p %d", proc->pid);
+  FILE *fp = popen(cmd, "r");


This is an expensive way to resolve the thread names, and is likely incorrect on !linux systems.

I would recommend instead reading /proc/PID/comm to get the thread name of an individual process on linux, and reading the directory /proc/PID/task/TID/comm to enumerate the threads and their names on linux.

turns out that I implemented it this way in 2013: wez/gimli@9b9e539

wez · 2024-11-07T04:26:45Z

proc.c

+  }
+
+  char cmd[100];
+  sprintf(cmd, "addr2line %lx -s -e /opt/msys/ecelerity/sbin/ecelerity", addr);


I realize that the goals you have now are not the same as the goals when I created this software, but: the original reasons for gimli to exist included that the target system could not be guaranteed to have developer tools installed, and in fact, might be mandated to never have them available.

This particular change both requires developer tools and assumes that the only program it will be used with is ecelerity.

While that may be true for the only way that you deploy this software today, it renders the open source version of gimli unusable with any other software.

There may not be much of it out there today, so this is likely just a technicality now.

From a "keeping the open source project usable in its original spirit" perspective, this change is not ideal, but it's been 12 years and likely nobody other than me cares.

wez · 2024-11-07T04:30:38Z

proc_service.c

+  if (!sym && !strncmp(obj, "libpthread.", strlen("libpthread."))) {
+    /* not found in libpthread */
+    /* glibc 2.34 (e.g. RHEL9 and above) moved libpthread symbols to libc */
+    sym = gimli_sym_lookup(ph, "libc.so.6", name);
+  }


Gimli itself is public because we open sourced it long ago (maybe in the omniti days? but definitely by the message systems days) for others to use and benefit from, because that's how we did things back then.

This repo is different from mine, which I think was a fork of the one that got published at OmniTI Labs (now also defunct), and yours is missing some (admittedly NOP) activity from 2012. You can find my version of the repo at:

https://github.com/wez/gimli/

There are also a number of changes I made to my fork in 2013 for darwin/intel in there that you probably don't care about. Those changes enabled the use of gimli in https://github.com/facebook/watchman back then although that project no longer uses gimli any more.

wez · 2024-11-07T04:34:07Z

configure.ac

@@ -11,7 +11,7 @@ AC_C_BIGENDIAN
 CFLAGS=`echo $CFLAGS | sed -e 's/-O2//;'`
 CFLAGS="-D_REENTRANT $CFLAGS"
 if test "$GCC" == "yes" ; then
-  CFLAGS="$CFLAGS -Wmissing-prototypes -Wformat"
+  CFLAGS="$CFLAGS -Wmissing-prototypes -Wformat -I/opt/msys/3rdParty/include"


This is actually "safe" for machines without those packages installed: -I bogus has no practical effect. This sort of change is however generally a bad thing to put into something like configure.ac though, because it can have surprising or unexpected effects on systems that might have the packages installed.

For example, if you still have or use the GRUP stuff to build your rpm distribution for ecelerity, hard coding this sort of path can cause the build to fail if you have an older version of ecelerity installed while trying to build a new/current version of your package distribution.

This sort of thing is generally better for the build environment to put into $CPPFLAGS when it calls configure than to hard code directly into configure here.

wez · 2024-11-07T04:37:40Z

linux.c

+#if 0 // __aarch64__ abandoned effort
+  uint64_t a;
+  if (gimli_read_mem(cur->proc, (gimli_addr_t)cur->st.pc, &a, sizeof(a)) == sizeof(a)) {
+    if (a == 0xd4000001d2801168) {
+      return 1;
+    }
+  }
+#endif


FWIW, I would recommend that you delete all these various unreachable code blocks from this PR to keep things smaller and easier to reason about

wez · 2024-11-07T05:16:00Z

symbols.c

-      printf("sym_lookup: %s => " PTRFMT "\n", name, sym ? sym->addr : 0);
+      printf("sym_lookup: %s => " PTRFMT "\n", name, find.sym ? find.sym->addr : 0);


Thanks; I've applied this to my fork!

wez · 2024-11-07T06:19:33Z

one of the commits in my fork is wez/gimli@1487ef8 which adds a configure option to use libunwind more directly, maybe you want to merge that and the other changes from my fork back? It's possible that it handles arm for you?

kkelley1 added 4 commits November 5, 2024 10:03

SD-4452 Gimli on ARM

be5f0cd

- intermediate commit with debugs for possible future rework

ad33031

- remove debugs and alternative workarounds

ed0b2ed

- make configure.ac changes aarch64-only

b9a84e9

kkelley1 requested review from juliebin, deepakpn and dkoerichbird November 6, 2024 14:16

kkelley1 commented Nov 6, 2024

View reviewed changes

dkoerichbird reviewed Nov 6, 2024

View reviewed changes

wez added a commit to wez/gimli that referenced this pull request Nov 7, 2024

fix debug print for gimli_sym_lookup

4ac84de

reported in SparkPost/gimli#2 which may not remain public, so I'm applying it here.

wez reviewed Nov 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SD-4452 Gimli on ARM #2

SD-4452 Gimli on ARM #2

kkelley1 commented Nov 6, 2024

kkelley1 Nov 6, 2024

juliebin Nov 6, 2024

kkelley1 Nov 6, 2024

wez Nov 7, 2024

kkelley1 Nov 6, 2024

wez Nov 7, 2024

dkoerichbird Nov 6, 2024

wez Nov 7, 2024

dkoerichbird Nov 6, 2024

dkoerichbird left a comment

wez left a comment

wez Nov 7, 2024

wez Nov 7, 2024

wez Nov 7, 2024

wez Nov 7, 2024

wez Nov 7, 2024

wez Nov 7, 2024

wez Nov 7, 2024

wez commented Nov 7, 2024

		printf("sym_lookup: %s => " PTRFMT "\n", name, sym ? sym->addr : 0);
		printf("sym_lookup: %s => " PTRFMT "\n", name, find.sym ? find.sym->addr : 0);

SD-4452 Gimli on ARM #2

Are you sure you want to change the base?

SD-4452 Gimli on ARM #2

Conversation

kkelley1 commented Nov 6, 2024

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

dkoerichbird left a comment

Choose a reason for hiding this comment

wez 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

wez commented Nov 7, 2024