[Lldb-commits] [lldb] r169645 - in /lldb/trunk: include/lldb/Core/ include/lldb/Expression/ source/Commands/ source/Core/ source/Expression/ source/Interpreter/ source/Plugins/DynamicLoader/MacOSX-DYLD/ source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/ source/Plugins/OperatingSystem/Python/ source/Plugins/Process/Linux/ source/Plugins/Process/POSIX/ source/Plugins/Process/Utility/ source/Plugins/SymbolFile/DWARF/ source/Utility/ tools/driver/

[Filipe Cabecinhas]

Hi,

On Fri, Dec 7, 2012 at 2:21 PM, Daniel Malea <daniel.malea at intel.com> wrote:

> ...

- change sizeof to use a type name instead of variable name
>
>
What, why? Using a variable name is usually what you want (and much safer,
if said variable changes type). This doesn't apply here (we're setting what
dst points to, not dst itself), but this sentence without any context felt
weird. :-)



> Modified: lldb/trunk/source/Plugins/Process/Linux/ProcessMonitor.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/Linux/ProcessMonitor.cpp?rev=169645&r1=169644&r2=169645&view=diff
>
> ==============================================================================
> --- lldb/trunk/source/Plugins/Process/Linux/ProcessMonitor.cpp (original)
> +++ lldb/trunk/source/Plugins/Process/Linux/ProcessMonitor.cpp Fri Dec  7
> 16:21:08 2012
> @@ -13,6 +13,7 @@
>  #include <errno.h>
>  #include <poll.h>
>  #include <string.h>
> +#include <stdint.h>
>  #include <unistd.h>
>  #include <sys/ptrace.h>
>  #include <sys/socket.h>
> @@ -200,7 +201,7 @@
>
>          // Copy the data into our buffer
>          if (log)
> -            memset(dst, 0, sizeof(dst));
> +            memset(dst, 0, sizeof(unsigned char));
>          for (unsigned i = 0; i < remainder; ++i)
>              dst[i] = ((data >> i*8) & 0xFF);
>
>

Here! We're only setting one of the bytes on dst. This doesn't make sense,
to me.
It also took me some time to figure out why we're only setting dst when
logging, but it's because a few lines ahead we do:

        if (log && ProcessPOSIXLog::AtTopNestLevel() &&
            (log->GetMask().Test(POSIX_LOG_MEMORY_DATA_LONG) ||
             (log->GetMask().Test(POSIX_LOG_MEMORY_DATA_SHORT) &&
              size <= POSIX_LOG_MEMORY_SHORT_BYTES)))
            log->Printf ("ProcessMonitor::%s() [%p]:0x%lx (0x%lx)",
__FUNCTION__,
                         (void*)vm_addr, *(unsigned long*)dst, (unsigned
long)data);

printing dst as if it were an unsigned long *.

I'm not familiar with the code on this part, but I would say that we want
one of these:
  - change the memset size to sizeof(unsigned long*)
  - change the memset size to size, which I suppose is the size we have in
the memory pointed to by buf (which gets static_cast'ed to dst). Are we
always sure size is equal or greater than sizeof(unsigned long)?

Or maybe I didn't understand this part of the code.
Could someone more familiar with ProcessMonitor chip in?

Regards,

  Filipe
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20121212/d2397adf/attachment.html>