gmane.comp.gdb.patches

Re: [PATCH 5/7] PR gdb/15224 , Change the default set history filename to ~/.gdb_history.

Pedro Alves — 2013-05-22T19:39:18

Sorry, but simply calling changes gratuitous when I've made an
effort to explain why I believe they're good doesn't help.  :-/

The idea is that enabling the feature by default will expose
it to more users, who will benefit from it, most (educated-guessing
here, of course) not being aware GDB presently can already use
history from previous sessions.


The default has been to not save the history at all.

When weighing the pros and cons, I believe the pros outweigh the cons.

That's just my opinion, and I've just tried to clarify why I have it.

Re: [PATCH] Revised display-linkage-name

Eli Zaretskii — 2013-05-22T19:28:33

Thanks.


Do we really need this?  Why not display the whole name always?


Please remove the on|off part from the index entry.  Just the command
is enough.


Is "linker symbol name" how users would expect this to be called?  I
wouldn't.  Index entries should use phrases that readers might have in
mind when they are looking for the stuff described in this section.

I think the correct terminology is "linkage name".


Isn't it confusing to have 2 subtly different settings that actually
achieve almost the same?  How about a single setting, perhaps a
tristate, instead of 2 separate booleans?

           ^^^^^^^^^^^^^^^^^^^^^^^^
Copy/paste error?  You already have an identical index entry a few
lines above.


I think we don't need this setting.  After all, we show source-level
function names in their full glory, right?

Re: [PATCH 5/7] PR gdb/15224 , Change the default set history filename to ~/.gdb_history.

Eli Zaretskii — 2013-05-22T19:18:35

Since my opinions are being voted down "on a case by case basis", that
doesn't help me.  I thought maybe generalizing will, because all these
incompatibilities add up to a tendency that I think is dead wrong.


But .gdbinit in the current directory is no longer read by default, so
I can't, not without restoring the old behavior, which does involve
using a command that will cause old GDB's to barf.


Another gratuitous incompatibility.


Being the odd one out is not a reason good enough to change behavior
that was the default for a long time.

[PATCH] Revised display-linkage-name

Michael Eager — 2013-05-22T18:03:22

Attached is a revised patch to display the linkage name (the linker symbol)
along with the function name when setting, listing, or hitting a breakpoint.
This is a revision of http://sourceware.org/ml/gdb-patches/2013-03/msg00806.html.
(BTW, the gdb-patches mailing list archive doesn't show the original email from
3/18/13, quoted in the referenced message.)

Changes:
   Add NEWS and docs.
   Add linkage name to "info break" listing.
   Add MI annotations.
   Add command to set/show display length limit, remove define.
   Add test case.

There is a minor inconsistency where the linkage name is displayed.
When setting/hitting a breakpoint, the linkage name is displayed
within brackets following the source name, before the arguments.  In
the "info breakpoint" command, the linkage name is displayed in brackets
after the arguments.  This is because SYMBOL_PRINT_NAME(sym) returns a
complete function signature, not just the function name.

The test case was added to gdb.cp for convenience, since it uses
the C++ compiler.  The patch is not dependent on C++.

Tom -- can you verify that the MI support is correct?   I don't have
any way to test this.


gdb/
2013-05-23  Michael Eager <eager< at >eagercon.com>

       * NEWS: Announcement.
       * ada-lang.c: Update calls to find_frame_funname.
       * disasm.c: Likewise.
       * python/py-frame.c: Likewise.
       * annotate.c (annotate_linkage_name): New.
       * annotate.h (annotate_linkage_name): Declare.
       * breakpoint.c (print_breakpoint_location): Print linkage name.
       * defs.h (build_address_symbolic): Add linkname arg.
       * printcmd.c (print_address_symbolic): Print linkage name.
       (build_address_symbolic): Return linkage name.
       * stack.c (find_frame_funname): Return linkage name.
       (print_frame): Print linkage name.
       * stack.h (find_frame_funname): Update declaration.
       * top.c (display_linkage_name, display_linkage_name_len): New.
       (show_display_linkage_name, show_display_linkage_name_len): New cmds.
       * top.h (display_linkage_name, display_linkage_name_len): Declare.

gdb/doc
2013-05-23  Michael Eager <eager< at >eagercon.com>

       * gdb.texinfo: Add description.

gdb/testsuite
2013-05-23  Michael Eager <eager< at >eagercon.com>

       * gdb.cp/display-linkage-name.exp: New.
       * gdb.cp/display-linkage-name.cc: New.

Re: [PATCH 5/7] PR gdb/15224 , Change the default set history filename to ~/.gdb_history.

Pedro Alves — 2013-05-22T18:08:31

I understand the sentiment, but I'd rather not generalize, and look
at it on a case by case basis.  Scripts do have a means get the
previous behavior.  Simply adding this to .gdbinit:

 "set history filename .gdb_history"

get you the old behavior back.  And works the same with the
older GDBs too.  Note the user must already have added

 "set history save on"

to .gdbinit in previous releases to have history saving.
He'll just need to add one more line next to that.

If you're talking about getting the previous behavior of
not having history enabled, then the user can just add

 "set history save off"

to .gdbinit.


:-)

The reasoning for changing the default is that we (Pedro/Jan/Muhammad)
believe enabling history by default is a better default that having it
disabled by default, as currently.  Couple the fact that ".gdb_history"
is a dot/hidden file, with enabling history saving by default, and users
could end up with their filesystem littered with random hidden .gdb_history
files.  I think GDB shouldn't do that by default.  So in order to enable
history saving by default, we believe we should default to ~/.gdb_history
instead first.  bash also defaults to saving history under $HOME, and I'd
think most other interactive programs/shell do so too.  So it feels like
GDB is the odd one out here.

Re: [PATCH 1/7] PR gdb/15224 "set history filename" to by immediately converted to absolute path

Pedro Alves — 2013-05-22T17:51:29

Ah, good.

It would have been better to file a different PR for this
one, and then making 15224 depend on it. This is a preexisting
bug, after all, that can be triggered no matter what the
default history filename is.

This is incomplete. You must have done something with
the new function. :-)

I think you meant "with a relative path". But this tests more now.
Please make it complete.

This leaks the previous history_filename. You can use reconcat
instead to address that.

The gdb_test_multiple above only sets HOME on success. That means this will
explode with an access to an unknown symbol ($HOME) if the gdb_test_multiple
fails.

You can put the result of [file join $HOME foobar.baz] in a
variable and use that, instead of doing that twice:

set filename [file join $HOME foobar.baz]

Actually, "file join" uses the build system's directory separator, but we
want the host's. Actually^2, GDB always uses '/' here, so we can ignore
that and just do:

set filename "$HOME/foobar.baz"

Also,

+gdb_test "show history filename" "The filename in which to record the command history is \"[file join $HOME foobar.baz]\"..*" \
+ "show history filename ([file join $HOME foobar.baz])"

Don't put that "([file join $HOME foobar.baz])" in the gdb.sum message,
as that will make the message depend on system/whoever runs it.

Another instance of the same problem mentioned before. [pwd]
returns the current working directory on the build machine,
not the host's. Use gdb's "pwd", not tcl's.

As long as we're breaking the long line with a \, please do it
before "The filename" too, so the line ends up a little shorter.

Re: [PATCH 5/7] PR gdb/15224 , Change the default set history filename to ~/.gdb_history.

Eli Zaretskii — 2013-05-22T17:49:16

That's even more annoying, IMO.

I think gratuitous backward incompatibility is evil, especially in
programs, like GDB, where there are no good facilities for
version-specific scripting.  We are breaking scripts out there for no
good reason, and we aren't giving the users of those scripts _any_
means to get their previous behavior cleanly.

But I know I'm somehow in the minority here.  It's probably the age or
something.

Re: [PATCH 4/7] Disable history saving for testsuit

Pedro Alves — 2013-05-22T17:27:39

Double space after name.


Say:

* lib/gdb.exp: Add '-ex "set history save off"' to
INTERNAL_GDBFLAGS

Okay with those changes.

Re: [PATCH 5/7] PR gdb/15224 , Change the default set history filename to ~/.gdb_history.

Pedro Alves — 2013-05-22T17:24:13

Keep in mind that history saving isn't enabled by default.

Would you prefer if we did something like:

At startup time, if "set history filename" hasn't been
used (tracked with a new global flag), then check whether there's
a $cwd/.gdb_history file, and if so, output a warning stating that
it's no longer read by default.  The warning could also
suggest adding "set history filename .gdb_history" to .gdbinit
to get back the old behavior.

Thus if the user already has "set history filename ~/.gdb_history"
in .gdbinit she'll not see any warning.

But I'm really not sure it's worth the bother though.

Re: [PATCH 2/7] [PR gdb/15224] Unify interactivity tests to use input_from_terminal_p

Pedro Alves — 2013-05-22T17:16:16

OK.

Thanks,

Re: [PATCH] dwarf2read.c: Don't assume uint32_t is unsigned int on all hosts.

Pedro Alves — 2013-05-22T16:26:34


It seems to solve exactly the issue I assumed gettext had.
That is, I thought with this

  printf (_("%" PRIu32), u);

the translator would see either "%u" or "%lu" depending on
which host had been used for extracting the strings.

But it seems gettext treats PRIu32 etc. specially when extracting
the strings and the translator sees "%PRIu32".  IIUC, then that bit
of code I pointed at is part of the mechanism that converts the
literal "PRIu32" in the translated string to the system's %u etc.
specifier at gettext/_/run time, so the resulting translated message
can be passed on to the destination function.  I mean "printf"
in the example above.  I might have misunderstood though.

Re: [RFA] Fix DLL unload events in MinGW GDB

Eli Zaretskii — 2013-05-22T16:19:09

Committed to the trunk.


I decided that this is too much overhead for a problem that no one
else reported ;-)


Thanks.

Re: [PATCH] dwarf2read.c: Don't assume uint32_t is unsigned int on all hosts.

Doug Evans — 2013-05-22T16:16:18

I don't mind much how it's implemented.  Remembering that there is
puint32 for the purpose of printing uint32's will stick in my memory
more than having to use pulongest.

Re: [PATCH] dwarf2read.c: Don't assume uint32_t is unsigned int on all hosts.

Pedro Alves — 2013-05-22T16:10:33

OK.


Yeah, though I think those would end up implemented as calls to
decimal2str(...,ULONGEST,...) internally anyway.

With pulongest, we don't have to care the width of the variable,
only the sign.  I guess I could see that as an advantage, even over
PRIxxx.


Me too.

Re: [PATCH] dwarf2read.c: Don't assume uint32_t is unsigned int on all hosts.

Eli Zaretskii — 2013-05-22T16:07:35

This solves quite a different problem, then, right?

Re: [PATCH] dwarf2read.c: Don't assume uint32_t is unsigned int on all hosts.

Doug Evans — 2013-05-22T15:57:18

If it's doable, I'd prefer this to pulongest.
Another possibility, mentioned for reference sake, would be adding puint32, etc.
I'm ambivalent on what The Right fix is here.

Re: [PATCH] dwarf2read.c: Don't assume uint32_t is unsigned int on all hosts.

Pedro Alves — 2013-05-22T15:50:48

For the archives,

On 05/22/2013 04:34 PM, Pedro Alves wrote:


I found the thread where this support was originally discussed:

 http://sourceware.org/ml/libc-alpha/2002-07/msg00202.html

I found this one in particular enlightening:

 http://sourceware.org/ml/libc-alpha/2002-07/msg00208.html

Re: [PATCH] dwarf2read.c: Don't assume uint32_t is unsigned int on all hosts.

Pedro Alves — 2013-05-22T15:34:22

Yeah.

...

I actually meant this:

/* Expand a system dependent string segment.  Return NULL if unsupported.  */
static const char *
get_sysdep_segment_value (name)
     const char *name;
{
  /* Test for an ISO C 99 section 7.8.1 format string directive.
     Syntax:
     P R I { d | i | o | u | x | X }
     { { | LEAST | FAST } { 8 | 16 | 32 | 64 } | MAX | PTR }  */
  /* We don't use a table of 14 times 6 'const char *' strings here, because
     data relocations cost startup time.  */
  if (name[0] == 'P' && name[1] == 'R' && name[2] == 'I')
    {
      if (name[3] == 'd' || name[3] == 'i' || name[3] == 'o' || name[3] == 'u'
  || name[3] == 'x' || name[3] == 'X')
{
  if (name[4] == '8' && name[5] == '\0')
    {
      if (name[3] == 'd')
return PRId8;
      if (name[3] == 'i')
return PRIi8;
      if (name[3] == 'o')
return PRIo8;
      if (name[3] == 'u')

I peeked at some .mo files in my system, and I see "PRIu64", etc.
there, so it seems gettext knows these macros are special.

I couldn't find much about it in the gettext manual, but

 http://www.gnu.org/software/gettext/manual/gettext.html#Marking

does contain a couple references to PRIuMAX.

Re: [PATCH] dwarf2read.c: Don't assume uint32_t is unsigned int on all hosts.

Eli Zaretskii — 2013-05-22T15:07:01

They are all in the same single source file, at least in GDB 7.6.


No magic, just a lot of these:

  #if !defined PRIo32 || PRI_MACROS_BROKEN
  # undef PRIo32
  # define PRIo32 "o"
  #endif
  #if !defined PRIu32 || PRI_MACROS_BROKEN
  # undef PRIu32
  # define PRIu32 "u"
  #endif
  #if !defined PRId64 || PRI_MACROS_BROKEN
  # undef PRId64
  # define PRId64 (sizeof (long) == 8 ? "ld" : "lld")
  #endif
  #if !defined PRIi64 || PRI_MACROS_BROKEN
  # undef PRIi64
  # define PRIi64 (sizeof (long) == 8 ? "li" : "lli")
  #endif

etc.


Fine with me.  Thanks.

Re: contribution checklist in the wiki

Eli Zaretskii — 2013-05-22T14:56:09

Perhaps we should encourage contributors to point to URL of the
discussion thread.

Re: [RFA/Doco] Document new /system-gdbinit area

Eli Zaretskii — 2013-05-22T14:54:32

Thanks.


Yes, with one gotcha:


< at >cindex entries should begin with a lower-case letter, unless it's a
proper name or acronym.  (This is because mixed-case sorts differently
in different locales.)

Thanks.