RFA: fix in features/Makefile

RFA: fix in features/Makefile

[Tom Tromey]

When regenerating some files in features using features/Makefile, I
saw some strange control characters show up at the start of the
generated files.

This patch changes features/Makefile to use gdb's logging feature
rather than redirection to create the output files.  This fixed the
problem I saw.

This patch also makes it so that the .c files are always rebuilt in
response to a 'make'.  Without the FORCE code, make was not running
the rule for me.

Ok?


Also, it would be nice if someone documented which .xml->.c
translations are to be checked in.  I ran the rule with XMLTOC set to
all the .xml files under features, and I got a bunch of new files that
aren't in the repository.  IMO the canonical list ought to be set in
features/Makefile somewhere.

Tom

:ADDPATCH maintenance:

ChangeLog:
2008-08-24  Tom Tromey  <tromey@redhat.com>

	* features/Makefile (%.c): Depend on FORCE.  Use logging, not
	redirection, to create the output file.
	(FORCE): New target.
	(.PHONY): Likewise.

Index: features/Makefile
===================================================================
RCS file: /cvs/src/src/gdb/features/Makefile,v
retrieving revision 1.9
diff -u -r1.9 Makefile
--- features/Makefile	15 Aug 2008 15:18:33 -0000	1.9
+++ features/Makefile	24 Aug 2008 18:07:59 -0000
@@ -70,10 +70,19 @@
 	sh ../../move-if-change $(outdir)/$*.tmp $(outdir)/$*.dat
 
 cfiles: $(CFILES)
-%.c: %.xml
+%.c: %.xml FORCE
 	$(GDB) -nx -q -batch \
-	  -ex "set tdesc filename $<" -ex 'maint print c-tdesc' > $@.tmp
+	  -ex 'set logging overwrite on' \
+	  -ex "set logging file $@.tmp" \
+	  -ex 'set logging redirect on' \
+	  -ex "set tdesc filename $<" \
+	  -ex 'set logging on' \
+	  -ex 'maint print c-tdesc' \
+	  -ex 'set logging off'
 	sh ../../move-if-change $@.tmp $@
 
 # Other dependencies.
 $(outdir)/arm-with-iwmmxt.dat: arm-core.xml xscale-iwmmxt.xml
+
+.PHONY: cfiles
+FORCE:

Re: RFA: fix in features/Makefile

[Daniel Jacobowitz]

On Sun, Aug 24, 2008 at 12:11:03PM -0600, Tom Tromey wrote:
> When regenerating some files in features using features/Makefile, I
> saw some strange control characters show up at the start of the
> generated files.
> 
> This patch changes features/Makefile to use gdb's logging feature
> rather than redirection to create the output files.  This fixed the
> problem I saw.

But why did it?  If you can't redirect GDB's output, something's
wrong.

> This patch also makes it so that the .c files are always rebuilt in
> response to a 'make'.  Without the FORCE code, make was not running
> the rule for me.

If you're going to change the generating code, IMO it's reasonable to
remove and remake the generated files by hand...

> Also, it would be nice if someone documented which .xml->.c
> translations are to be checked in.  I ran the rule with XMLTOC set to
> all the .xml files under features, and I got a bunch of new files that
> aren't in the repository.  IMO the canonical list ought to be set in
> features/Makefile somewhere.

Sounds reasonable.  It's all the ones with a full target description
(rather than a single feature), I believe.

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFA: fix in features/Makefile

[Tom Tromey]

>>>>> "Daniel" == Daniel Jacobowitz <drow@false.org> writes:

Tom> This patch changes features/Makefile to use gdb's logging feature
Tom> rather than redirection to create the output files.  This fixed the
Tom> problem I saw.

Daniel> But why did it?  If you can't redirect GDB's output, something's
Daniel> wrong.

Yeah, I should have looked first.

I disabled stdout buffering and used frysk's ftrace to get a stack
trace at the point where the weird stuff is written:

#0 0x00110416 in __kernel_vsyscall() from [vdso]
#1 0x05e4d135 in new_do_write() from libc-2.7.so
#2 0x05e4d41f in _IO_do_write@@GLIBC_2.1() from libc-2.7.so
#3 0x05e4dd28 in _IO_file_overflow@@GLIBC_2.1() from libc-2.7.so
#4 0x05e50793 in __overflow() from libc-2.7.so
#5 0x05e4a55b in putc() from libc-2.7.so
#6 0x082f35c5 in _rl_output_character_function() from gdb
#7 0x006c554a in tputs() from libtinfo.so.5.6
#8 0x082f3741 in _rl_enable_meta_key() from gdb
#9 0x082df0d8 in readline_initialize_everything() from gdb
#10 0x082def5c in rl_initialize() from gdb
#11 0x081a6081 in tui_initialize_readline() from gdb
#12 0x081a6ab7 in tui_init() from gdb
#13 0x0820ba2b in interp_set() from gdb
#14 0x080a1a1b in captured_main() from gdb
#15 0x0820b6cc in catch_errors() from gdb
#16 0x080a20f8 in gdb_main() from gdb
#17 0x080a1027 in main() from gdb
#18 0x05e00390 in __libc_start_main() from libc-2.7.so
#19 0x080a0f21 in _start() from gdb

So, it is readline.

Maybe we could fool it by setting TERM=dumb or something.
I suppose -batch ought to do this kind of thing automatically.
I'll file a bug report soon.

Tom> This patch also makes it so that the .c files are always rebuilt in
Tom> response to a 'make'.  Without the FORCE code, make was not running
Tom> the rule for me.

Daniel> If you're going to change the generating code, IMO it's reasonable to
Daniel> remove and remake the generated files by hand...

Note that due to the use of move-if-change, you only get updates if
there really were any.  This change just makes it simpler to ask for
the changes.  I don't really care either way though.

Tom

Re: RFA: fix in features/Makefile

[Daniel Jacobowitz]

On Sun, Aug 24, 2008 at 06:02:40PM -0600, Tom Tromey wrote:
> Yeah, I should have looked first.
> 
> I disabled stdout buffering and used frysk's ftrace to get a stack
> trace at the point where the weird stuff is written:
> 
> #0 0x00110416 in __kernel_vsyscall() from [vdso]
> #1 0x05e4d135 in new_do_write() from libc-2.7.so
> #2 0x05e4d41f in _IO_do_write@@GLIBC_2.1() from libc-2.7.so
> #3 0x05e4dd28 in _IO_file_overflow@@GLIBC_2.1() from libc-2.7.so
> #4 0x05e50793 in __overflow() from libc-2.7.so
> #5 0x05e4a55b in putc() from libc-2.7.so
> #6 0x082f35c5 in _rl_output_character_function() from gdb
> #7 0x006c554a in tputs() from libtinfo.so.5.6
> #8 0x082f3741 in _rl_enable_meta_key() from gdb
> #9 0x082df0d8 in readline_initialize_everything() from gdb
> #10 0x082def5c in rl_initialize() from gdb
> #11 0x081a6081 in tui_initialize_readline() from gdb
> #12 0x081a6ab7 in tui_init() from gdb

stdout isn't a tty... dubious we should be doing this if it is.  I'm
not sure why you get tui here, though, I don't think I do.  Do you
have the gdbtui binary installed as gdb?

> Tom> This patch also makes it so that the .c files are always rebuilt in
> Tom> response to a 'make'.  Without the FORCE code, make was not running
> Tom> the rule for me.
> 
> Daniel> If you're going to change the generating code, IMO it's reasonable to
> Daniel> remove and remake the generated files by hand...
> 
> Note that due to the use of move-if-change, you only get updates if
> there really were any.  This change just makes it simpler to ask for
> the changes.  I don't really care either way though.

Makes sense, this makefile is already for manual use only and it's not
like they take a long time.

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFA: fix in features/Makefile

[Tom Tromey]

>>>>> "Daniel" == Daniel Jacobowitz <drow@false.org> writes:

Daniel> stdout isn't a tty... dubious we should be doing this if it is.  I'm
Daniel> not sure why you get tui here, though, I don't think I do.  Do you
Daniel> have the gdbtui binary installed as gdb?

I passed the full path to the just-built gdb to make.

I looked at config.status and I didn't pass any argument to configure
that would affect this.  I believe my build takes the enable_tui=auto
branch and then finds all the prereqs.  So, I think this could affect
anybody, depending on what devel packages they have installed.

Tom

Re: RFA: fix in features/Makefile

[Daniel Jacobowitz]

On Sun, Aug 24, 2008 at 10:03:11PM -0600, Tom Tromey wrote:
> >>>>> "Daniel" == Daniel Jacobowitz <drow@false.org> writes:
> 
> Daniel> stdout isn't a tty... dubious we should be doing this if it is.  I'm
> Daniel> not sure why you get tui here, though, I don't think I do.  Do you
> Daniel> have the gdbtui binary installed as gdb?
> 
> I passed the full path to the just-built gdb to make.
> 
> I looked at config.status and I didn't pass any argument to configure
> that would affect this.  I believe my build takes the enable_tui=auto
> branch and then finds all the prereqs.  So, I think this could affect
> anybody, depending on what devel packages they have installed.

Huh, you're right.  I get tui_init called too.

Ah, it's _rl_term_mm.  If your terminal has a meta character enable
sequence it will be output by tputs (smm/rmm).  Some versions of xterm
do.

Guess we shouldn't be initializing readline when stdout / stderr are
not a terminal...

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFA: fix in features/Makefile

[Tom Tromey]

>>>>> "Daniel" == Daniel Jacobowitz <drow@false.org> writes:

Daniel> Guess we shouldn't be initializing readline when stdout / stderr are
Daniel> not a terminal...

Here's one stab at a fix.

I built and regtested this on x86-64 (compile farm).  I also tried
rebuilding rs6000/rs6000.c before and after; this verified that before
the patch I get weird junk in the output, and after it works fine.

I wonder if this should be conditional on -batch, though, and not just
input_from_terminal_p.

Tom


2008-10-06  Tom Tromey  <tromey@redhat.com>

	* features/Makefile (%.c): Redirect gdb's stdin.
	* tui/tui.c (tui_initialize_readline): Return early if input is
	not from terminal.

diff --git a/gdb/features/Makefile b/gdb/features/Makefile
index 9241e2a..aecf280 100644
--- a/gdb/features/Makefile
+++ b/gdb/features/Makefile
@@ -72,7 +72,8 @@ $(outdir)/%.dat: %.xml number-regs.xsl sort-regs.xsl gdbserver-regs.xsl
 cfiles: $(CFILES)
 %.c: %.xml
 	$(GDB) -nx -q -batch \
-	  -ex "set tdesc filename $<" -ex 'maint print c-tdesc' > $@.tmp
+	  -ex "set tdesc filename $<" -ex 'maint print c-tdesc' \
+	  < /dev/null > $@.tmp
 	sh ../../move-if-change $@.tmp $@
 
 # Other dependencies.
diff --git a/gdb/tui/tui.c b/gdb/tui/tui.c
index 9710141..e0e9760 100644
--- a/gdb/tui/tui.c
+++ b/gdb/tui/tui.c
@@ -309,6 +309,9 @@ tui_initialize_readline (void)
   int i;
   Keymap tui_ctlx_keymap;
 
+  if (!input_from_terminal_p ())
+    return;
+
   rl_initialize ();
 
   rl_add_defun ("tui-switch-mode", tui_rl_switch_mode, -1);