[llvm-commits] [ASan] Replace AsanProcMaps::GetObjectNameAndOffset with a simpler AsanProcMaps::DescribeAddress (issue 5643047)
Kostya Serebryany
kcc at google.com
Tue Feb 7 10:20:47 PST 2012
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 at 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<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<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://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<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<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<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
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120207/a18dde64/attachment.html>
More information about the llvm-commits
mailing list