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 v2 5/7] btrace, gdbserver: remove the to_supports_btrace target method


Hi Markus,

> https://sourceware.org/ml/gdb-patches/2018-02/msg00420.html
> 
> 
> It's the same I sent inline to the email with a recurring white-space 
> error fixed.

 With the change description you have also overrun our 74-column limit: 
<https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Column_limits>. 
NB due to how `git log' etc. indents descriptions I prefer to stay within 
72 columns with my own changes for a better visual effect, though you are 
of course free to use your own judgement here as long as you're within 74 
columns.

 Same with ChangeLog entries.  Also as per the the GNU Coding Standard:
<https://www.gnu.org/prep/standards/standards.html#Style-of-Change-Logs>
long function lists, etc. use `)' as the closing character, so:

	* target.h (target_enable_btrace, target_disable_btrace)
	(target_read_btrace, target_read_btrace_conf): Turn macro into 
	inline function.  Throw error if target method not defined.

 You need to mark new error messages for translation, so:

+    error (_("Target does not support branch tracing."));

etc.

 This:

+static inline int
+target_read_btrace (struct btrace_target_info *tinfo, struct buffer *buffer,
+		    enum btrace_read_type type)

also overruns the 74-column limit.

 The formatting appears otherwise OK to me and I'll leave the matter 
itself of this change up to someone with suitable expertise in this area.

> Andreas already indicated that the patch fixes the problem on s390x.  Did the full
> test show any further issues on your side, Maciej?

 We had a failure last night in the lab literally as I have scheduled the 
tests and consequently they didn't run.  I'll schedule them again as soon 
as possible, but the target system I have been using for testing is not 
back up yet, so it may not happen before tomorrow.  Sorry.

> >  Also did you verify that old-GDB/new-gdbserver and new-GDB/old-gdbserver
> > combinations work correctly?
> 
> That was discussed here:
> https://sourceware.org/ml/gdb-patches/2018-02/msg00117.html

 Thanks for confirming.  Is there going to be any difference here for 
non-x86 targets that needs to be verified?

  Maciej


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