Author: florian
Date: Mon Apr 20 21:42:42 2015
New Revision: 15116
Log:
Remove function sr_ResHI from Linux specific code.
Add function VG_(sr_as_string).
Modified:
trunk/coregrind/m_libcprint.c
trunk/coregrind/m_syswrap/priv_types_n_macros.h
trunk/coregrind/m_syswrap/syswrap-main.c
trunk/include/pub_tool_basics.h
trunk/include/pub_tool_libcprint.h
Modified: trunk/coregrind/m_libcprint.c
==============================================================================
--- trunk/coregrind/m_libcprint.c (original)
+++ trunk/coregrind/m_libcprint.c Mon Apr 20 21:42:42 2015
@@ -643,6 +643,44 @@
VG_(exit)(1);
}
+/* ---------------------------------------------------------------------
+ VG_(sr_as_string)()
+ ------------------------------------------------------------------ */
+
+/* Return a textual representation of a SysRes value in a statically
+ allocated buffer. The buffer will be overwritten with the next
+ invocation. */
+#if defined(VGO_linux)
+// FIXME: Does this function need to be adjusted for MIPS's _valEx ?
+const HChar *VG_(sr_as_string) ( SysRes sr )
+{
+ static HChar buf[7+1+2+16+1+1]; // large enough
+
+ if (sr_isError(sr))
+ VG_(sprintf)(buf, "Failure(0x%lx)", sr_Err(sr));
+ else
+ VG_(sprintf)(buf, "Success(0x%lx)", sr_Res(sr));
+ return buf;
+}
+
+#elif defined(VGO_darwin)
+
+const HChar *VG_(sr_as_string) ( SysRes sr )
+{
+ static HChar buf[7+1+2+16+1+2+16+1+1]; // large enough
+
+ if (sr_isError(sr))
+ VG_(sprintf)(buf, "Failure(0x%lx)", sr_Err(sr));
+ else
+ VG_(sprintf)(buf, "Success(0x%lx:0x%lx)", sr_ResHI(sr), sr_Res(sr));
+ return buf;
+}
+
+#else
+
+#error unknown OS
+
+#endif
/*--------------------------------------------------------------------*/
/*--- end ---*/
Modified: trunk/coregrind/m_syswrap/priv_types_n_macros.h
==============================================================================
--- trunk/coregrind/m_syswrap/priv_types_n_macros.h (original)
+++ trunk/coregrind/m_syswrap/priv_types_n_macros.h Mon Apr 20 21:42:42 2015
@@ -331,11 +331,13 @@
return sr_Res(st->sres);
}
+#if defined(VGO_darwin)
static inline UWord getRESHI ( SyscallStatus* st ) {
vg_assert(st->what == SsComplete);
vg_assert(!sr_isError(st->sres));
return sr_ResHI(st->sres);
}
+#endif
static inline UWord getERR ( SyscallStatus* st ) {
vg_assert(st->what == SsComplete);
Modified: trunk/coregrind/m_syswrap/syswrap-main.c
==============================================================================
--- trunk/coregrind/m_syswrap/syswrap-main.c (original)
+++ trunk/coregrind/m_syswrap/syswrap-main.c Mon Apr 20 21:42:42 2015
@@ -1669,9 +1669,7 @@
if (sci->flags & SfNoWriteResult) {
PRINT(" --> [pre-success] NoWriteResult");
} else {
- PRINT(" --> [pre-success] Success(0x%llx:0x%llx)",
- (ULong)sr_ResHI(sci->status.sres),
- (ULong)sr_Res(sci->status.sres));
+ PRINT(" --> [pre-success] %s", VG_(sr_as_string)(sci->status.sres));
}
/* In this case the allowable flags are to ask for a signal-poll
and/or a yield after the call. Changing the args isn't
@@ -1684,7 +1682,7 @@
else
if (sci->status.what == SsComplete && sr_isError(sci->status.sres)) {
/* The pre-handler decided to fail syscall itself. */
- PRINT(" --> [pre-fail] Failure(0x%llx)", (ULong)sr_Err(sci->status.sres));
+ PRINT(" --> [pre-fail] %s", VG_(sr_as_string)(sci->status.sres));
/* In this case, the pre-handler is also allowed to ask for the
post-handler to be run anyway. Changing the args is not
allowed. */
@@ -1769,18 +1767,9 @@
/* Be decorative, if required. */
if (VG_(clo_trace_syscalls)) {
- Bool failed = sr_isError(sci->status.sres);
- if (failed) {
- PRINT("SYSCALL[%d,%d](%s) ... [async] --> Failure(0x%llx)",
- VG_(getpid)(), tid, VG_SYSNUM_STRING(sysno),
- (ULong)sr_Err(sci->status.sres));
- } else {
- PRINT("SYSCALL[%d,%d](%s) ... [async] --> "
- "Success(0x%llx:0x%llx)",
- VG_(getpid)(), tid, VG_SYSNUM_STRING(sysno),
- (ULong)sr_ResHI(sci->status.sres),
- (ULong)sr_Res(sci->status.sres) );
- }
+ PRINT("SYSCALL[%d,%d](%s) ... [async] --> %s",
+ VG_(getpid)(), tid, VG_SYSNUM_STRING(sysno),
+ VG_(sr_as_string)(sci->status.sres));
}
} else {
@@ -1800,15 +1789,7 @@
/* Be decorative, if required. */
if (VG_(clo_trace_syscalls)) {
- Bool failed = sr_isError(sci->status.sres);
- if (failed) {
- PRINT("[sync] --> Failure(0x%llx)",
- (ULong)sr_Err(sci->status.sres) );
- } else {
- PRINT("[sync] --> Success(0x%llx:0x%llx)",
- (ULong)sr_ResHI(sci->status.sres),
- (ULong)sr_Res(sci->status.sres) );
- }
+ PRINT("[sync] --> %s", VG_(sr_as_string)(sci->status.sres));
}
}
}
Modified: trunk/include/pub_tool_basics.h
==============================================================================
--- trunk/include/pub_tool_basics.h (original)
+++ trunk/include/pub_tool_basics.h Mon Apr 20 21:42:42 2015
@@ -192,9 +192,6 @@
static inline UWord sr_ResEx ( SysRes sr ) {
return sr._isError ? 0 : sr._valEx;
}
-static inline UWord sr_ResHI ( SysRes sr ) {
- return 0;
-}
static inline UWord sr_Err ( SysRes sr ) {
return sr._isError ? sr._val : 0;
}
Modified: trunk/include/pub_tool_libcprint.h
==============================================================================
--- trunk/include/pub_tool_libcprint.h (original)
+++ trunk/include/pub_tool_libcprint.h Mon Apr 20 21:42:42 2015
@@ -154,6 +154,9 @@
/* Flush any output cached by previous calls to VG_(message) et al. */
extern void VG_(message_flush) ( void );
+/* Return a SysRes value as a character string. */
+extern const HChar *VG_(sr_as_string) ( SysRes sr );
+
#endif // __PUB_TOOL_LIBCPRINT_H
/*--------------------------------------------------------------------*/
|
|
From: Philippe W. <phi...@sk...> - 2015-04-20 21:26:59
|
Nice cleanup.
One small comment: wouldn't the description of VG_(sr_as_string)()
better be put in the .h rather than in the .c file ?
The fact that the memory is static (or must be freed by the caller)
is better described there ?
Philippe
On Mon, 2015-04-20 at 20:42 +0000, sv...@va... wrote:
> +/* ---------------------------------------------------------------------
> + VG_(sr_as_string)()
> + ------------------------------------------------------------------ */
> +
> +/* Return a textual representation of a SysRes value in a statically
> + allocated buffer. The buffer will be overwritten with the next
> + invocation. */
> +#if defined(VGO_linux)
> +// FIXME: Does this function need to be adjusted for MIPS's _valEx ?
> +const HChar *VG_(sr_as_string) ( SysRes sr )
> +{
....
> Modified: trunk/include/pub_tool_libcprint.h
> ==============================================================================
> --- trunk/include/pub_tool_libcprint.h (original)
> +++ trunk/include/pub_tool_libcprint.h Mon Apr 20 21:42:42 2015
> @@ -154,6 +154,9 @@
> /* Flush any output cached by previous calls to VG_(message) et al. */
> extern void VG_(message_flush) ( void );
>
> +/* Return a SysRes value as a character string. */
> +extern const HChar *VG_(sr_as_string) ( SysRes sr );
> +
> #endif // __PUB_TOOL_LIBCPRINT_H
>
> /*--------------------------------------------------------------------*/
>
>
> ------------------------------------------------------------------------------
> BPM Camp - Free Virtual Workshop May 6th at 10am PDT/1PM EDT
> Develop your own process in accordance with the BPMN 2 standard
> Learn Process modeling best practices with Bonita BPM through live exercises
> http://www.bonitasoft.com/be-part-of-it/events/bpm-camp-virtual- event?utm_
> source=Sourceforge_BPM_Camp_5_6_15&utm_medium=email&utm_campaign=VA_SF
> _______________________________________________
> Valgrind-developers mailing list
> Val...@li...
> https://lists.sourceforge.net/lists/listinfo/valgrind-developers
|
|
From: Philippe W. <phi...@sk...> - 2015-04-21 21:01:51
|
On Mon, 2015-04-20 at 23:46 +0200, Florian Krohm wrote: > On 20.04.2015 23:28, Philippe Waroquiers wrote: > > Nice cleanup. > > One small comment: wouldn't the description of VG_(sr_as_string)() > > better be put in the .h rather than in the .c file ? > > The fact that the memory is static (or must be freed by the caller) > > is better described there ? > > I tend to favour small comments in header files and put noteworthy > details next to the implementation. But I have no strong feelings and we > don't have a policy for this either. So if you want to change it please > feel free. Done as revision 15127. At work, we have a convention/guideline that to know how to use a module, it is/should be sufficient to read the 'spec' (i.e. the .h) of the module. This allows to limit the reading to what is needed. E.g. allows to skip internal functions/comments/internal data structures and so on. So, information for the caller such as 'you must free the returned pointer' or 'returns a static buffer, overwritten on next invocation' is better placed in the .h, following this convention. I think many modules of Valgrind already follows this approach. Maybe we could be more systematic in that approach ? Philippe |
|
From: Bart V. A. <bva...@ac...> - 2015-04-22 10:20:10
|
On 04/21/15 23:02, Philippe Waroquiers wrote: > On Mon, 2015-04-20 at 23:46 +0200, Florian Krohm wrote: >> On 20.04.2015 23:28, Philippe Waroquiers wrote: >>> Nice cleanup. >>> One small comment: wouldn't the description of VG_(sr_as_string)() >>> better be put in the .h rather than in the .c file ? >>> The fact that the memory is static (or must be freed by the caller) >>> is better described there ? >> >> I tend to favour small comments in header files and put noteworthy >> details next to the implementation. But I have no strong feelings and we >> don't have a policy for this either. So if you want to change it please >> feel free. > Done as revision 15127. > > At work, we have a convention/guideline that to know how to use a > module, it is/should be sufficient to read the 'spec' (i.e. the .h) > of the module. > This allows to limit the reading to what is needed. > E.g. allows to skip internal functions/comments/internal data structures > and so on. > > So, information for the caller such as > 'you must free the returned pointer' > or > 'returns a static buffer, overwritten on next invocation' > is better placed in the .h, following this convention. > > I think many modules of Valgrind already follows > this approach. > > Maybe we could be more systematic in that approach ? There might be good reasons to add documentation for Valgrind functions in Valgrind header files. But I think it's worth mentioning here that there is a strict policy in the Linux kernel is to add such documentation in the .c files only. The rationale behind this policy is that it is easy to forget to update a header file when modifying the implementation. That's why in the Linux kernel the documentation is kept close to the implementation. Bart. |
|
From: Florian K. <fl...@ei...> - 2015-04-20 21:46:51
|
On 20.04.2015 23:28, Philippe Waroquiers wrote: > Nice cleanup. > One small comment: wouldn't the description of VG_(sr_as_string)() > better be put in the .h rather than in the .c file ? > The fact that the memory is static (or must be freed by the caller) > is better described there ? I tend to favour small comments in header files and put noteworthy details next to the implementation. But I have no strong feelings and we don't have a policy for this either. So if you want to change it please feel free. Florian |