This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH, updated] Add support for setting disassembler-options in GDB for POWER, ARM and S390


On 2/14/17 2:01 PM, Pedro Alves wrote:
 - Maintain the intended git commit log as an integral part
   of the patch, and include it in patch re-posts, so that
   any revision of the patch can be reviewed as a
   self-contained entity.  There's probably some rationale for
   some changes to the tests that is written down in some
   earlier intro, but was lost meanwhile.

I'm not a git expert, as I'm paid to work on gcc and we still use
subversion.  I am willing to learn though, so can you explain what
you mean by the above?


 - For each new revision of the patch, bump a v2, v3, etc.
   revision number in the email subject, so that's easier
   to find specific revisions, and to identify whic
   email contains the latest version.

Easily done, as I've been doing just that internally.
I'm frightened to say that I'm at v25 and counting. :-(


+manual and/or the output of @kbd{objdump --help}.  The default value is ''.

'' renders as a single double-quote, not two single-quotes.  In any
case, I'd suggest saying instead "The default is the empty string", or
"The default is not options".  Likewise in NEWS.

Done.


@@ -888,6 +896,8 @@ gdb_buffered_insn_length_init_dis (struct gdbarch *gdbarch,
   di->endian = gdbarch_byte_order (gdbarch);
   di->endian_code = gdbarch_byte_order_for_code (gdbarch);

+  di->application_data = gdbarch;

Is this above used anywhere?

I think this might be left over from something I was trying and forgot
to remove.  I'll remove it.




+  char *options = remove_whitespace_and_extra_commas (prospective_options);
+  char *iter, opt[256];

Can we get rid of the hardcoded (and not enforced) limit?
Maybe just use strtok_r instead of FOR_EACH_DISASSEMBLER_OPTION?

+  /* Verify we have valid disassembler options.  */
+  FOR_EACH_DISASSEMBLER_OPTION (opt, iter, options)

The problem with strtok{,_r} is that it is destructive to the option
string, so we'd have to dup the string before scanning it, which
doesn't seem very elegant either.  We would also need to remember
to free the dup'd string as well when we were done with it.

The reason I copied the parsed option into a char array was that I
needed a null terminated string that I could use with strcmp.
Unfortunately, the POWER port has several options that have a
common prefix:

  Eg: "e500" & "e500mc", "ppc" & "ppc32" and "ppc64", etc.

...which strncmp cannot disambiguate, because it doesn't enforce the
two strings have the same length.

I have two ideas, one is to write our own strcmp that treats option
delimiters like ',' just like '\0'.  The other idea would be to
modify the disassembler_options string so that we use '\0' as the
delimiter between the different options.  We'd need an extra '\0'
at the end to know when we've run out of options though.  If we
did this, then we could just use standard strcmp on the options.


+/* A completion function for "set disassembler".  */
+
+static VEC (char_ptr) *
+disassembler_options_completer (struct cmd_list_element *ignore,
+				const char *text, const char *word)
+{
+  struct gdbarch *gdbarch = get_current_arch ();
+  const disasm_options_t *opts = gdbarch_valid_disassembler_options (gdbarch);
+
+  if (opts != NULL)
+    {
+      /* Only attempt to complete on the last option text.  */
+      const char *separator = strrchr (text, ',');
+      if (separator != NULL)
+	text = separator + 1;

I believe 'word' points past the comma already?

'word' does point past the last comma, but sometimes, it points well
past the last comma.  For example, if I type:

  set disassembler-options force-thumb, reg-name-g<tab>

...then 'text' will equal "force-thumb, reg-name-g" and 'word'
will equal "g".  To get the completer to match "reg-names-gcc",
I have to modify 'text' to be "reg-names-g".


+      while (ISSPACE (*text))
+	text++;

skip_spaces_const.

Done.



+  /* Add the command that controls the disassembler options.  */
+  cmd = add_setshow_string_noescape_cmd ("disassembler-options", no_class,
+					 &prospective_options, _("\
+Set the disassembler options.\n\
+Usage: set disassembler <option>[,<option>...]\n\n\
+See: show disassembler' for valid option values.\n"), _("\

Spell out "set/show disassembler-options" in full.

Ahh, I forgot to modify this once we changed the name of the command.
Thanks for catching that.



+extern void set_disassembler_options (char *);

Need intro comment.

Done.



...how about avoiding the hard coded buffer size:

  std::string long_name = std::string ("reg-names-") + disassembly_style;
  set_disassembler_options (&long_name[0]);

Nice, done.


+static void
+show_disassembly_style_sfunc (struct ui_file *file, int from_tty,
+			      struct cmd_list_element *c, const char *value)
 {
-  set_disassembly_style ();
+  struct gdbarch *gdbarch = get_current_arch ();
+  const char *options = gdbarch_disassembler_options (gdbarch);
+  char *style = "default";

Why "default" instead of empty?

That might be left over old code too.  Changed to "".



+++ b/gdb/testsuite/gdb.arch/powerpc-altivec.exp
@@ -0,0 +1,257 @@
+# Copyright 2016 Free Software Foundation, Inc.

2016-2017.  Actually, if you're largely copying the file contents
from some other file, you'll need to preserve the original
copyright years in the new file (and include 2017).

The original was 2014-..., so I changed it to 2014-2017.



Please use an updated GPLv3 copyright header.  Looks like a couple tests
sneaked in with the old v2 header.

I just copied in the header used by powerpc-power.exp (ie, the huge test
case I broke up) and it had the old copyright header version.  I have
updated the tests to use version 3. Thanks for noticing.




+gdb_test "set disassembler-options altivec"
+set test "disass func"
+gdb_test_multiple $test $test {
+    -re "\r\nDump of assembler code for function func:(\r\n.*\r\n)End of assembler dump.\r\n$gdb_prompt $" {
+	set func $expect_out(1,string)
+	pass $test
+    }
+}

If this test fails, $func is left unset, and then the following
tests that use it error out with a tcl error.  Set it to
some bogus default before calling gdb_test_multiple.

Ok, I'll do that.


I understand that you're largely copying the mechanism
from an existing test, but, I should mention that this extracting
the function disassembly in one go seems fragile -- at some point
this can grow enough to overflow expect's buffer.

I'm just using the mechanism that the original HUGE test case used.
The fact that I'm breaking that test case up into many much smaller
test cases is an improvement and makes overflowing expect's buffer
less likely with the patch than before the patch.



How about instead using gdb_test_sequence ?  Something like:

set insn_sequence_re ""

foreach insn {
  "dss     3"
  "dssall"
  "dst     r5,r4,1"
  ....
} {
    lappend insn_list_re [instr_to_patt $insn]
}

gdb_test_sequence "some test name" "" $insn_list_re

You could even go a step further and extract the
instruction list from the .s file, since you already
have the disassembled instructions written there:

 +	.long  0x117e0001    /* vmul10cuq v11,v30          */
 +	.long  0x13c1b807    /* vcmpneb v30,v1,v23         */

That'd result in smaller .exp files, and truly only one
place to update the tests whenever they need to change.

Bonus would be to use "disassemble /r" and match the raw opcodes
as well.

That's sounds great, but I'm afraid I have no idea how to do the
above and I don't see any examples like it in the testsuite to copy.
Unless you can show me a lot more context to your idea, I'm afraid
I'm going to have to leave the test cases as they are.




+proc instr_to_patt {instr} {
+    # 0x0000000000000018 <+24>:	stxvd2x vs43,r4,r5
+    return ".*\r\n\[ \t\]*0x\[0-9a-f\]+ <\\+\[0-9a-f\]*>:\[ \t\]*[string map [list { } "\[ \t\]+" . {\.}] $instr]\[ \t\]*\r\n.*"

Run $instr through string_to_regexp instead of string
map.

Could also use $hex instead of the the 0-9a-f patterns.

These were just cut/pasted from other usage.  I'll try and make
those changes though.




+proc func_check {instr} {
+    global func
+
+    set test "Found: $instr"

lowercase "found:"  (all test names in gdb are lowercase.)

Done.



The usual style is to put the test name in a variable, like:

set test "set disassembler-options invalid_option_value"
gdb_test_multiple $test $test {
  -re "Invalid disassembler option value: 'invalid_option_value'\." {
    pass $test
  }
  -re "'set disassembler-options \.\.\.' is not supported on this architecture\." {
    pass $test
  }
}

This makes it copy/paste typo-proof.

Done.

I'll post another patch after I've played with the option scanning I
mentioned above.  Thanks for the review.

Peter



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]