Issue 5643047: [ASan] Replace AsanProcMaps::GetObjectNameAndOffset with a simpler AsanProcMaps::DescribeAddress (Closed)
|
Created:
12 years, 12 months ago by timurrrr_at_google_com Modified:
12 years, 10 months ago CC:
llvm-commits_cs.uiuc.edu Visibility:
Public. |
Patch Set 1 #
Total comments: 19
MessagesTotal messages: 7
Hi, Can you please review this patch? Rationale: on Windows, it's much easier to get function name, source file and source line than obj+offset we use on POSIX (where we have to use an exterenal symbolizer script). I think the old API goes a little bit too much into the details on how things work, so I'm suggesting a slightly more generic API. There's one question inline, see comments. Thanks, Timur http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_internal.h File lib/asan/asan_internal.h (left): http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_internal.h#oldcode145 lib/asan/asan_internal.h:145: int SNPrint(char *buffer, size_t length, const char *format, ...); This was wrong in the first place http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_linux.cc File lib/asan/asan_linux.cc (right): http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_linux.cc#newcode216 lib/asan/asan_linux.cc:216: uintptr_t offset; this code is identical to that on Mac, see comment below http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_linux.cc#newcode265 lib/asan/asan_linux.cc:265: if (dl_iterate_phdr(dl_iterate_phdr_callback, &data)) { Alternative suggestion: I can extract this code to IterateForObjectNameAndOffset and move the common AsanProcMaps::DescribeAddress code to asan_posix.cc (we have the same symbolization capabilities on Linux and Mac, right?) WDYT? http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_procmaps.h File lib/asan/asan_procmaps.h (right): http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_procmaps.h#newcode30 lib/asan/asan_procmaps.h:30: // and puts it into the given buffer with a leading space I know this may sound a bit confusing, but look at how easy it is to use this function now (asan_stack.cc, which is the only place this is used)
Sign in to reply to this message.
Ok in general. Please define "const int kAsanMaxPath = 4096;" in asan_internal.h and use it everywhere instead of the literal const. Please ask glider@ to review and then commit. --kcc On Tue, Feb 7, 2012 at 9:41 AM, <timurrrr@google.com> wrote: > Reviewers: kcc1, Evgeniy Stepanov, glider, > > Message: > Hi, > > Can you please review this patch? > > Rationale: on Windows, it's much easier to get function name, source > file and source line than obj+offset we use on POSIX (where we have to > use an exterenal symbolizer script). > I think the old API goes a little bit too much into the details on how > things work, so I'm suggesting a slightly more generic API. > > There's one question inline, see comments. > > Thanks, > Timur > > > http://codereview.appspot.com/**5643047/diff/1/lib/asan/asan_**internal.h<htt... > File lib/asan/asan_internal.h (left): > > http://codereview.appspot.com/**5643047/diff/1/lib/asan/asan_** > internal.h#oldcode145<http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_internal.h#oldcode145> > lib/asan/asan_internal.h:145: int SNPrint(char *buffer, size_t length, > const char *format, ...); > This was wrong in the first place > > http://codereview.appspot.com/**5643047/diff/1/lib/asan/asan_**linux.cc<http:... > File lib/asan/asan_linux.cc (right): > > http://codereview.appspot.com/**5643047/diff/1/lib/asan/asan_** > linux.cc#newcode216<http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_linux.cc#newcode216> > lib/asan/asan_linux.cc:216: uintptr_t offset; > this code is identical to that on Mac, see comment below > > http://codereview.appspot.com/**5643047/diff/1/lib/asan/asan_** > linux.cc#newcode265<http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_linux.cc#newcode265> > lib/asan/asan_linux.cc:265: if > (dl_iterate_phdr(dl_iterate_**phdr_callback, &data)) { > Alternative suggestion: > I can extract this code to IterateForObjectNameAndOffset > and move the common AsanProcMaps::DescribeAddress code to asan_posix.cc > (we have the same symbolization capabilities on Linux and Mac, right?) > > WDYT? > > http://codereview.appspot.com/**5643047/diff/1/lib/asan/asan_**procmaps.h<htt... > File lib/asan/asan_procmaps.h (right): > > http://codereview.appspot.com/**5643047/diff/1/lib/asan/asan_** > procmaps.h#newcode30<http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_procmaps.h#newcode30> > lib/asan/asan_procmaps.h:30: // and puts it into the given buffer with a > leading space > I know this may sound a bit confusing, but look at how easy it is to use > this function now (asan_stack.cc, which is the only place this is used) > > > > Please review this at http://codereview.appspot.com/**5643047/<http://codereview.appspot.com/5643047/> > > Affected files: > M lib/asan/asan_internal.h > M lib/asan/asan_linux.cc > M lib/asan/asan_mac.cc > M lib/asan/asan_procmaps.h > M lib/asan/asan_stack.cc > > > Index: lib/asan/asan_internal.h > diff --git lib/asan/asan_internal.h lib/asan/asan_internal.h > index 041fa3dce803be895a325f1601891b**c98bffad92..** > 31842013b64419263a9ba67f8dca30**d08cca4fab 100644 > --- lib/asan/asan_internal.h > +++ lib/asan/asan_internal.h > @@ -142,7 +142,7 @@ size_t ReadFileToBuffer(const char *file_name, char > **buff, > > // asan_printf.cc > void RawWrite(const char *buffer); > -int SNPrint(char *buffer, size_t length, const char *format, ...); > +int SNPrintf(char *buffer, size_t length, const char *format, ...); > void Printf(const char *format, ...); > int SScanf(const char *str, const char *format, ...); > void Report(const char *format, ...); > Index: lib/asan/asan_linux.cc > diff --git lib/asan/asan_linux.cc lib/asan/asan_linux.cc > index 01a019eac850c2de05d8204bc068f1**9e9b25a52a..** > d530415cc69c459496c6f50a561aeb**ac204f7623 100644 > --- lib/asan/asan_linux.cc > +++ lib/asan/asan_linux.cc > @@ -210,11 +210,16 @@ bool AsanProcMaps::Next(uintptr_t *start, uintptr_t > *end, > > #ifdef __arm__ > > -// Gets the object name and the offset by walking AsanProcMaps. > -bool AsanProcMaps::**GetObjectNameAndOffset(**uintptr_t addr, uintptr_t > *offset, > - char filename[], > - size_t filename_size) { > - return IterateForObjectNameAndOffset(**addr, offset, filename, > filename_size); > +void AsanProcMaps::DescribeAddress(**uintptr_t addr, > + char out_buffer[], > + size_t buffer_size) { > + uintptr_t offset; > + char objname[4096]; > + if (**IterateForObjectNameAndOffset(**addr, &offset, objname, > sizeof(objname))) { > + SNPrintf(out_buffer, buffer_size, " (%s+0x%lx)", objname, offset); > + } else { > + out_buffer[0] = '\0'; > + } > } > > #else // __arm__ > @@ -248,19 +253,21 @@ static int dl_iterate_phdr_callback(**struct > dl_phdr_info *info, > } > > // Gets the object name and the offset using dl_iterate_phdr. > -bool AsanProcMaps::**GetObjectNameAndOffset(**uintptr_t addr, uintptr_t > *offset, > - char filename[], > - size_t filename_size) { > +void AsanProcMaps::DescribeAddress(**uintptr_t addr, > + char out_buffer[], > + size_t buffer_size) { > + char filename[4096]; > DlIterateData data; > data.count = 0; > data.addr = addr; > data.filename = filename; > - data.filename_size = filename_size; > + data.filename_size = sizeof(filename); > if (dl_iterate_phdr(dl_iterate_**phdr_callback, &data)) { > - *offset = data.offset; > - return true; > + SNPrintf(out_buffer, buffer_size, > + " (%s+0x%lx)", data.filename, data.offset); > + } else { > + out_buffer[0] = '\0'; > } > - return false; > } > > #endif // __arm__ > Index: lib/asan/asan_mac.cc > diff --git lib/asan/asan_mac.cc lib/asan/asan_mac.cc > index 112aebf8d93b52a9c8c75e516ccd79**07afadac44..** > 7832ce8a17722192f87f96cce90733**ea6fadc4fa 100644 > --- lib/asan/asan_mac.cc > +++ lib/asan/asan_mac.cc > @@ -271,10 +271,16 @@ bool AsanProcMaps::Next(uintptr_t *start, uintptr_t > *end, > return false; > } > > -bool AsanProcMaps::**GetObjectNameAndOffset(**uintptr_t addr, uintptr_t > *offset, > - char filename[], > - size_t filename_size) { > - return IterateForObjectNameAndOffset(**addr, offset, filename, > filename_size); > +void AsanProcMaps::DescribeAddress(**uintptr_t addr, > + char out_buffer[], > + size_t buffer_size) { > + uintptr_t offset; > + char objname[4096]; > + if (**IterateForObjectNameAndOffset(**addr, &offset, objname, > sizeof(objname))) { > + SNPrintf(out_buffer, buffer_size, " (%s+0x%lx)", objname, offset); > + } else { > + out_buffer[0] = '\0'; > + } > } > > void AsanThread::**SetThreadStackTopAndBottom() { > Index: lib/asan/asan_procmaps.h > diff --git lib/asan/asan_procmaps.h lib/asan/asan_procmaps.h > index 6dd42f9f653610588dca4553537138**0792516f89..** > bc5f67b2de033acf4f205101f9a690**82ec6bdce3 100644 > --- lib/asan/asan_procmaps.h > +++ lib/asan/asan_procmaps.h > @@ -24,10 +24,13 @@ class AsanProcMaps { > bool Next(uintptr_t *start, uintptr_t *end, uintptr_t *offset, > char filename[], size_t filename_size); > void Reset(); > - // Gets the object file name and the offset in that object for a given > - // address 'addr'. Returns true on success. > - bool GetObjectNameAndOffset(**uintptr_t addr, uintptr_t *offset, > - char filename[], size_t filename_size); > + > + // Gets all the information possible for a given address 'addr' > + // (object file name, offset, source file name, source line, etc.) > + // and puts it into the given buffer with a leading space > + // or puts an empty string "" if no information is available. > + void DescribeAddress(uintptr_t addr, char out_buffer[], size_t > buffer_size); > + > ~AsanProcMaps(); > private: > // Default implementation of GetObjectNameAndOffset. > Index: lib/asan/asan_stack.cc > diff --git lib/asan/asan_stack.cc lib/asan/asan_stack.cc > index 9f7469749085b576d8359b927da2c5**8623773e15..** > fe83534e8ff96546c09029705e2d94**a1af039c00 100644 > --- lib/asan/asan_stack.cc > +++ lib/asan/asan_stack.cc > @@ -42,14 +42,9 @@ void AsanStackTrace::PrintStack(**uintptr_t *addr, > size_t size) { > for (size_t i = 0; i < size && addr[i]; i++) { > proc_maps.Reset(); > uintptr_t pc = addr[i]; > - uintptr_t offset; > - char filename[4096]; > - if (proc_maps.**GetObjectNameAndOffset(pc, &offset, > - filename, sizeof(filename))) { > - Printf(" #%ld 0x%lx (%s+0x%lx)\n", i, pc, filename, offset); > - } else { > - Printf(" #%ld 0x%lx\n", i, pc); > - } > + char descr[4096]; > + proc_maps.DescribeAddress(pc, descr, sizeof(descr)); > + Printf(" #%ld 0x%lx%s\n", i, pc, descr); > } > } > #endif // ASAN_USE_EXTERNAL_SYMBOLIZER > > >
Sign in to reply to this message.
http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_procmaps.h File lib/asan/asan_procmaps.h (right): http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_procmaps.h#newcode30 lib/asan/asan_procmaps.h:30: // and puts it into the given buffer with a leading space On 2012/02/07 17:41:15, timurrrr_at_google_com wrote: > I know this may sound a bit confusing, but look at how easy it is to use this > function now (asan_stack.cc, which is the only place this is used) I think it's weird to write leading space here. "puts"->"writes"? http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_procmaps.h#newcode32 lib/asan/asan_procmaps.h:32: void DescribeAddress(uintptr_t addr, char out_buffer[], size_t buffer_size); Why not GetAddressDescription()? We've got two other different DescribeAddress functions already :) http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_stack.cc File lib/asan/asan_stack.cc (right): http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_stack.cc#newcode47 lib/asan/asan_stack.cc:47: Printf(" #%ld 0x%lx%s\n", i, pc, descr); Why not write space here?
Sign in to reply to this message.
Alexander, Can you please take a look about the refactoring idea? http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_procmaps.h File lib/asan/asan_procmaps.h (right): http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_procmaps.h#newcode30 lib/asan/asan_procmaps.h:30: // and puts it into the given buffer with a leading space On 2012/02/08 07:11:02, samsonov wrote: > On 2012/02/07 17:41:15, timurrrr_at_google_com wrote: > > I know this may sound a bit confusing, but look at how easy it is to use this > > function now (asan_stack.cc, which is the only place this is used) > > I think it's weird to write leading space here. See the other comment; otherwise you'll need "if" or "?:" every time you use a function > "puts"->"writes"? Will do http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_procmaps.h#newcode32 lib/asan/asan_procmaps.h:32: void DescribeAddress(uintptr_t addr, char out_buffer[], size_t buffer_size); On 2012/02/08 07:11:02, samsonov wrote: > Why not GetAddressDescription()? Shorter names are better if they mean the same thing, right? > We've got two other different DescribeAddress > functions already :) That's good, no? One has to remember less names. http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_stack.cc File lib/asan/asan_stack.cc (right): http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_stack.cc#newcode47 lib/asan/asan_stack.cc:47: Printf(" #%ld 0x%lx%s\n", i, pc, descr); Extra trailing space if descr is empty On 2012/02/08 07:11:02, samsonov wrote: > Why not write space here?
Sign in to reply to this message.
http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_internal.h File lib/asan/asan_internal.h (left): http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_internal.h#oldcode145 lib/asan/asan_internal.h:145: int SNPrint(char *buffer, size_t length, const char *format, ...); On 2012/02/07 17:41:15, timurrrr_at_google_com wrote: > This was wrong in the first place I think it's generally better to send separate CLs for cleanup. I'm ok with this particular change however. http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_linux.cc File lib/asan/asan_linux.cc (right): http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_linux.cc#newcode216 lib/asan/asan_linux.cc:216: uintptr_t offset; On 2012/02/07 17:41:15, timurrrr_at_google_com wrote: > this code is identical to that on Mac, see comment below You'd better put the default implementation into asan_procmaps.h instead of IterateForObjectNameAndOffset. http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_linux.cc#newcode221 lib/asan/asan_linux.cc:221: out_buffer[0] = '\0'; Please check that out_buffer!=NULL http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_linux.cc#newcode265 lib/asan/asan_linux.cc:265: if (dl_iterate_phdr(dl_iterate_phdr_callback, &data)) { On 2012/02/07 17:41:15, timurrrr_at_google_com wrote: > Alternative suggestion: > I can extract this code to IterateForObjectNameAndOffset > and move the common AsanProcMaps::DescribeAddress code to asan_posix.cc > (we have the same symbolization capabilities on Linux and Mac, right?) > > WDYT? We do not have dl_iterate_phdr on Mac. http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_linux.cc#newcode266 lib/asan/asan_linux.cc:266: SNPrintf(out_buffer, buffer_size, Please mind the return value of snprintf. http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_procmaps.h File lib/asan/asan_procmaps.h (right): http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_procmaps.h#newcode30 lib/asan/asan_procmaps.h:30: // and puts it into the given buffer with a leading space On 2012/02/07 17:41:15, timurrrr_at_google_com wrote: > I know this may sound a bit confusing, but look at how easy it is to use this > function now (asan_stack.cc, which is the only place this is used) Please no leading spaces. http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_stack.cc File lib/asan/asan_stack.cc (right): http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_stack.cc#newcode47 lib/asan/asan_stack.cc:47: Printf(" #%ld 0x%lx%s\n", i, pc, descr); Please keep in mind this should be %p (not %lx) on Windows See also http://mail.python.org/pipermail/patches/2000-June/000871.html
Sign in to reply to this message.
http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_procmaps.h File lib/asan/asan_procmaps.h (right): http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_procmaps.h#newcode32 lib/asan/asan_procmaps.h:32: void DescribeAddress(uintptr_t addr, char out_buffer[], size_t buffer_size); On 2012/02/08 09:43:33, timurrrr_at_google_com wrote: > On 2012/02/08 07:11:02, samsonov wrote: > > Why not GetAddressDescription()? > Shorter names are better if they mean the same thing, right? > > > We've got two other different DescribeAddress > > functions already :) > That's good, no? Not if they do different things. DescribeAddress we already have acts like a procedure and prints the description to the output. What you have here is function that returns the buffer with description. Leaving this up to you, though. > One has to remember less names.
Sign in to reply to this message.
After thinking for a while and studying the code better I've decided to leave the AsanProcMaps as-is. See http://codereview.appspot.com/5647052/diff/4001/lib/asan/asan_internal.h instead. http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_linux.cc File lib/asan/asan_linux.cc (right): http://codereview.appspot.com/5643047/diff/1/lib/asan/asan_linux.cc#newcode265 lib/asan/asan_linux.cc:265: if (dl_iterate_phdr(dl_iterate_phdr_callback, &data)) { I meant Mac and Android. On 2012/02/08 10:12:40, glider wrote: > On 2012/02/07 17:41:15, timurrrr_at_google_com wrote: > > Alternative suggestion: > > I can extract this code to IterateForObjectNameAndOffset > > and move the common AsanProcMaps::DescribeAddress code to asan_posix.cc > > (we have the same symbolization capabilities on Linux and Mac, right?) > > > > WDYT? > > We do not have dl_iterate_phdr on Mac.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
