Re: [gofrontend-dev] Re: Go patch committed: Improve name mangling for package paths

2018-10-26 Thread Than McIntosh via gcc-patches
OK, thanks again. Another fix sent:

 https://go-review.googlesource.com/c/gofrontend/+/145021

Cheers, Than

On Fri, Oct 26, 2018 at 10:20 AM Rainer Orth
 wrote:
>
> Hi Than,
>
> > Thanks for reporting this.
> >
> > Sent https://go-review.googlesource.com/c/gofrontend/+/145017 with a
> > tentative fix.
>
> fine, thanks.
>
> While actually running the libgo testsuite, another issue came up: all
> tests FAIL like this:
>
> /vol/gcc/src/hg/trunk/local/libgo/testsuite/gotest[516]: local: not found [No 
> such file or directory]
> FAIL: crypto/sha512
>
> The Solaris 11 /bin/sh is ksh93, and Solaris 10 even comes with the
> original Bourne shell, neither of which support the unportable local
> command.  AFAICS, there's no need at all to use it, and indeed with the
> following patch libgo testsuite results are as before the patch.
>
> Rainer
>
> --
> -
> Rainer Orth, Center for Biotechnology, Bielefeld University
>
>


Re: [gofrontend-dev] Re: Go patch committed: Improve name mangling for package paths

2018-10-26 Thread Than McIntosh via gcc-patches
Thanks for reporting this.

Sent https://go-review.googlesource.com/c/gofrontend/+/145017 with a
tentative fix.

Than

On Fri, Oct 26, 2018 at 7:55 AM Rainer Orth  
wrote:
>
> Hi Ian,
>
> > This patch by Than McIntosh improves the mangling of package paths in
> > the Go frontend.
> >
> > The current implementation of Gogo::pkgpath_for_symbol was written in
> > a way that allowed two distinct package paths to map to the same
> > symbol, which could cause collisions at link- time or compile-time.
> >
> > This patch switches to a better mangling scheme to ensure that we get
> > a unique packagepath symbol for each package.  In the new scheme
> > instead of having separate mangling schemes for identifiers and
> > package paths, the main identifier mangler ("go_encode_id") now
> > handles mangling of both packagepath characters and identifier
> > characters.
> >
> > The new mangling scheme is more intrusive: "foo/bar.Baz" is mangled as
> > "foo..z2fbar.Baz" instead of "foo_bar.Baz".  To mitigate this, this
> > patch also adds a demangling capability so that function names
> > returned from runtime.CallersFrames are converted back to their
> > original unmangled form.
> >
> > Changing the pkgpath_for_symbol scheme requires updating a number of
> > //go:linkname directives and C "__asm__" directives to match the new
> > scheme, as well as updating the 'gotest' driver (which makes
> > assumptions about the correct mapping from pkgpath symbol to package
> > name).
>
> it seems you missed a case here: both i386-pc-solaris2.* and
> sparc-sun-solaris2.* bootstraps broke linking the gotools:
>
> Undefined   first referenced
>  symbol in file
> log..z2fsyslog.syslog_c 
> ../sparc-sun-solaris2.11/libgo/.libs/libgo.so
> ld: fatal: symbol referencing errors
> collect2: error: ld returned 1 exit status
> make[2]: *** [Makefile:751: go] Error 1
>
> The following patch fixes this allowing the links to succeed, though
> I've not run the testsuite yet.
>
> Rainer
>
> --
> -
> Rainer Orth, Center for Biotechnology, Bielefeld University
>
>
> --
> You received this message because you are subscribed to the Google Groups 
> "gofrontend-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an 
> email to gofrontend-dev+unsubscr...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.


[PATCH] fix PR 86213, split stack runtime clobber of SSE reg

2018-06-20 Thread Than McIntosh via gcc-patches
Hi all,

Please find below a patch to fix PR 86213. This changes the runtime
code for -fsplit-stack to move a couple of calls (getenv, etc) from
the routine called by __morestack to a function called on startup,
so as to avoid having the calls clobber SSE input regs.

Thanks, Than

---

libgcc/ChangeLog:

   * libgcc/generic-morestack.c (allocate_segment): move
   calls to getenv and getpagesize to __morestack_load_mmap
   (__morestack_load_mmap) initialize static_pagesize and
   use_guard_page here so as to avoid clobbering SSE
   regs during a __morestack call.

gcc/testsuite/ChangeLog:

PR libgcc/86213
* gcc.dg/split-8.c: New test.

diff --git a/gcc/testsuite/gcc.dg/split-8.c b/gcc/testsuite/gcc.dg/split-8.c
new file mode 100644
index 000..33662e24c4f
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/split-8.c
@@ -0,0 +1,43 @@
+/* { dg-do run } */
+/* { dg-require-effective-target split_stack } */
+/* { dg-options "-fsplit-stack" } */
+
+/* Testcase for PR86213. On the first call to __morestack there is a live
+   value in xmm0, which was being clobbered by a call to getenv().  */
+
+#include 
+
+double gd[8];
+int z;
+
+double bar(double q)  __attribute__ ((noinline));
+double foo(double q)  __attribute__ ((noinline));
+int ck(double q)  __attribute__ ((noinline));
+int main(int argc, char **argv) __attribute__ ((no_split_stack));
+
+double bar(double q)
+{
+  double d[8];
+  for (unsigned i = 0; i < 8; ++i)
+d[i] = gd[8-i-1];
+  return q + d[z&3];
+}
+
+double foo(double d)
+{
+  return bar(d);
+}
+
+int ck(double d)
+{
+  if (d != 64.0)
+abort();
+  return 0;
+}
+
+typedef double (*fp)(double);
+fp g = foo;
+
+int main(int argc, char **argv) {
+  return ck(g(64.0));
+}
diff --git a/libgcc/generic-morestack.c b/libgcc/generic-morestack.c
index 80bfd7feda7..d70ca0922ce 100644
--- a/libgcc/generic-morestack.c
+++ b/libgcc/generic-morestack.c
@@ -243,6 +243,12 @@ __thread struct initial_sp __morestack_initial_sp

 static sigset_t __morestack_fullmask;

+/* Page size, as returned from getpagesize(). Set on startup. */
+static unsigned int static_pagesize;
+
+/* Set on startup to non-zero value if SPLIT_STACK_GUARD env var is set. */
+static int use_guard_page;
+
 /* Convert an integer to a decimal string without using much stack
space.  Return a pointer to the part of the buffer to use.  We this
instead of sprintf because sprintf will require too much stack
@@ -320,8 +326,6 @@ __morestack_fail (const char *msg, size_t len, int err)
 static struct stack_segment *
 allocate_segment (size_t frame_size)
 {
-  static unsigned int static_pagesize;
-  static int use_guard_page;
   unsigned int pagesize;
   unsigned int overhead;
   unsigned int allocate;
@@ -329,27 +333,6 @@ allocate_segment (size_t frame_size)
   struct stack_segment *pss;

   pagesize = static_pagesize;
-  if (pagesize == 0)
-{
-  unsigned int p;
-
-  pagesize = getpagesize ();
-
-#ifdef __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
-  p = __sync_val_compare_and_swap (_pagesize, 0, pagesize);
-#else
-  /* Just hope this assignment is atomic.  */
-  static_pagesize = pagesize;
-  p = 0;
-#endif
-
-  use_guard_page = getenv ("SPLIT_STACK_GUARD") != 0;
-
-  /* FIXME: I'm not sure this assert should be in the released
- code.  */
-  assert (p == 0 || p == pagesize);
-}
-
   overhead = sizeof (struct stack_segment);

   allocate = pagesize;
@@ -815,7 +798,10 @@ __generic_findstack (void *stack)
 /* This function is called at program startup time to make sure that
mmap, munmap, and getpagesize are resolved if linking dynamically.
We want to resolve them while we have enough stack for them, rather
-   than calling into the dynamic linker while low on stack space.  */
+   than calling into the dynamic linker while low on stack space.
+   Similarly, invoke getenv here to check for split-stack related control
+   variables, since doing do as part of the __morestack path can result
+   in unwanted use of SSE/AVX registers (see GCC PR 86213). */

 void
 __morestack_load_mmap (void)
@@ -825,7 +811,12 @@ __morestack_load_mmap (void)
  TLS accessor function is resolved.  */
   mmap (__morestack_current_segment, 0, PROT_READ, MAP_ANONYMOUS, -1, 0);
   mprotect (NULL, 0, 0);
-  munmap (0, getpagesize ());
+  munmap (0, static_pagesize);
+
+  /* Initialize these values here, so as to avoid dynamic linker
+ activity as part of a __morestack call. */
+  static_pagesize = getpagesize();
+  use_guard_page = getenv ("SPLIT_STACK_GUARD") != 0;
 }

 /* This function may be used to iterate over the stack segments.


Re: libgo patch committed: break dependence on unwind-pe.h

2018-05-03 Thread Than McIntosh via gcc-patches
Thanks for catching that.

I don't have access to test machines, but could you possibly try the
attached patch?

Thanks, Than



On Thu, May 3, 2018 at 3:03 PM Rainer Orth 
wrote:

> Hi Ian,

> > This patch by Than McIntosh breaks the dependence of go-unwind.c on
> > unwind-pe.h, by adding the required definitions and code directly to
> > go-unwind.c.  go-unwind.c still depends on the public unwind.h API.
> > This makes it easier to build libgo separately.  Bootstrapped and ran
> > Go testsuite on x86_64-pc-linux-gnu.  Committed to mainline.

> I strongly suspect that this patch severely broke Go on Solaris/SPARC:
> between 20180502 (r259840) and 20180503 (r259897) there appeared tons of
> new execution failures, both 32 and 64-bit:

> +FAIL: go.go-torture/execute/printnil.go execution,  -O0
> +FAIL: go.go-torture/execute/printnil.go execution,  -O1
> +FAIL: go.go-torture/execute/printnil.go execution,  -O2
> +FAIL: go.go-torture/execute/printnil.go execution,  -O2 -fbounds-check
> +FAIL: go.go-torture/execute/printnil.go execution,  -O2
-fomit-frame-pointer -finline-functions
> +FAIL: go.go-torture/execute/printnil.go execution,  -O2
-fomit-frame-pointer -finline-functions -funroll-loops
> +FAIL: go.go-torture/execute/printnil.go execution,  -O3 -g
> +FAIL: go.go-torture/execute/printnil.go execution,  -Os

> and many more, also in libgo and gotools tests.

> One example is printnil.x:

> Thread 9 received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 4 (LWP 4)]
> read_encoded_value (val=, p=0xfebcb953 "",
>  encoding=3 '\003', context=0x0)
>  at /vol/gcc/src/hg/trunk/local/libgo/runtime/go-unwind.c:250
> 250 decoded = (_Unwind_Internal_Ptr)(*(const void *const
*)p);
> (gdb) where
> #0  read_encoded_value (val=, p=0xfebcb953 "",
>  encoding=3 '\003', context=0x0)
>  at /vol/gcc/src/hg/trunk/local/libgo/runtime/go-unwind.c:250
> #1  __gccgo_personality_v0 (version=, actions=1,
>  exception_class=, ue_header=0x1cc44000,
context=0x105810ec)
>  at /vol/gcc/src/hg/trunk/local/libgo/runtime/go-unwind.c:472
> #2  0xfefbd450 in _Unwind_RaiseException (exc=0x1cc44000)
>  at
/builds2/ulhg/workspace/Solaris_Trunk/Userland/full-build/02b-build-sparc/components/gcc7/gcc-7.3.0/libgcc/unwind.inc:113
> #3  0xfe59fb00 in runtime.throwException ()
>  at /vol/gcc/src/hg/trunk/local/libgo/runtime/go-unwind.c:124
> #4  0xfe9e5a90 in runtime.unwindStack ()
>  at /vol/gcc/src/hg/trunk/local/libgo/go/runtime/panic.go:336
> #5  runtime.gopanic (e=...)
>  at /vol/gcc/src/hg/trunk/local/libgo/go/runtime/panic.go:527
> #6  0xfe5a0758 in runtime_panicstring (s=0xfe3e32d8 "nil pointer
dereference")
>  at /vol/gcc/src/hg/trunk/local/libgo/runtime/panic.c:38
> #7  0xfe59f65c in __go_runtime_error (i=)
>  at /vol/gcc/src/hg/trunk/local/libgo/runtime/go-runtime-error.c:76
> #8  0x000120b4 in main.MyType.String ()
>  at
/vol/gcc/src/hg/trunk/local/gcc/testsuite/go.go-torture/execute/printnil.go:11
> #9  0xfe6efa08 in fmt.pp.handleMethods (p=0x10bf4000, verb=115)
>  at /vol/gcc/src/hg/trunk/local/libgo/go/fmt/print.go:603
> #10 0xfe6eebdc in fmt.pp.printArg (p=0x10bf4000, arg=..., verb=115)
>  at /vol/gcc/src/hg/trunk/local/libgo/go/fmt/print.go:686
> #11 0xfe6f0d84 in fmt.pp.doPrintf (p=0x10bf4000, format=..., a=...)
>  at /vol/gcc/src/hg/trunk/local/libgo/go/fmt/print.go:1003
> #12 0xfe6f1460 in fmt.Sprintf (format=..., a=...)
>  at /vol/gcc/src/hg/trunk/local/libgo/go/fmt/print.go:203
> #13 0x000121e0 in main.main ()
>  at
/vol/gcc/src/hg/trunk/local/gcc/testsuite/go.go-torture/execute/printnil.go:16

> Seems like the new code doesn't play well on strict-alignment targets.

>  Rainer

> --

-
> Rainer Orth, Center for Biotechnology, Bielefeld University
diff --git a/libgo/runtime/go-unwind.c b/libgo/runtime/go-unwind.c
index 536a619d..6464bec7 100644
--- a/libgo/runtime/go-unwind.c
+++ b/libgo/runtime/go-unwind.c
@@ -247,7 +247,7 @@ read_encoded_value (struct _Unwind_Context *context, 
uint8_t encoding,
   break;
 }
   case DW_EH_PE_absptr:
-decoded = (_Unwind_Internal_Ptr)(*(const void *const *)p);
+__builtin_memcpy (, (const void *)p, sizeof(const void*));
 p += sizeof(void *);
 break;
   default:


Re: libbacktrace patch: fix memory allocation buglet on DWARF line table with zero dirs

2017-05-19 Thread Than McIntosh via gcc-patches
Hi again,

I wrote:
== The attached patch to libbacktrace is intended to fix a memory
== allocation bug involving reading of line table information.

I've revised my previous patch to include a new test ("edtest") that
verifies the fix.

Thanks, Than

---

Patch (take 2):

diff --git a/Makefile.am b/Makefile.am
index ec494c4..d236261 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -96,6 +96,17 @@ stest_LDADD = libbacktrace.la

 check_PROGRAMS += stest

+edtest_SOURCES = edtest.c edtest2_build.c
+edtest_LDADD = libbacktrace.la
+
+check_PROGRAMS += edtest
+
+edtest2_build.c: gen_edtest2_build; @true
+gen_edtest2_build: $(srcdir)/edtest2.c
+ cat $(srcdir)/edtest2.c > tmp-edtest2_build.c
+ $(SHELL) $(srcdir)/mvifdiff.sh tmp-edtest2_build.c edtest2_build.c
+ echo timestamp > $@
+
 endif NATIVE

 # We can't use automake's automatic dependency tracking, because it
@@ -126,5 +137,6 @@ read.lo: config.h backtrace.h internal.h
 simple.lo: config.h backtrace.h internal.h
 sort.lo: config.h backtrace.h internal.h
 stest.lo: config.h backtrace.h internal.h
+edtest.lo: config.h backtrace.h internal.h
 state.lo: config.h backtrace.h backtrace-supported.h internal.h
 unknown.lo: config.h backtrace.h internal.h
diff --git a/Makefile.in b/Makefile.in
index f3a7a42..04c6689 100644
--- a/Makefile.in
+++ b/Makefile.in
@@ -85,7 +85,7 @@ build_triplet = @build@
 host_triplet = @host@
 target_triplet = @target@
 check_PROGRAMS = $(am__EXEEXT_1)
-@NATIVE_TRUE@am__append_1 = btest stest
+@NATIVE_TRUE@am__append_1 = btest stest edtest
 subdir = .
 DIST_COMMON = $(srcdir)/Makefile.in $(srcdir)/Makefile.am \
  $(top_srcdir)/configure $(am__configure_deps) \
@@ -135,13 +135,18 @@ am__DEPENDENCIES_1 =
 am_libbacktrace_la_OBJECTS = atomic.lo dwarf.lo fileline.lo posix.lo \
  print.lo sort.lo state.lo
 libbacktrace_la_OBJECTS = $(am_libbacktrace_la_OBJECTS)
-@NATIVE_TRUE@am__EXEEXT_1 = btest$(EXEEXT) stest$(EXEEXT)
+@NATIVE_TRUE@am__EXEEXT_1 = btest$(EXEEXT) stest$(EXEEXT) \
+@NATIVE_TRUE@ edtest$(EXEEXT)
 @NATIVE_TRUE@am_btest_OBJECTS = btest-btest.$(OBJEXT)
 btest_OBJECTS = $(am_btest_OBJECTS)
 @NATIVE_TRUE@btest_DEPENDENCIES = libbacktrace.la
 btest_LINK = $(LIBTOOL) --tag=CC $(AM_LIBTOOLFLAGS) $(LIBTOOLFLAGS) \
  --mode=link $(CCLD) $(btest_CFLAGS) $(CFLAGS) $(AM_LDFLAGS) \
  $(LDFLAGS) -o $@
+@NATIVE_TRUE@am_edtest_OBJECTS = edtest.$(OBJEXT) \
+@NATIVE_TRUE@ edtest2_build.$(OBJEXT)
+edtest_OBJECTS = $(am_edtest_OBJECTS)
+@NATIVE_TRUE@edtest_DEPENDENCIES = libbacktrace.la
 @NATIVE_TRUE@am_stest_OBJECTS = stest.$(OBJEXT)
 stest_OBJECTS = $(am_stest_OBJECTS)
 @NATIVE_TRUE@stest_DEPENDENCIES = libbacktrace.la
@@ -158,7 +163,7 @@ LINK = $(LIBTOOL) --tag=CC $(AM_LIBTOOLFLAGS)
$(LIBTOOLFLAGS) \
  --mode=link $(CCLD) $(AM_CFLAGS) $(CFLAGS) $(AM_LDFLAGS) \
  $(LDFLAGS) -o $@
 SOURCES = $(libbacktrace_la_SOURCES) $(EXTRA_libbacktrace_la_SOURCES) \
- $(btest_SOURCES) $(stest_SOURCES)
+ $(btest_SOURCES) $(edtest_SOURCES) $(stest_SOURCES)
 MULTISRCTOP =
 MULTIBUILDTOP =
 MULTIDIRS =
@@ -350,6 +355,8 @@ TESTS = $(check_PROGRAMS)
 @NATIVE_TRUE@btest_LDADD = libbacktrace.la
 @NATIVE_TRUE@stest_SOURCES = stest.c
 @NATIVE_TRUE@stest_LDADD = libbacktrace.la
+@NATIVE_TRUE@edtest_SOURCES = edtest.c edtest2_build.c
+@NATIVE_TRUE@edtest_LDADD = libbacktrace.la
 all: config.h
  $(MAKE) $(AM_MAKEFLAGS) all-am

@@ -452,6 +459,9 @@ clean-checkPROGRAMS:
 btest$(EXEEXT): $(btest_OBJECTS) $(btest_DEPENDENCIES)
$(EXTRA_btest_DEPENDENCIES)
  @rm -f btest$(EXEEXT)
  $(btest_LINK) $(btest_OBJECTS) $(btest_LDADD) $(LIBS)
+edtest$(EXEEXT): $(edtest_OBJECTS) $(edtest_DEPENDENCIES)
$(EXTRA_edtest_DEPENDENCIES)
+ @rm -f edtest$(EXEEXT)
+ $(LINK) $(edtest_OBJECTS) $(edtest_LDADD) $(LIBS)
 stest$(EXEEXT): $(stest_OBJECTS) $(stest_DEPENDENCIES)
$(EXTRA_stest_DEPENDENCIES)
  @rm -f stest$(EXEEXT)
  $(LINK) $(stest_OBJECTS) $(stest_LDADD) $(LIBS)
@@ -801,6 +811,12 @@ uninstall-am: uninstall-includeHEADERS
uninstall-libLTLIBRARIES
  uninstall-am uninstall-includeHEADERS uninstall-libLTLIBRARIES


+@NATIVE_TRUE@edtest2_build.c: gen_edtest2_build; @true
+@NATIVE_TRUE@gen_edtest2_build: $(srcdir)/edtest2.c
+@NATIVE_TRUE@ cat $(srcdir)/edtest2.c > tmp-edtest2_build.c
+@NATIVE_TRUE@ $(SHELL) $(srcdir)/mvifdiff.sh tmp-edtest2_build.c
edtest2_build.c
+@NATIVE_TRUE@ echo timestamp > $@
+
 # We can't use automake's automatic dependency tracking, because it
 # breaks when using bootstrap-lean.  Automatic dependency tracking
 # with GCC bootstrap will cause some of the objects to depend on
@@ -829,6 +845,7 @@ read.lo: config.h backtrace.h internal.h
 simple.lo: config.h backtrace.h internal.h
 sort.lo: config.h backtrace.h internal.h
 stest.lo: config.h backtrace.h internal.h
+edtest.lo: config.h backtrace.h internal.h
 state.lo: config.h backtrace.h backtrace-supported.h internal.h
 unknown.lo: config.h backtrace.h internal.h

diff --git a/dwarf.c b/dwarf.c
index b182567..c1bfbf5 100644
--- a/dwarf.c
+++ b/dwarf.c
@@ -1648,16 +1648,15 @@ add_line (struct backtrace_state *state,
struct 

libbacktrace patch: fix memory allocation buglet on DWARF line table with zero dirs

2017-05-12 Thread Than McIntosh via gcc-patches
Hello,

The attached patch to libbacktrace is intended to fix a memory
allocation bug involving reading of line table information.

The scenario of interest takes place when libbacktrace reads a DWARF
line table whose directory count is zero (an unusual case). If the
memory allocator invocation triggers a call to mmap, this can cause
the size passed to mmap to be zero, resulting in an EINVAL error.  The
fix is to detect the zero-directory case and avoid invoking the
allocation helper with a zero size.

Questions + comments welcome.

Thanks, Than

---

diff --git a/libbacktrace/dwarf.c b/libbacktrace/dwarf.c
index 80c64034092..bcb2c0991ef 100644
--- a/libbacktrace/dwarf.c
+++ b/libbacktrace/dwarf.c
@@ -1563,16 +1563,15 @@ add_line (struct backtrace_state *state,
struct dwarf_data *ddata,
   return 1;
 }

-/* Free the line header information.  If FREE_FILENAMES is true we
-   free the file names themselves, otherwise we leave them, as there
-   may be line structures pointing to them.  */
+/* Free the line header information.  */

 static void
 free_line_header (struct backtrace_state *state, struct line_header *hdr,
   backtrace_error_callback error_callback, void *data)
 {
-  backtrace_free (state, hdr->dirs, hdr->dirs_count * sizeof (const char *),
-  error_callback, data);
+  if (hdr->dirs_count != 0)
+backtrace_free (state, hdr->dirs, hdr->dirs_count * sizeof (const char *),
+error_callback, data);
   backtrace_free (state, hdr->filenames,
   hdr->filenames_count * sizeof (char *),
   error_callback, data);
@@ -1633,12 +1632,16 @@ read_line_header (struct backtrace_state
*state, struct unit *u,
   ++hdr->dirs_count;
 }

-  hdr->dirs = ((const char **)
-   backtrace_alloc (state,
- hdr->dirs_count * sizeof (const char *),
- line_buf->error_callback, line_buf->data));
-  if (hdr->dirs == NULL)
-return 0;
+  hdr->dirs = NULL;
+  if (hdr->dirs_count != 0)
+{
+  hdr->dirs = ((const char **)
+   backtrace_alloc (state,
+hdr->dirs_count * sizeof (const char *),
+line_buf->error_callback, line_buf->data));
+  if (hdr->dirs == NULL)
+return 0;
+}

   i = 0;
   while (*hdr_buf.buf != '\0')