On 05/16/2017 02:52 PM, Bernd Edlinger wrote:
I think I solved the problem with -fsplit-stack, I am not sure
if ix86_static_chain_on_stack might change after reload due to
final.c possibly calling targetm.calls.static_chain, but if that
is the case, that is an already pre-existing problem.
The goal of this patch is to make all decisions regarding the
frame layout before the reload pass, and to make sure that
the frame layout does not change unexpectedly it asserts
that the data that goes into the decision does not change
after reload_completed.
With the attached patch -fsplit-stack and the attribute ms_hook_prologue
is handed directly at the ix86_expand_call, because that data is
already known before expansion.
The calls_eh_return and ix86_static_chain_on_stack may become
known at a later time, but after reload it should not change any more.
To be sure, I added an assertion at ix86_static_chain, which the
regression test did not trigger, neither with -m64 nor with -m32.
I have bootstrapped the patch several times, and a few times I
encounterd a segfault in the garbage collection, but it did not
happen every time. Currently I think that is unrelated to this patch.
Bootstrapped and reg-tested on x86_64-pc-linux-gnu with -m64/-m32.
Is it OK for trunk?
Thanks
Bernd.
With as many formatting errors as I seem to have had, I would like to
fix those then you patch on top of that if you wouldn't mind terribly.
While gcc uses subversion, git-blame is still very helpful (then again,
since Uros committed it for me, I guess that's already off).
Index: gcc/config/i386/i386.c
===================================================================
--- gcc/config/i386/i386.c (revision 248031)
+++ gcc/config/i386/i386.c (working copy)
@@ -2425,7 +2425,9 @@ static int const x86_64_int_return_registers[4] =
/* Additional registers that are clobbered by SYSV calls. */
-unsigned const x86_64_ms_sysv_extra_clobbered_registers[12] =
+#define NUM_X86_64_MS_CLOBBERED_REGS 12
+static int const x86_64_ms_sysv_extra_clobbered_registers
+ [NUM_X86_64_MS_CLOBBERED_REGS] =
Is there a reason you're changing this unsigned to signed int? While
AX_REG and such are just preprocessor macros, everywhere else it seems
that register numbers are dealt with as unsigned ints.
@@ -2484,13 +2486,13 @@ class xlogue_layout {
needs to store registers based upon data in the
machine_function. */
HOST_WIDE_INT get_stack_space_used () const
{
- const struct machine_function &m = *cfun->machine;
- unsigned last_reg = m.call_ms2sysv_extra_regs + MIN_REGS - 1;
+ const struct machine_function *m = cfun->machine;
+ unsigned last_reg = m->call_ms2sysv_extra_regs + MIN_REGS - 1;
What is the reason for this change?
- gcc_assert (m.call_ms2sysv_extra_regs <= MAX_EXTRA_REGS);
+ gcc_assert (m->call_ms2sysv_extra_regs <= MAX_EXTRA_REGS);
return m_regs[last_reg].offset
- + (m.call_ms2sysv_pad_out ? 8 : 0)
- + STUB_INDEX_OFFSET;
+ + (m->call_ms2sysv_pad_out ? 8 : 0)
+ + STUB_INDEX_OFFSET;
}
/* Returns the offset for the base pointer used by the stub. */
@@ -2532,7 +2534,7 @@ class xlogue_layout {
/* Lazy-inited cache of symbol names for stubs. */
char
m_stub_names[XLOGUE_STUB_COUNT][VARIANT_COUNT][STUB_NAME_MAX_LEN];
- static const struct xlogue_layout GTY(())
s_instances[XLOGUE_SET_COUNT];
+ static const struct GTY(()) xlogue_layout
s_instances[XLOGUE_SET_COUNT];
Hmm, during development I originally had C-style xlogue_layout as a
struct and later decided to make it a class and apparently forgot to
remove the "struct" here. None the less, it's bazaar that the GTY()
would go in between the "struct" and the "xlogue_layout." As I said
before, I don't fully understand how this GTY works. Can we just remove
the "struct" keyword?
Also, if the way I had it was wrong, (and resulted in garbage collection
not working right) then perhaps it was the cause of a problem I had with
caching symbol rtx objects. I could not get this to work because my
cached objects would somehow become stale and I've since removed that
code (from xlogue_layout::get_stub_rtx). (i.e., does GTY effect
lifespan of globals, TU statics and static C++ data members?)
/* Constructor for xlogue_layout. */
@@ -2639,11 +2643,11 @@ xlogue_layout::xlogue_layout (HOST_WIDE_INT
stack_
: m_hfp (hfp) , m_nregs (hfp ? 17 : 18),
m_stack_align_off_in (stack_align_off_in)
{
+ HOST_WIDE_INT offset = stack_align_off_in;
+ unsigned i, j;
+
memset (m_regs, 0, sizeof (m_regs));
memset (m_stub_names, 0, sizeof (m_stub_names));
-
- HOST_WIDE_INT offset = stack_align_off_in;
- unsigned i, j;
for (i = j = 0; i < MAX_REGS; ++i)
{
unsigned regno = REG_ORDER[i];
@@ -2662,11 +2666,12 @@ xlogue_layout::xlogue_layout (HOST_WIDE_INT
stack_
m_regs[j].regno = regno;
m_regs[j++].offset = offset - STUB_INDEX_OFFSET;
}
- gcc_assert (j == m_nregs);
+ gcc_assert (j == m_nregs);
}
Aside from my formatting errors, this would actually be incorrect per
the GNU coding conventions
(https://gcc.gnu.org/codingconventions.html#Variable), which is probably
based off of Effective C++ Item 26. Obviously, we're still dealing with
part of this as classical C++ and the rest as if it were C, but I'm
trying to follow C++ norms in C++ classes and member functions.
@@ -2676,7 +2681,7 @@ xlogue_layout::xlogue_layout (HOST_WIDE_INT stack_
{
int res = snprintf (name, STUB_NAME_MAX_LEN, "__%s_%u",
STUB_BASE_NAMES[stub], MIN_REGS + n_extra_regs);
- gcc_checking_assert (res <= (int)STUB_NAME_MAX_LEN);
+ gcc_checking_assert (res < (int)STUB_NAME_MAX_LEN);
Good catch! Thank you.
@@ -12634,10 +12573,6 @@ ix86_hard_regno_scratch_ok (unsigned int regno)
&& df_regs_ever_live_p (regno)));
}
-/* Registers who's save & restore will be managed by stubs called from
- pro/epilogue. */
-static HARD_REG_SET GTY(()) stub_managed_regs;
-
/* Return true if register class CL should be an additional allocno
class. */
@@ -12718,10 +12653,17 @@ ix86_save_reg (unsigned int regno, bool
maybe_eh_r
}
}
- if (ignore_outlined && cfun->machine->call_ms2sysv
- && in_hard_reg_set_p (stub_managed_regs, DImode, regno))
- return false;
+ if (ignore_outlined && cfun->machine->call_ms2sysv)
+ {
+ /* Registers who's save & restore will be managed by stubs
called from
+ pro/epilogue. */
+ HARD_REG_SET stub_managed_regs;
+ xlogue_layout::compute_stub_managed_regs (stub_managed_regs);
+ if (in_hard_reg_set_p (stub_managed_regs, DImode, regno))
+ return false;
+ }
+
if (crtl->drap_reg
&& regno == REGNO (crtl->drap_reg)
&& !cfun->machine->no_drap_save_restore)
This makes no sense. The entire purpose of stub_managed_regs is to
cache the result of xlogue_layout::compute_stub_managed_regs() and this
would unnecessarily repeat that calculation for each time
ix86_save_reg() is called. Since
xlogue_layout::compute_stub_managed_regs() calls ix86_save_reg many
times, this makes it even worse.Which registers are being saved
out-of-line and inline MUST be known at the time the stack layout is
determined. So stub_managed_regsshould either be left a TU static or
just moved to struct machine_function.
As an aside, I've noticed that xlogue_layout::compute_stub_managed_regs
is calling ix86_save_reg (which isn't trivial) more often than it really
has to, so I've refactored it.
-/* Disables out-of-lined msabi to sysv pro/epilogues and emits a
warning if
- warn_once is null, or *warn_once is zero. */
-static void disable_call_ms2sysv_xlogues (const char *feature)
+/* Emits a warning for unsupported msabi to sysv pro/epilogues. */
+static void warn_once_call_ms2sysv_xlogues (const char *feature)
{
- cfun->machine->call_ms2sysv = false;
- warning (OPT_mcall_ms2sysv_xlogues, "not currently compatible with
%s.",
- feature);
+ static bool warned_once = false;
+ if (!warned_once)
+ {
+ warning (0, "-mcall-ms2sysv-xlogues is not compatible with %s",
+ feature);
+ warned_once = true;
+ }
}
We probably don't want to suppress all warnings across cases. I've got
a new version of this that takes a mask for the "warned":
static void disable_call_ms2sysv_xlogues (const char *feature, int warned_mask)
{
static int warned = 0;
cfun->machine->call_ms2sysv = false;
if (!(warned & warned_mask))
{
warning (OPT_mcall_ms2sysv_xlogues, "not currently compatible with %s.",
feature);
warned |= warned_mask;
}
}
/* When using -fsplit-stack, the allocation routines set a field in
@@ -12836,8 +12780,9 @@ ix86_builtin_setjmp_frame_value (void)
/* Fill structure ix86_frame about frame of currently computed
function. */
static void
-ix86_compute_frame_layout (struct ix86_frame *frame)
+ix86_compute_frame_layout (void)
{
+ struct ix86_frame *frame = &cfun->machine->frame;
struct machine_function *m = cfun->machine;
unsigned HOST_WIDE_INT stack_alignment_needed;
HOST_WIDE_INT offset;
@@ -12845,46 +12790,41 @@ static void
HOST_WIDE_INT size = get_frame_size ();
HOST_WIDE_INT to_allocate;
- CLEAR_HARD_REG_SET (stub_managed_regs);
-
/* m->call_ms2sysv is initially enabled in ix86_expand_call for all
64-bit
* ms_abi functions that call a sysv function. We now need to
prune away
* cases where it should be disabled. */
if (TARGET_64BIT && m->call_ms2sysv)
- {
- gcc_assert (TARGET_64BIT_MS_ABI);
- gcc_assert (TARGET_CALL_MS2SYSV_XLOGUES);
- gcc_assert (!TARGET_SEH);
+ {
+ gcc_assert (TARGET_64BIT_MS_ABI);
+ gcc_assert (TARGET_CALL_MS2SYSV_XLOGUES);
+ gcc_assert (!TARGET_SEH);
+ gcc_assert (TARGET_SSE);
+ gcc_assert (!ix86_using_red_zone ());
- if (!TARGET_SSE)
- m->call_ms2sysv = false;
+ if (crtl->calls_eh_return)
+ {
+ gcc_assert (!reload_completed);
+ m->call_ms2sysv = false;
+ warn_once_call_ms2sysv_xlogues ("__builtin_eh_return");
+ }
- /* Don't break hot-patched functions. */
- else if (ix86_function_ms_hook_prologue (current_function_decl))
- m->call_ms2sysv = false;
+ else if (ix86_static_chain_on_stack)
+ {
+ gcc_assert (!reload_completed);
+ m->call_ms2sysv = false;
+ warn_once_call_ms2sysv_xlogues ("static call chains");
+ }
- /* TODO: Cases not yet examined. */
- else if (crtl->calls_eh_return)
- disable_call_ms2sysv_xlogues ("__builtin_eh_return");
+ /* Finally, compute which registers the stub will manage. */
+ else
+ {
+ HARD_REG_SET stub_managed_regs;
+ unsigned count = xlogue_layout
+ ::compute_stub_managed_regs (stub_managed_regs);
+ m->call_ms2sysv_extra_regs = count - xlogue_layout::MIN_REGS;
+ }
+ }
- else if (ix86_static_chain_on_stack)
- disable_call_ms2sysv_xlogues ("static call chains");
-
- else if (ix86_using_red_zone ())
- disable_call_ms2sysv_xlogues ("red zones");
-
- else if (flag_split_stack)
- disable_call_ms2sysv_xlogues ("split stack");
-
- /* Finally, compute which registers the stub will manage. */
- else
- {
- unsigned count = xlogue_layout
- ::compute_stub_managed_regs (stub_managed_regs);
- m->call_ms2sysv_extra_regs = count - xlogue_layout::MIN_REGS;
- }
- }
-
@@ -29320,7 +29265,24 @@ ix86_expand_call (rtx retval, rtx fnaddr, rtx
call
/* Set here, but it may get cleared later. */
if (TARGET_CALL_MS2SYSV_XLOGUES)
- cfun->machine->call_ms2sysv = true;
+ {
+ if (!TARGET_SSE)
+ ;
+
+ /* Don't break hot-patched functions. */
+ else if (ix86_function_ms_hook_prologue (current_function_decl))
+ ;
+
+ /* TODO: Cases not yet examined. */
+ else if (flag_split_stack)
+ warn_once_call_ms2sysv_xlogues ("-fsplit-stack");
+
+ else
+ {
+ gcc_assert (!reload_completed);
+ cfun->machine->call_ms2sysv = true;
+ }
+ }
}
Other than the local compute_stub_managed_regs (which is a problem),
this looks like a very good improvement. It clarifies conditions that
will not change over the course of compiling a function (split stack,
ms_hook_prologue, etc.) and those that can.
After some thought, I've decided that it's not better to use a sorry()
to filter out -fsplit-stack combined with -march-ms2sysv-xlogues because
it would break support for a TU that uses both via function attributes.
Example:
void __attribute__((ms_abi))
a (void)
{
call_a_sysv_fn ();
/* stuff */
}
void __attribute__((optimize("-fsplit-stack")))
b(void)
{
/* stuff */
}
So you're fixes for this are better.
Thanks,
Daniel
