Ok in general. <div>Please define "const int kAsanMaxPath = 4096;" in asan_internal.h and use it everywhere instead of the literal const. </div><div>Please ask glider@ to review and then commit. </div><div><br></div>
<div>--kcc </div><div><br><br><div class="gmail_quote">On Tue, Feb 7, 2012 at 9:41 AM, <span dir="ltr"><<a href="mailto:timurrrr@google.com">timurrrr@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Reviewers: kcc1, Evgeniy Stepanov, glider,<br>
<br>
Message:<br>
Hi,<br>
<br>
Can you please review this patch?<br>
<br>
Rationale: on Windows, it's much easier to get function name, source<br>
file and source line than obj+offset we use on POSIX (where we have to<br>
use an exterenal symbolizer script).<br>
I think the old API goes a little bit too much into the details on how<br>
things work, so I'm suggesting a slightly more generic API.<br>
<br>
There's one question inline, see comments.<br>
<br>
Thanks,<br>
Timur<br>
<br>
<br>
<a href="http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_internal.h" target="_blank">http://codereview.appspot.com/<u></u>5643047/diff/1/lib/asan/asan_<u></u>internal.h</a><br>
File lib/asan/asan_internal.h (left):<br>
<br>
<a href="http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_internal.h#oldcode145" target="_blank">http://codereview.appspot.com/<u></u>5643047/diff/1/lib/asan/asan_<u></u>internal.h#oldcode145</a><br>
lib/asan/asan_internal.h:145: int SNPrint(char *buffer, size_t length,<br>
const char *format, ...);<br>
This was wrong in the first place<br>
<br>
<a href="http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_linux.cc" target="_blank">http://codereview.appspot.com/<u></u>5643047/diff/1/lib/asan/asan_<u></u>linux.cc</a><br>
File lib/asan/asan_linux.cc (right):<br>
<br>
<a href="http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_linux.cc#newcode216" target="_blank">http://codereview.appspot.com/<u></u>5643047/diff/1/lib/asan/asan_<u></u>linux.cc#newcode216</a><br>
lib/asan/asan_linux.cc:216: uintptr_t offset;<br>
this code is identical to that on Mac, see comment below<br>
<br>
<a href="http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_linux.cc#newcode265" target="_blank">http://codereview.appspot.com/<u></u>5643047/diff/1/lib/asan/asan_<u></u>linux.cc#newcode265</a><br>
lib/asan/asan_linux.cc:265: if<br>
(dl_iterate_phdr(dl_iterate_<u></u>phdr_callback, &data)) {<br>
Alternative suggestion:<br>
I can extract this code to IterateForObjectNameAndOffset<br>
and move the common AsanProcMaps::DescribeAddress code to asan_posix.cc<br>
(we have the same symbolization capabilities on Linux and Mac, right?)<br>
<br>
WDYT?<br>
<br>
<a href="http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_procmaps.h" target="_blank">http://codereview.appspot.com/<u></u>5643047/diff/1/lib/asan/asan_<u></u>procmaps.h</a><br>
File lib/asan/asan_procmaps.h (right):<br>
<br>
<a href="http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_procmaps.h#newcode30" target="_blank">http://codereview.appspot.com/<u></u>5643047/diff/1/lib/asan/asan_<u></u>procmaps.h#newcode30</a><br>
lib/asan/asan_procmaps.h:30: // and puts it into the given buffer with a<br>
leading space<br>
I know this may sound a bit confusing, but look at how easy it is to use<br>
this function now (asan_stack.cc, which is the only place this is used)<br>
<br>
<br>
<br>
Please review this at <a href="http://codereview.appspot.com/5643047/" target="_blank">http://codereview.appspot.com/<u></u>5643047/</a><br>
<br>
Affected files:<br>
M lib/asan/asan_internal.h<br>
M lib/asan/asan_linux.cc<br>
M lib/asan/asan_mac.cc<br>
M lib/asan/asan_procmaps.h<br>
M lib/asan/asan_stack.cc<br>
<br>
<br>
Index: lib/asan/asan_internal.h<br>
diff --git lib/asan/asan_internal.h lib/asan/asan_internal.h<br>
index 041fa3dce803be895a325f1601891b<u></u>c98bffad92..<u></u>31842013b64419263a9ba67f8dca30<u></u>d08cca4fab 100644<br>
--- lib/asan/asan_internal.h<br>
+++ lib/asan/asan_internal.h<br>
@@ -142,7 +142,7 @@ size_t ReadFileToBuffer(const char *file_name, char **buff,<br>
<br>
// asan_printf.cc<br>
void RawWrite(const char *buffer);<br>
-int SNPrint(char *buffer, size_t length, const char *format, ...);<br>
+int SNPrintf(char *buffer, size_t length, const char *format, ...);<br>
void Printf(const char *format, ...);<br>
int SScanf(const char *str, const char *format, ...);<br>
void Report(const char *format, ...);<br>
Index: lib/asan/asan_linux.cc<br>
diff --git lib/asan/asan_linux.cc lib/asan/asan_linux.cc<br>
index 01a019eac850c2de05d8204bc068f1<u></u>9e9b25a52a..<u></u>d530415cc69c459496c6f50a561aeb<u></u>ac204f7623 100644<br>
--- lib/asan/asan_linux.cc<br>
+++ lib/asan/asan_linux.cc<br>
@@ -210,11 +210,16 @@ bool AsanProcMaps::Next(uintptr_t *start, uintptr_t *end,<br>
<br>
#ifdef __arm__<br>
<br>
-// Gets the object name and the offset by walking AsanProcMaps.<br>
-bool AsanProcMaps::<u></u>GetObjectNameAndOffset(<u></u>uintptr_t addr, uintptr_t *offset,<br>
- char filename[],<br>
- size_t filename_size) {<br>
- return IterateForObjectNameAndOffset(<u></u>addr, offset, filename, filename_size);<br>
+void AsanProcMaps::DescribeAddress(<u></u>uintptr_t addr,<br>
+ char out_buffer[],<br>
+ size_t buffer_size) {<br>
+ uintptr_t offset;<br>
+ char objname[4096];<br>
+ if (<u></u>IterateForObjectNameAndOffset(<u></u>addr, &offset, objname, sizeof(objname))) {<br>
+ SNPrintf(out_buffer, buffer_size, " (%s+0x%lx)", objname, offset);<br>
+ } else {<br>
+ out_buffer[0] = '\0';<br>
+ }<br>
}<br>
<br>
#else // __arm__<br>
@@ -248,19 +253,21 @@ static int dl_iterate_phdr_callback(<u></u>struct dl_phdr_info *info,<br>
}<br>
<br>
// Gets the object name and the offset using dl_iterate_phdr.<br>
-bool AsanProcMaps::<u></u>GetObjectNameAndOffset(<u></u>uintptr_t addr, uintptr_t *offset,<br>
- char filename[],<br>
- size_t filename_size) {<br>
+void AsanProcMaps::DescribeAddress(<u></u>uintptr_t addr,<br>
+ char out_buffer[],<br>
+ size_t buffer_size) {<br>
+ char filename[4096];<br>
DlIterateData data;<br>
data.count = 0;<br>
data.addr = addr;<br>
data.filename = filename;<br>
- data.filename_size = filename_size;<br>
+ data.filename_size = sizeof(filename);<br>
if (dl_iterate_phdr(dl_iterate_<u></u>phdr_callback, &data)) {<br>
- *offset = data.offset;<br>
- return true;<br>
+ SNPrintf(out_buffer, buffer_size,<br>
+ " (%s+0x%lx)", data.filename, data.offset);<br>
+ } else {<br>
+ out_buffer[0] = '\0';<br>
}<br>
- return false;<br>
}<br>
<br>
#endif // __arm__<br>
Index: lib/asan/asan_mac.cc<br>
diff --git lib/asan/asan_mac.cc lib/asan/asan_mac.cc<br>
index 112aebf8d93b52a9c8c75e516ccd79<u></u>07afadac44..<u></u>7832ce8a17722192f87f96cce90733<u></u>ea6fadc4fa 100644<br>
--- lib/asan/asan_mac.cc<br>
+++ lib/asan/asan_mac.cc<br>
@@ -271,10 +271,16 @@ bool AsanProcMaps::Next(uintptr_t *start, uintptr_t *end,<br>
return false;<br>
}<br>
<br>
-bool AsanProcMaps::<u></u>GetObjectNameAndOffset(<u></u>uintptr_t addr, uintptr_t *offset,<br>
- char filename[],<br>
- size_t filename_size) {<br>
- return IterateForObjectNameAndOffset(<u></u>addr, offset, filename, filename_size);<br>
+void AsanProcMaps::DescribeAddress(<u></u>uintptr_t addr,<br>
+ char out_buffer[],<br>
+ size_t buffer_size) {<br>
+ uintptr_t offset;<br>
+ char objname[4096];<br>
+ if (<u></u>IterateForObjectNameAndOffset(<u></u>addr, &offset, objname, sizeof(objname))) {<br>
+ SNPrintf(out_buffer, buffer_size, " (%s+0x%lx)", objname, offset);<br>
+ } else {<br>
+ out_buffer[0] = '\0';<br>
+ }<br>
}<br>
<br>
void AsanThread::<u></u>SetThreadStackTopAndBottom() {<br>
Index: lib/asan/asan_procmaps.h<br>
diff --git lib/asan/asan_procmaps.h lib/asan/asan_procmaps.h<br>
index 6dd42f9f653610588dca4553537138<u></u>0792516f89..<u></u>bc5f67b2de033acf4f205101f9a690<u></u>82ec6bdce3 100644<br>
--- lib/asan/asan_procmaps.h<br>
+++ lib/asan/asan_procmaps.h<br>
@@ -24,10 +24,13 @@ class AsanProcMaps {<br>
bool Next(uintptr_t *start, uintptr_t *end, uintptr_t *offset,<br>
char filename[], size_t filename_size);<br>
void Reset();<br>
- // Gets the object file name and the offset in that object for a given<br>
- // address 'addr'. Returns true on success.<br>
- bool GetObjectNameAndOffset(<u></u>uintptr_t addr, uintptr_t *offset,<br>
- char filename[], size_t filename_size);<br>
+<br>
+ // Gets all the information possible for a given address 'addr'<br>
+ // (object file name, offset, source file name, source line, etc.)<br>
+ // and puts it into the given buffer with a leading space<br>
+ // or puts an empty string "" if no information is available.<br>
+ void DescribeAddress(uintptr_t addr, char out_buffer[], size_t buffer_size);<br>
+<br>
~AsanProcMaps();<br>
private:<br>
// Default implementation of GetObjectNameAndOffset.<br>
Index: lib/asan/asan_stack.cc<br>
diff --git lib/asan/asan_stack.cc lib/asan/asan_stack.cc<br>
index 9f7469749085b576d8359b927da2c5<u></u>8623773e15..<u></u>fe83534e8ff96546c09029705e2d94<u></u>a1af039c00 100644<br>
--- lib/asan/asan_stack.cc<br>
+++ lib/asan/asan_stack.cc<br>
@@ -42,14 +42,9 @@ void AsanStackTrace::PrintStack(<u></u>uintptr_t *addr, size_t size) {<br>
for (size_t i = 0; i < size && addr[i]; i++) {<br>
proc_maps.Reset();<br>
uintptr_t pc = addr[i];<br>
- uintptr_t offset;<br>
- char filename[4096];<br>
- if (proc_maps.<u></u>GetObjectNameAndOffset(pc, &offset,<br>
- filename, sizeof(filename))) {<br>
- Printf(" #%ld 0x%lx (%s+0x%lx)\n", i, pc, filename, offset);<br>
- } else {<br>
- Printf(" #%ld 0x%lx\n", i, pc);<br>
- }<br>
+ char descr[4096];<br>
+ proc_maps.DescribeAddress(pc, descr, sizeof(descr));<br>
+ Printf(" #%ld 0x%lx%s\n", i, pc, descr);<br>
}<br>
}<br>
#endif // ASAN_USE_EXTERNAL_SYMBOLIZER<br>
<br>
<br>
</blockquote></div><br></div>