From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 34394 invoked by alias); 12 Dec 2019 01:30:00 -0000 Mailing-List: contact elfutils-devel-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Post: List-Help: List-Subscribe: Sender: elfutils-devel-owner@sourceware.org Received: (qmail 34356 invoked by uid 89); 12 Dec 2019 01:30:00 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Checked: by ClamAV 0.100.3 on sourceware.org X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=relocate X-Spam-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on sourceware.org X-Spam-Level: X-HELO: mail-pl1-f181.google.com Received: from mail-pl1-f181.google.com (HELO mail-pl1-f181.google.com) (209.85.214.181) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 12 Dec 2019 01:29:56 +0000 Received: by mail-pl1-f181.google.com with SMTP id w7so298323plz.12 for ; Wed, 11 Dec 2019 17:29:56 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=osandov-com.20150623.gappssmtp.com; s=20150623; h=from:to:subject:date:message-id:in-reply-to:references:mime-version :content-transfer-encoding; bh=+T8I5hKgoU4dZPIRtRk3JNtB+LiuFjByfnxblVX48iI=; b=GfX69jXz4XIjH55AGREfWRhdM5ISoYEkCw24UYWQ28biv/HFnG4Fyjt2ADfb88kREC jWf+rYE7KIn1PKjxofQtm2Vr1uinn1KP+I0mhoRwzx+wF+pr9jD7/qmancgQ6LWYNqk2 YIqF7nsArfelWJgMHV3bNM/3sEIwK+wBmygkTK4q7dctJIJ0rnJ/LNf9ReVCgjtSOyPG LehkBSmU4rMIpur3r2o3opFf2/85LHVvs9kOX5AUNU8mpbOHQEcGu210odpzU4B3R407 KEdyyzI4lXxWUX1+ZPaFszWD9RKC5UONJgd2NzzmtKhNJl1urA68jfNMW8/gF9L6bt8z In0g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=+T8I5hKgoU4dZPIRtRk3JNtB+LiuFjByfnxblVX48iI=; b=mDJPXZ6XLtWQP6z+449E9g3myI+XjYWbzmE9/uEE95mEBxNWROO25WsTYKa4rDjjNW hvndRjAGaIt3JV/XxN838Ngi7WnECM+cXk55AHlyS6ShUOFG3zeOohYxidYsqH83Bc1l tawlErhUphbFP99g5qVwVbmgyC5vlj1gdMZeTS3cc9cvlmdkJ1gq8o0M74HEJe32SnqA uqjCQ6mX+zJDG7650DNroPR+9AIxVFwkmUy4rTFk0I4oNplQ7PCWfgQg9DvVMy5PEm/w zoAgabvmGhNCJzpGbpbdeyLdJH85JtDQNTyMrVUHB3t5n+ZCzbtMnkW5hZa6GnRI4nCq KmIw== X-Gm-Message-State: APjAAAXtzFhfgf1cqH/7/oKu/Mr09HMLe7Vl9YBPOGmCY4rxtM49DQ1f WoU7blEVK9p7Ub/Yd5h30wlSrD3aUTQP/Q== X-Google-Smtp-Source: APXvYqzSD1FmD/ctZqRWvoUjOlNfLRyGfbpTGMSle7ITOKaQrf6GlfpmzDu+5ZHK1mphcquSu3G74g== X-Received: by 2002:a17:902:7784:: with SMTP id o4mr6657011pll.176.1576114194457; Wed, 11 Dec 2019 17:29:54 -0800 (PST) Received: from vader.thefacebook.com ([2620:10d:c090:200::3:19b7]) by smtp.gmail.com with ESMTPSA id d6sm1102556pjl.8.2019.12.11.17.29.53 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 11 Dec 2019 17:29:53 -0800 (PST) From: Omar Sandoval To: elfutils-devel@sourceware.org Subject: [PATCH 2/4] libdwfl: remove broken coalescing logic in dwfl_report_segment Date: Thu, 12 Dec 2019 01:30:00 -0000 Message-Id: <7a37afaf5a667a1bb9ff56f575db9fb4a4f9adf0.1576112311.git.osandov@fb.com> X-Mailer: git-send-email 2.24.0 In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2019-q4/txt/msg00264.txt.bz2 From: Omar Sandoval dwfl_report_segment has some logic that detects when a segment is contiguous with the previously reported segment, in which case it's supposed to coalesce them. However, in this case, it actually returns without updating the segment array at all. As far as I can tell, this has always been broken. It appears that no one uses the coalescing logic anyways, as they pass IDENT as NULL. Let's just get rid of the logic and add a test case. Signed-off-by: Omar Sandoval --- .gitignore | 1 + libdwfl/ChangeLog | 4 ++ libdwfl/libdwfl.h | 20 ++----- libdwfl/libdwflP.h | 7 +-- libdwfl/segment.c | 35 +++++------ tests/ChangeLog | 5 ++ tests/Makefile.am | 6 +- tests/dwfl-report-segment-contiguous.c | 82 ++++++++++++++++++++++++++ 8 files changed, 115 insertions(+), 45 deletions(-) create mode 100644 tests/dwfl-report-segment-contiguous.c diff --git a/.gitignore b/.gitignore index 72f22855..43fb7275 100644 --- a/.gitignore +++ b/.gitignore @@ -112,6 +112,7 @@ Makefile.in /tests/dwfl-bug-report /tests/dwfl-proc-attach /tests/dwfl-report-elf-align +/tests/dwfl-report-segment-contiguous /tests/dwfllines /tests/dwflmodtest /tests/dwflsyms diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog index b00ac8d6..09a4a473 100644 --- a/libdwfl/ChangeLog +++ b/libdwfl/ChangeLog @@ -1,10 +1,14 @@ 2019-12-11 Omar Sandoval * libdwflP.h: Add new NOT_LOADED DWFL_ERROR. + (Dwfl_Module): Remove coalescing state. + Rename lookup_tail_ndx to next_segndx. * relocate.c (__libdwfl_relocate_value): Return DWFL_E_NOT_LOADED if section is not loaded. (relocate): Handle DWFL_E_NOT_LOADED. * dwfl_module_getsym: Ignore DWFL_E_NOT_LOADED. + * segment.c (dwfl_report_segment): Remove coalescing logic. + * libdwfl.h (dwfl_report_segment): Document that IDENT is now ignored. 2019-12-05 Mark Wielaard diff --git a/libdwfl/libdwfl.h b/libdwfl/libdwfl.h index a0c1d357..d5fa06d4 100644 --- a/libdwfl/libdwfl.h +++ b/libdwfl/libdwfl.h @@ -111,7 +111,7 @@ extern void dwfl_report_begin (Dwfl *dwfl); /* Report that segment NDX begins at PHDR->p_vaddr + BIAS. If NDX is < 0, the value succeeding the last call's NDX - is used instead (zero on the first call). + is used instead (zero on the first call). IDENT is ignored. If nonzero, the smallest PHDR->p_align value seen sets the effective page size for the address space DWFL describes. @@ -120,21 +120,9 @@ extern void dwfl_report_begin (Dwfl *dwfl); Returns -1 for errors, or NDX (or its assigned replacement) on success. - When NDX is the value succeeding the last call's NDX (or is implicitly - so as above), IDENT is nonnull and matches the value in the last call, - and the PHDR and BIAS values reflect a segment that would be contiguous, - in both memory and file, with the last segment reported, then this - segment may be coalesced internally with preceding segments. When given - an address inside this segment, dwfl_addrsegment may return the NDX of a - preceding contiguous segment. To prevent coalesced segments, always - pass a null pointer for IDENT. - - The values passed are not stored (except to track coalescence). - The only information that can be extracted from DWFL later is the - mapping of an address to a segment index that starts at or below - it. Reporting segments at all is optional. Its only benefit to - the caller is to offer this quick lookup via dwfl_addrsegment, - or use other segment-based calls. */ + Reporting segments at all is optional. Its only benefit to the caller is to + offer this quick lookup via dwfl_addrsegment, or use other segment-based + calls. */ extern int dwfl_report_segment (Dwfl *dwfl, int ndx, const GElf_Phdr *phdr, GElf_Addr bias, const void *ident); diff --git a/libdwfl/libdwflP.h b/libdwfl/libdwflP.h index 6c10eddc..d21471b1 100644 --- a/libdwfl/libdwflP.h +++ b/libdwfl/libdwflP.h @@ -133,12 +133,7 @@ struct Dwfl GElf_Addr *lookup_addr; /* Start address of segment. */ Dwfl_Module **lookup_module; /* Module associated with segment, or null. */ int *lookup_segndx; /* User segment index, or -1. */ - - /* Cache from last dwfl_report_segment call. */ - const void *lookup_tail_ident; - GElf_Off lookup_tail_vaddr; - GElf_Off lookup_tail_offset; - int lookup_tail_ndx; + int next_segndx; struct Dwfl_User_Core *user_core; }; diff --git a/libdwfl/segment.c b/libdwfl/segment.c index d9599a7f..f6a3e84e 100644 --- a/libdwfl/segment.c +++ b/libdwfl/segment.c @@ -287,11 +287,15 @@ int dwfl_report_segment (Dwfl *dwfl, int ndx, const GElf_Phdr *phdr, GElf_Addr bias, const void *ident) { + /* This was previously used for coalescing segments, but it was buggy since + day one. We don't use it anymore. */ + (void)ident; + if (dwfl == NULL) return -1; if (ndx < 0) - ndx = dwfl->lookup_tail_ndx; + ndx = dwfl->next_segndx; if (phdr->p_align > 1 && (dwfl->segment_align <= 1 || phdr->p_align < dwfl->segment_align)) @@ -307,30 +311,19 @@ dwfl_report_segment (Dwfl *dwfl, int ndx, const GElf_Phdr *phdr, GElf_Addr bias, GElf_Addr end = __libdwfl_segment_end (dwfl, bias + phdr->p_vaddr + phdr->p_memsz); - /* Coalesce into the last one if contiguous and matching. */ - if (ndx != dwfl->lookup_tail_ndx - || ident == NULL - || ident != dwfl->lookup_tail_ident - || start != dwfl->lookup_tail_vaddr - || phdr->p_offset != dwfl->lookup_tail_offset) - { - /* Normally just appending keeps us sorted. */ + /* Normally just appending keeps us sorted. */ - size_t i = dwfl->lookup_elts; - while (i > 0 && unlikely (start < dwfl->lookup_addr[i - 1])) - --i; + size_t i = dwfl->lookup_elts; + while (i > 0 && unlikely (start < dwfl->lookup_addr[i - 1])) + --i; - if (unlikely (insert (dwfl, i, start, end, ndx))) - { - __libdwfl_seterrno (DWFL_E_NOMEM); - return -1; - } + if (unlikely (insert (dwfl, i, start, end, ndx))) + { + __libdwfl_seterrno (DWFL_E_NOMEM); + return -1; } - dwfl->lookup_tail_ident = ident; - dwfl->lookup_tail_vaddr = end; - dwfl->lookup_tail_offset = end - bias - phdr->p_vaddr + phdr->p_offset; - dwfl->lookup_tail_ndx = ndx + 1; + dwfl->next_segndx = ndx + 1; return ndx; } diff --git a/tests/ChangeLog b/tests/ChangeLog index ce67ce55..30bd8256 100644 --- a/tests/ChangeLog +++ b/tests/ChangeLog @@ -1,3 +1,8 @@ +2019-12-11 Omar Sandoval + + * dwfl-report-segment-coalesce.c: New test. + * Makefile.am: Add dwfl-report-segment-coalesce + 2019-12-06 Mark Wielaard * run-debuginfod-find.sh: Force -Wl,--build-id. diff --git a/tests/Makefile.am b/tests/Makefile.am index eab4ae6f..2c3ac299 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -50,7 +50,8 @@ check_PROGRAMS = arextract arsymtest newfile saridx scnnames sectiondump \ test-flag-nobits dwarf-getstring rerequest_tag \ alldts typeiter typeiter2 low_high_pc \ test-elf_cntl_gelf_getshdr dwflsyms dwfllines \ - dwfl-report-elf-align varlocs backtrace backtrace-child \ + dwfl-report-elf-align dwfl-report-segment-contiguous \ + varlocs backtrace backtrace-child \ backtrace-data backtrace-dwarf debuglink debugaltlink \ buildid deleted deleted-lib.so aggregate_size peel_type \ vdsosyms \ @@ -113,7 +114,7 @@ TESTS = run-arextract.sh run-arsymtest.sh run-ar.sh newfile test-nlist \ run-native-test.sh run-bug1-test.sh \ run-debuglink.sh run-debugaltlink.sh run-buildid.sh \ dwfl-bug-addr-overflow run-addrname-test.sh \ - dwfl-bug-fd-leak dwfl-bug-report \ + dwfl-bug-fd-leak dwfl-bug-report dwfl-report-segment-contiguous \ run-dwfl-bug-offline-rel.sh run-dwfl-addr-sect.sh \ run-disasm-x86.sh run-disasm-x86-64.sh \ run-early-offscn.sh run-dwarf-getmacros.sh run-dwarf-ranges.sh \ @@ -589,6 +590,7 @@ test_elf_cntl_gelf_getshdr_LDADD = $(libelf) dwflsyms_LDADD = $(libdw) $(libelf) $(argp_LDADD) dwfllines_LDADD = $(libdw) $(libelf) $(argp_LDADD) dwfl_report_elf_align_LDADD = $(libdw) +dwfl_report_segment_contiguous_LDADD = $(libdw) $(libebl) $(libelf) varlocs_LDADD = $(libdw) $(libelf) $(argp_LDADD) backtrace_LDADD = $(libdw) $(libelf) $(argp_LDADD) # backtrace-child-biarch also uses those *_CFLAGS and *_LDLAGS variables: diff --git a/tests/dwfl-report-segment-contiguous.c b/tests/dwfl-report-segment-contiguous.c new file mode 100644 index 00000000..61e6bfee --- /dev/null +++ b/tests/dwfl-report-segment-contiguous.c @@ -0,0 +1,82 @@ +/* Test bug in dwfl_report_segment() coalescing. + Copyright (C) 2019 Facebook + This file is part of elfutils. + + This file is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + elfutils is distributed in the hope that it will be useful, but + WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see . */ + +#include +#include +#include +#include +#include +#include +#include ELFUTILS_HEADER(dwfl) + + +static const Dwfl_Callbacks offline_callbacks = + { + .find_debuginfo = INTUSE(dwfl_standard_find_debuginfo), + .section_address = INTUSE(dwfl_offline_section_address), + }; + + +int +main (void) +{ + /* We use no threads here which can interfere with handling a stream. */ + (void) __fsetlocking (stdout, FSETLOCKING_BYCALLER); + + /* Set locale. */ + (void) setlocale (LC_ALL, ""); + + Dwfl *dwfl = dwfl_begin (&offline_callbacks); + assert (dwfl != NULL); + + GElf_Phdr phdr1 = + { + .p_type = PT_LOAD, + .p_flags = PF_R, + .p_offset = 0xf00, + .p_vaddr = 0xf00, + .p_filesz = 0x100, + .p_memsz = 0x100, + .p_align = 4, + }; + + int ndx = dwfl_report_segment (dwfl, 1, &phdr1, 0, dwfl); + assert(ndx == 1); + + ndx = dwfl_addrsegment (dwfl, 0xf00, NULL); + assert(ndx == 1); + + GElf_Phdr phdr2 = + { + .p_type = PT_LOAD, + .p_flags = PF_R | PF_W, + .p_offset = 0x1000, + .p_vaddr = 0x1000, + .p_filesz = 0x100, + .p_memsz = 0x100, + .p_align = 4, + }; + ndx = dwfl_report_segment (dwfl, 2, &phdr2, 0, dwfl); + assert(ndx == 2); + + ndx = dwfl_addrsegment (dwfl, 0x1000, NULL); + assert(ndx == 1 || ndx == 2); + + dwfl_end (dwfl); + + return 0; +} -- 2.24.0