PR 89394
* cp-demangle.c (cplus_demangle_fill_name): Reject negative
lengths.
(d_count_templates_scopes): Replace num_templates and num_scopes
parameters with a struct d_print_info pointer parameter. Adjust
body of the function accordingly. Add recursion counter and check
that the recursion limit is not reached.
(d_print_init): Pass dpi parameter to d_count_templates_scopes.
Reset recursion counter afterwards, unless the recursion limit was
reached.
This introduces a new iterator and range adapter for iteration over
the separate debug files of a given objfile. As in the current
approach, the requested objfile is returned first, followed by the
separate debug objfiles.
gdb/ChangeLog
2019-04-10 Tom Tromey <tom@tromey.com>
* symtab.c (lookup_global_symbol_from_objfile)
(lookup_symbol_in_objfile_from_linkage_name): Use the iterator.
* objfiles.h (class separate_debug_iterator): New.
(class separate_debug_range): New.
(struct objfile) <separate_debug_objfiles>: New method.
(objfile_separate_debug_iterate): Don't declare.
* objfiles.c (separate_debug_iterator::operator++): Rename from
objfile_separate_debug_iterate.
(objfile_relocate, objfile_rebase, objfile_has_symbols): Use the
iterator.
* minsyms.c (lookup_minimal_symbol_by_pc_section): Use the
iterator.
While working on objfiles I noticed a typo in one comment, and another
comment that, as far as I can tell, has been obsolete for a very long
time.
gdb/ChangeLog
2019-04-10 Tom Tromey <tom@tromey.com>
* symfile.c (reread_symbols): Remove old comment.
* objfiles.c (free_all_objfiles): Fix a typo.
The "object_files" macro is sometimes used when iterating over
objfiles. This patch removes a few such uses in favor of the new
range adapter.
gdb/ChangeLog
2019-04-10 Tom Tromey <tom@tromey.com>
* ia64-tdep.c (ia64_get_dyn_info_list): Use foreach.
* minsyms.c (lookup_minimal_symbol): Use foreach.
(lookup_minimal_symbol_text, lookup_minimal_symbol_by_pc_name)
(lookup_minimal_symbol_solib_trampoline): Likewise.
* symfile.c (reread_symbols): Use foreach.
The fix H.J. implemented for PR gas/22791 in the thread starting at
[PATCH] x86-64: Treat PC32 relocation with branch as PLT32
https://sourceware.org/ml/binutils/2018-02/msg00065.html
is causing problems on Solaris/x86. The native linker is strongly
preferred there, and there's no intention of implementing the linker
optimization he plans there. Besides, the kernel runtime linker,
otherwise has no need to deal with that reloc at all, and instead of
adding (possibly even more) workarounds with no benefit, it seems
appropriate to disable the R_X86_64_PLT32 generation as branch marker on
Solaris/x86 in the first place.
The patch itself is trivial, the only complication is adapting the
testsuite. Since I've found no way to have conditional sections in the
.d files, I've instead used the solution already found elsewhere of
having separate .d files for the affected tests in an i386/solaris
subdirectory and skipping the original ones.
Tested on amd64-pc-solaris2.11 and x86_64-pc-linux-gnu without
regressions.
* config/tc-i386.c (need_plt32_p) [TE_SOLARIS]: Return FALSE.
* testsuite/gas/i386/solaris/solaris.exp: New driver.
* testsuite/gas/i386/solaris/reloc64.d,
testsuite/gas/i386/solaris/x86-64-jump.d,
testsuite/gas/i386/solaris/x86-64-mpx-branch-1.d,
testsuite/gas/i386/solaris/x86-64-mpx-branch-2.d,
testsuite/gas/i386/solaris/x86-64-nop-3.d,
testsuite/gas/i386/solaris/x86-64-nop-4.d,
testsuite/gas/i386/solaris/x86-64-nop-5.d,
testsuite/gas/i386/solaris/x86-64-relax-2.d,
testsuite/gas/i386/solaris/x86-64-relax-3.d: New tests.
* testsuite/gas/i386/reloc64.d,
testsuite/gas/i386/x86-64-jump.d,
testsuite/gas/i386/x86-64-mpx-branch-1.d,
testsuite/gas/i386/x86-64-mpx-branch-2.d,
testsuite/gas/i386/x86-64-nop-3.d,
testsuite/gas/i386/x86-64-nop-4.d,
testsuite/gas/i386/x86-64-nop-5.d,
testsuite/gas/i386/x86-64-relax-2.d,
testsuite/gas/i386/x86-64-relax-3.d: Skip on *-*-solaris*.
This fixes the csky-elf ld-elf/pr21884 and ld-unique/pr21529 failures,
by disabling branch stubs for binary (and other non-csky) output.
The csky-linux target gets branch stubs off by default because
presumably there are reasons why branch stubs were disabled, but
rather than killing the support completely it can be enabled by
--branch-stub.
* emultempl/cskyelf.em (csk_elf_before_parse): New function,
setting use_branch_stub false for linux.
(csky_elf_create_output_section_statements): Do emit this
function and all others in the file for linux, plus the branch
option control. Disable branch stubs when non-ELF.
In looking at the csky-elf vs. csky-linux differences, the first thing
I compared was csky_elf.sh and cskyelf_linux.sh. Those files are
mostly the same but besides the real differences, annoyingly have some
lines ordered differently. It's better in such cases to have one file
source the other, making differences plain. This patch does that for
csky and microblaze, removes an unused variable defined in a few
places, and fixes ld makefile dependencies.
* Makefile.am (eskyelf.c, eskyelf_linux.c): Depend on cskyelf.em.
(ecskyelf_linux.c): Depend on cskyelf.sh.
(eelf32microblazeel.c): Depend on elf32microblaze.sh.
* Makefile.in: Regenerate.
* emulparams/cskyelf.sh: Comment regarding cskelf_linux.sh.
(PAGE_SIZE): Don't define.
* emulparams/cskyelf_linux.sh: Source sckyelf.sh, leaving just
the differing variable defs/undefs.
* emulparams/elf32mcore.sh (PAGE_SIZE): Don't define.
* emulparams/elf32microblaze.sh: Comment re. elf32microblazeel.sh.
(OUTPUT_FORMAT): Use BIG_OUTPUT_FORMAT.
(PAGE_SIZE): Don't define.
* emulparams/elf32microblazeel.sh: Source elf32microblaze.sh,
leaving just the differing OUTPUT_FORMAT.
This patch is aimed a fixing large numbers of x86_64-cloudabi failures
caused by gas selecting the wrong target name.
* config/te-cloudabi.h: New file.
* config/tc-aarch64.c (aarch64_after_parse_args): Use TE_CLOUDABI
rather than TARGET_OS to select cloudabi.
* config/tc-i386.h (ELF_TARGET_FORMAT64): Define for TE_CLOUDABI.
* configure.tgt (*-*-cloudabi*): Set em=cloudabi.
PR rust/24414 points out that the Rust lexer uses strtoul when lexing
an integer, and that this can give the wrong results in some
situations.
This patch changes it to use strtoulst, like most of the rest of gdb.
It also adds a self test.
Tested on x86-64 Fedora 29 using an i686 build.
gdb/ChangeLog
2019-04-09 Ivan Begert <ivanbegert@gmail.com>
Tom Tromey <tromey@adacore.com>
PR rust/24414:
* rust-exp.y (rust_parser::lex_number): Use strtoulst.
(rust_lex_int_test): Change "value" to be LONGEST.
(rust_lex_tests): Add test for long integer literal.
I noticed that find_thread_in_random duplicates the code in
find_thread_in_random, so this patch changes the latter to use the
former.
There are two other spots in gdb that do this, but to unify all of
them would require switching some code from using the "iterate over"
idiom to using iterators.
Another possible improvement is that find_thread_in_random could be
made single-pass using reservoir sampling.
Tested by the buildbot.
gdb/gdbserver/ChangeLog
2019-04-09 Tom Tromey <tromey@adacore.com>
* linux-low.c (select_event_lwp): Use find_thread_in_random.
I noticed a few spots where fake_pid_p is handled as an int, whereas
the field in struct inferior has type bool. This patch changes the
remaining places to use bool as well.
Tested by the buildbot.
gdb/ChangeLog
2019-04-09 Tom Tromey <tromey@adacore.com>
* remote.c (remote_target::remote_add_inferior): Change fake_pid_p
to bool.
(extended_remote_target::attach): Update.
(remote_target::remote_notice_new_inferior): Update.
(remote_target::add_current_inferior_and_thread): Update.
* inferior.c (exit_inferior_1): Use "false".
* corelow.c (add_to_thread_list): Make fake_pid_p bool.
When using the "start" command, GDB puts a temporary breakpoint on the
"main" symbol (we literally invoke the tbreak command). However, since
it does wild matching by default, it also puts a breakpoint on any C++
method or "main" function in a namespace. For example, when debugging
GDB, it creates a total of 24 locations:
(gdb) start
Temporary breakpoint 1 at 0x198c1e9: main. (24 locations)
as there are a bunch of methods called main in the selftests, such as
selftests::string_view::capacity_1::main()
If such method was called in the constructor of a global object, or a
function marked with the attribute "constructor", then we would stop at
the wrong place. Also, this causes a few extra symtabs (those that
contain the "wrong" mains) to be expanded for nothing.
The dummiest, most straightforward solution is to add -qualified when
invoking tbreak. With this patch, "start" creates a single-location
breakpoint, as expected.
I copied the start.exp test to start-cpp.exp and made it use a C++ test
file, which contains two main functions. The new test verifies that the
output of "start" is the output we get when we set a single-location
breakpoint.
gdb/ChangeLog:
* infcmd.c (run_command_1): Pass -qualified to tbreak when usind
the "start" command.
gdb/testsuite/ChangeLog:
* gdb.base/start-cpp.exp: New file.
* gdb.base/start-cpp.cc: New file.
In Release 6 of the MIPS architecture [1], instruction RDHWR supports
a 3rd operand to serve as the 3-bit select field for the hardware
register.
[1] "MIPS Architecture for Programmers Volume II-A: The MIPS32
Instruction Set Manual", Imagination Technologies Ltd., Document
Number: MD00086, Revision 6.06, December 15, 2016, Section 3.2
"Alphabetical List of Instructions", pp. 332-334
opcodes/
* mips-opc.c (mips_builtin_opcodes): Add RDHWR rt rd sel.
gas/
* testsuite/gas/mips/mips.exp: Run hwr-names test.
* testsuite/gas/mips/hwr-names.s: Add test cases for RDHWR with
the SEL field.
* testsuite/gas/mips/mipsr6@hwr-names.d: New file.
This renaming was done to stay consistent with the naming of the new
gdb.InferiorThread.handle method. I had initially named it "thread_handle"
but Tom Tromey suggested just "handle".
The old name (thread_from_thread_handle) still works, but is marked as
deprecated in comments in the code as well as in the documentation.
I have some code which uses these functions. I very much like the
brevity of the new names.
gdb/doc/ChangeLog:
* python.texi (Inferiors In Python): Rename
Inferior.thread_from_thread_handle to Inferior.thread_from_handle.
Add note about the former being deprecated.
gdb/ChangeLog:
* python/py-inferior.c (infpy_thread_from_thread_handle):
Adjust comments to reflect renaming of thread_from_thread_handle
to thread_from_handle. Adjust keywords. Fix type error message.
(inferior_object_methods): Add thread_from_handle. Retain
thread_from_thread_handle, but mark it as deprecated.
testsuite/ChangeLog:
* gdb.python/py-thrhandle.exp: Adjust tests to call
thread_from_handle instead of thread_from_thread_handle.
InferiorThread.handle() returns a python bytes object. We'd like to
be able to pass the output of this function, a thread handle, to
Inferior.thread_from_thread_handle(). Up to now,
thread_from_thread_handle() expects to receive a gdb.Value input.
This commit adds support to also allow a python buffer object to be
passed as the handle.
infpy_thread_from_thread_handle() calls find_thread_by_handle() which
has the obvious functionality. It used to pass the thread handle via
a struct value pointer. I've revised this interface to instead pass a
gdb::array_view<const gdb_byte> object. (Thanks to Tom Tromey for
suggesting this data structure over an earlier version which passed a
gdb_byte pointer and length.)
gdb/ChangeLog:
* gdbthread.h (find_thread_by_handle): Revise declaration.
* thread.c (find_thread_by_handle): Likewise. Adjust
implementation too.
* python/py-inferior.c (infpy_thread_from_thread_handle): Add
support for buffer objects as handles.
- Explicitly include <string> for std::string.
- Use std::make_shared to construct gdb_exception::message instead of
operator new, avoiding one heap allocation (2 instead of 3). Add
'const char *fmt, va_list ap' parameters to
gdb_exception{,error,quit}'s ctors, and do the std::make_shared in
the gdb_exception ctor.
- gdb_exception_error's constructor does not need to have an 'enum
return_reason' parameter, since it is always RETURN_ERROR, by
definition.
- Similarly, gdb_exception_quit's contructor does not need to have
'enum return_reason'/'enum errors' parameters.
- In the gdb_exception_{quit,_error} ctors that take a gdb_exception
as argument, assert that they're being passed a gdb_exception object
of the right 'reason'.
gdb/ChangeLog:
2019-04-08 Pedro Alves <palves@redhat.com>
* common/common-exceptions.c (throw_exception): Don't create
named object to throw; throw directly.
(throw_it): Likewise. Don't initialize gdb_exception::message
here, with new; pass FMT and AP to the ctor instead.
* common/common-exceptions.h: Include <string>.
(gdb_exception::gdb_exception(enum return_reason, enum errors,
const char *, va_list)): New ctor. Use std::make_shared.
(gdb_exception_error::gdb_exception_error(enum return_reason, enum
errors)): Delete.
(gdb_exception_error::gdb_exception_error(enum errors, const char
*, va_list)): New.
(gdb_exception_error::gdb_exception_error(const gdb_exception &)):
Add assertion.
(gdb_exception_quit::gdb_exception_quit(enum return_reason, enum
errors)): Delete.
(gdb_exception_quit::gdb_exception_quit(const char *, va_list)): New.
(gdb_exception_quit::gdb_exception_quit(const gdb_exception &)):
Add assertion.
1. Use single entry for vcvtne2ps2bf16 and vdpbf16ps with Disp8ShiftVL.
2. Use 5 entries, instead of 8, for vcvtneps2bf16.
* i386-opc.tbl: Consolidate AVX512 BF16 entries.
* i386-init.h: Regenerated.
This makes exception throwing a bit more efficient, by removing some
copies.
gdb/ChangeLog
2019-04-08 Tom Tromey <tom@tromey.com>
* common/common-exceptions.c (throw_exception): Rename from
throw_exception_cxx. Remove old copy. Make argument const.
(throw_it): Create and throw exception objects directly.
* common/common-exceptions.h (throw_exception): Make argument
const.
(struct gdb_exception_error): Add constructor.
(struct gdb_exception_quit): Add constructor.
After the rewriting to use try/catch, some of the exception code is
now unused. This patch removes that code.
gdb/ChangeLog
2019-04-08 Tom Tromey <tom@tromey.com>
* common/common-exceptions.h (exception_rethrow): Don't declare.
(TRY_SJLJ): Update comment.
(TRY, CATCH, END_CATCH): Remove.
* common/common-exceptions.c (exception_rethrow): Remove.
This rewrites gdb's TRY/CATCH to plain C++ try/catch. The patch was
largely written by script, though one change (to a comment in
common-exceptions.h) was reverted by hand.
gdb/ChangeLog
2019-04-08 Tom Tromey <tom@tromey.com>
* xml-support.c: Use C++ exception handling.
* x86-linux-nat.c: Use C++ exception handling.
* windows-nat.c: Use C++ exception handling.
* varobj.c: Use C++ exception handling.
* value.c: Use C++ exception handling.
* valprint.c: Use C++ exception handling.
* valops.c: Use C++ exception handling.
* unittests/parse-connection-spec-selftests.c: Use C++ exception
handling.
* unittests/cli-utils-selftests.c: Use C++ exception handling.
* typeprint.c: Use C++ exception handling.
* tui/tui.c: Use C++ exception handling.
* tracefile-tfile.c: Use C++ exception handling.
* top.c: Use C++ exception handling.
* thread.c: Use C++ exception handling.
* target.c: Use C++ exception handling.
* symmisc.c: Use C++ exception handling.
* symfile-mem.c: Use C++ exception handling.
* stack.c: Use C++ exception handling.
* sparc64-linux-tdep.c: Use C++ exception handling.
* solib.c: Use C++ exception handling.
* solib-svr4.c: Use C++ exception handling.
* solib-spu.c: Use C++ exception handling.
* solib-frv.c: Use C++ exception handling.
* solib-dsbt.c: Use C++ exception handling.
* selftest-arch.c: Use C++ exception handling.
* s390-tdep.c: Use C++ exception handling.
* rust-lang.c: Use C++ exception handling.
* rust-exp.y: Use C++ exception handling.
* rs6000-tdep.c: Use C++ exception handling.
* rs6000-aix-tdep.c: Use C++ exception handling.
* riscv-tdep.c: Use C++ exception handling.
* remote.c: Use C++ exception handling.
* remote-fileio.c: Use C++ exception handling.
* record-full.c: Use C++ exception handling.
* record-btrace.c: Use C++ exception handling.
* python/python.c: Use C++ exception handling.
* python/py-value.c: Use C++ exception handling.
* python/py-utils.c: Use C++ exception handling.
* python/py-unwind.c: Use C++ exception handling.
* python/py-type.c: Use C++ exception handling.
* python/py-symbol.c: Use C++ exception handling.
* python/py-record.c: Use C++ exception handling.
* python/py-record-btrace.c: Use C++ exception handling.
* python/py-progspace.c: Use C++ exception handling.
* python/py-prettyprint.c: Use C++ exception handling.
* python/py-param.c: Use C++ exception handling.
* python/py-objfile.c: Use C++ exception handling.
* python/py-linetable.c: Use C++ exception handling.
* python/py-lazy-string.c: Use C++ exception handling.
* python/py-infthread.c: Use C++ exception handling.
* python/py-inferior.c: Use C++ exception handling.
* python/py-gdb-readline.c: Use C++ exception handling.
* python/py-framefilter.c: Use C++ exception handling.
* python/py-frame.c: Use C++ exception handling.
* python/py-finishbreakpoint.c: Use C++ exception handling.
* python/py-cmd.c: Use C++ exception handling.
* python/py-breakpoint.c: Use C++ exception handling.
* python/py-arch.c: Use C++ exception handling.
* printcmd.c: Use C++ exception handling.
* ppc-linux-tdep.c: Use C++ exception handling.
* parse.c: Use C++ exception handling.
* p-valprint.c: Use C++ exception handling.
* objc-lang.c: Use C++ exception handling.
* mi/mi-main.c: Use C++ exception handling.
* mi/mi-interp.c: Use C++ exception handling.
* mi/mi-cmd-stack.c: Use C++ exception handling.
* mi/mi-cmd-break.c: Use C++ exception handling.
* main.c: Use C++ exception handling.
* linux-thread-db.c: Use C++ exception handling.
* linux-tdep.c: Use C++ exception handling.
* linux-nat.c: Use C++ exception handling.
* linux-fork.c: Use C++ exception handling.
* linespec.c: Use C++ exception handling.
* language.c: Use C++ exception handling.
* jit.c: Use C++ exception handling.
* infrun.c: Use C++ exception handling.
* infcmd.c: Use C++ exception handling.
* infcall.c: Use C++ exception handling.
* inf-loop.c: Use C++ exception handling.
* i386-tdep.c: Use C++ exception handling.
* i386-linux-tdep.c: Use C++ exception handling.
* guile/scm-value.c: Use C++ exception handling.
* guile/scm-type.c: Use C++ exception handling.
* guile/scm-symtab.c: Use C++ exception handling.
* guile/scm-symbol.c: Use C++ exception handling.
* guile/scm-pretty-print.c: Use C++ exception handling.
* guile/scm-ports.c: Use C++ exception handling.
* guile/scm-param.c: Use C++ exception handling.
* guile/scm-math.c: Use C++ exception handling.
* guile/scm-lazy-string.c: Use C++ exception handling.
* guile/scm-frame.c: Use C++ exception handling.
* guile/scm-disasm.c: Use C++ exception handling.
* guile/scm-cmd.c: Use C++ exception handling.
* guile/scm-breakpoint.c: Use C++ exception handling.
* guile/scm-block.c: Use C++ exception handling.
* guile/guile-internal.h: Use C++ exception handling.
* gnu-v3-abi.c: Use C++ exception handling.
* gdbtypes.c: Use C++ exception handling.
* frame.c: Use C++ exception handling.
* frame-unwind.c: Use C++ exception handling.
* fbsd-tdep.c: Use C++ exception handling.
* f-valprint.c: Use C++ exception handling.
* exec.c: Use C++ exception handling.
* event-top.c: Use C++ exception handling.
* event-loop.c: Use C++ exception handling.
* eval.c: Use C++ exception handling.
* dwarf2read.c: Use C++ exception handling.
* dwarf2loc.c: Use C++ exception handling.
* dwarf2-frame.c: Use C++ exception handling.
* dwarf2-frame-tailcall.c: Use C++ exception handling.
* dwarf-index-write.c: Use C++ exception handling.
* dwarf-index-cache.c: Use C++ exception handling.
* dtrace-probe.c: Use C++ exception handling.
* disasm-selftests.c: Use C++ exception handling.
* darwin-nat.c: Use C++ exception handling.
* cp-valprint.c: Use C++ exception handling.
* cp-support.c: Use C++ exception handling.
* cp-abi.c: Use C++ exception handling.
* corelow.c: Use C++ exception handling.
* completer.c: Use C++ exception handling.
* compile/compile-object-run.c: Use C++ exception handling.
* compile/compile-object-load.c: Use C++ exception handling.
* compile/compile-cplus-symbols.c: Use C++ exception handling.
* compile/compile-c-symbols.c: Use C++ exception handling.
* common/selftest.c: Use C++ exception handling.
* common/new-op.c: Use C++ exception handling.
* cli/cli-script.c: Use C++ exception handling.
* cli/cli-interp.c: Use C++ exception handling.
* cli/cli-cmds.c: Use C++ exception handling.
* c-varobj.c: Use C++ exception handling.
* btrace.c: Use C++ exception handling.
* breakpoint.c: Use C++ exception handling.
* break-catch-throw.c: Use C++ exception handling.
* arch-utils.c: Use C++ exception handling.
* amd64-tdep.c: Use C++ exception handling.
* ada-valprint.c: Use C++ exception handling.
* ada-typeprint.c: Use C++ exception handling.
* ada-lang.c: Use C++ exception handling.
* aarch64-tdep.c: Use C++ exception handling.
gdb/gdbserver/ChangeLog
2019-04-08 Tom Tromey <tom@tromey.com>
* server.c: Use C++ exception handling.
* linux-low.c: Use C++ exception handling.
* gdbreplay.c: Use C++ exception handling.
Now that cleanups have been removed, TRY/CATCH can't be SJLJ-based any
more. This patch simplifies the exception handling code, by removing
the non-working variants.
Note that the "pure" C++ exception handling code is removed as well; I
think the route forward must be to change exceptions to be
self-destructing, so that try_scope_depth can simply be removed.
Some longjmp-based code remains, as it is needed to throw an exception
through readline.
gdb/ChangeLog
2019-04-08 Tom Tromey <tom@tromey.com>
* common/common-exceptions.h (GDB_XCPT_SJMP, GDB_XCPT_TRY)
(GDB_XCPT_RAW_TRY, GDB_XCPT): Remove.
(TRY, CATCH, END_CATCH): Remove some definitions.
* common/common-exceptions.c: Don't use GDB_XCPT.
(catcher_list_size): Remove.
(throw_exception, throw_it): Simplify.
x86_64-rdos is one of the few x86_64 targets that is 64-bit only and
the x86_64 configure entries don't depend on elf-vxworks.lo. This
results in undefined references from elfxx-x86.o.
* configure.ac (elfxx_x86): Define and use.
* configure: Regenerate.
I've broken "make info" a couple of times now, because I sometimes
forget to run "make info" after modifying a Texinfo file.
I don't know why gdb's "make all" doesn't build the info pages. I
suspect this was some Cygnus-local oddity back in the day.
This patch changes doc/Makefile.in so that the info pages are built by
"make all". As a point of reference, Automake has essentially always
worked this way. According to the Automake manual (I didn't
double-check) this is required by the GNU coding standards.
The first time I sent this patch, I mentioned that I wanted to look
into some existing bugs in bugzilla about missing "makeinfo".
However, today I tried and I discovered that BFD requires makeinfo,
and builds its info file as part of "all". So, I think this change
doesn't worsen the situation for users in any way, and can simply go
in.
gdb/doc/ChangeLog
2019-04-07 Tom Tromey <tom@tromey.com>
* Makefile.in (all): Depend on "info".
* ppc-dis.c (print_insn_powerpc): Use a tiny state machine
op_separator to control printing of spaces, comma and parens
rather than need_comma, need_paren and spaces vars.
