<div dir="ltr">Well, that's weird. Thanks for reverting!<div><br></div><div>It's probably some weird corruption with the format string (uint64_t passed to "%d"). I'll try to reland it with some changes and watch the 32bit build bots.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Oct 17, 2019 at 1:50 AM Hans Wennborg <<a href="mailto:hans@chromium.org">hans@chromium.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">This somehow broke a bunch of tests in 32-bit builds, e.g.<br>
<a href="http://lab.llvm.org:8011/builders/clang-cmake-armv7-quick/builds/10925" rel="noreferrer" target="_blank">http://lab.llvm.org:8011/builders/clang-cmake-armv7-quick/builds/10925</a><br>
<br>
I've reverted in r375088.<br>
<br>
On Tue, Oct 15, 2019 at 8:10 PM Jordan Rupprecht via llvm-commits<br>
<<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
><br>
> Author: rupprecht<br>
> Date: Tue Oct 15 11:13:20 2019<br>
> New Revision: 374931<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=374931&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=374931&view=rev</a><br>
> Log:<br>
> [llvm-objdump] Use a counter for llvm-objdump -h instead of the section index.<br>
><br>
> Summary:<br>
> When listing the index in `llvm-objdump -h`, use a zero-based counter instead of the actual section index (e.g. shdr->sh_index for ELF).<br>
><br>
> While this is effectively a noop for now (except one unit test for XCOFF), the index values will change in a future patch that filters certain sections out (e.g. symbol tables). See D68669 for more context. Note: the test case in `test/tools/llvm-objdump/X86/section-index.s` already covers the case of incrementing the section index counter when sections are skipped.<br>
><br>
> Reviewers: grimar, jhenderson, espindola<br>
><br>
> Reviewed By: grimar<br>
><br>
> Subscribers: emaste, sbc100, arichardson, aheejin, arphaman, seiya, llvm-commits, MaskRay<br>
><br>
> Tags: #llvm<br>
><br>
> Differential Revision: <a href="https://reviews.llvm.org/D68848" rel="noreferrer" target="_blank">https://reviews.llvm.org/D68848</a><br>
><br>
> Modified:<br>
> llvm/trunk/test/tools/llvm-objdump/xcoff-section-headers.test<br>
> llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp<br>
> llvm/trunk/tools/llvm-objdump/llvm-objdump.h<br>
><br>
> Modified: llvm/trunk/test/tools/llvm-objdump/xcoff-section-headers.test<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-objdump/xcoff-section-headers.test?rev=374931&r1=374930&r2=374931&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-objdump/xcoff-section-headers.test?rev=374931&r1=374930&r2=374931&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/test/tools/llvm-objdump/xcoff-section-headers.test (original)<br>
> +++ llvm/trunk/test/tools/llvm-objdump/xcoff-section-headers.test Tue Oct 15 11:13:20 2019<br>
> @@ -13,11 +13,11 @@<br>
> # CHECK: xcoff-section-headers.o: file format aixcoff-rs6000<br>
> # CHECK: Sections:<br>
> # CHECK: Idx Name Size VMA Type<br>
> -# CHECK: 1 .text 00000080 00000000 TEXT<br>
> -# CHECK: 2 .data 00000024 00000080 DATA<br>
> -# CHECK: 3 .bss 00000004 000000a4 BSS<br>
> -# CHECK: 4 .tdata 00000008 00000000 DATA<br>
> -# CHECK: 5 .tbss 00000004 00000008 BSS<br>
> +# CHECK: 0 .text 00000080 00000000 TEXT<br>
> +# CHECK: 1 .data 00000024 00000080 DATA<br>
> +# CHECK: 2 .bss 00000004 000000a4 BSS<br>
> +# CHECK: 3 .tdata 00000008 00000000 DATA<br>
> +# CHECK: 4 .tbss 00000004 00000008 BSS<br>
><br>
> # xcoff-section-headers.o Compiled with IBM XL C/C++ for AIX, V16.1.0<br>
> # test.c:<br>
> @@ -32,10 +32,10 @@<br>
><br>
> # LONG: xcoff-long-sec-names.o: file format aixcoff-rs6000<br>
> # LONG: Sections:<br>
> -# LONG: Idx Name Size VMA Type<br>
> -# LONG: 1 .dwarnge 00000004 00000000<br>
> -# LONG: 2 .dwpbnms 00000004 00000000<br>
> -# LONG: 3 .dwpbtyp 00000004 00000000<br>
> +# LONG: Idx Name Size VMA Type<br>
> +# LONG: 0 .dwarnge 00000004 00000000<br>
> +# LONG: 1 .dwpbnms 00000004 00000000<br>
> +# LONG: 2 .dwpbtyp 00000004 00000000<br>
><br>
> # xcoff-long-sec-names.o was generated by assembling the following .s file:<br>
> # .dwsect 0x30000 # .dwpbnms section<br>
><br>
> Modified: llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp?rev=374931&r1=374930&r2=374931&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp?rev=374931&r1=374930&r2=374931&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp (original)<br>
> +++ llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp Tue Oct 15 11:13:20 2019<br>
> @@ -342,14 +342,27 @@ static StringRef ToolName;<br>
><br>
> typedef std::vector<std::tuple<uint64_t, StringRef, uint8_t>> SectionSymbolsTy;<br>
><br>
> -static bool shouldKeep(object::SectionRef S) {<br>
> +namespace {<br>
> +struct FilterResult {<br>
> + // True if the section should not be skipped.<br>
> + bool Keep;<br>
> +<br>
> + // True if the index counter should be incremented, even if the section should<br>
> + // be skipped. For example, sections may be skipped if they are not included<br>
> + // in the --section flag, but we still want those to count toward the section<br>
> + // count.<br>
> + bool IncrementIndex;<br>
> +};<br>
> +} // namespace<br>
> +<br>
> +static FilterResult checkSectionFilter(object::SectionRef S) {<br>
> if (FilterSections.empty())<br>
> - return true;<br>
> + return {/*Keep=*/true, /*IncrementIndex=*/true};<br>
><br>
> Expected<StringRef> SecNameOrErr = S.getName();<br>
> if (!SecNameOrErr) {<br>
> consumeError(SecNameOrErr.takeError());<br>
> - return false;<br>
> + return {/*Keep=*/false, /*IncrementIndex=*/false};<br>
> }<br>
> StringRef SecName = *SecNameOrErr;<br>
><br>
> @@ -357,11 +370,26 @@ static bool shouldKeep(object::SectionRe<br>
> // no name (such as the section with index 0) here.<br>
> if (!SecName.empty())<br>
> FoundSectionSet.insert(SecName);<br>
> - return is_contained(FilterSections, SecName);<br>
> -}<br>
><br>
> -SectionFilter ToolSectionFilter(object::ObjectFile const &O) {<br>
> - return SectionFilter([](object::SectionRef S) { return shouldKeep(S); }, O);<br>
> + // Only show the section if it's in the FilterSections list, but always<br>
> + // increment so the indexing is stable.<br>
> + return {/*Keep=*/is_contained(FilterSections, SecName),<br>
> + /*IncrementIndex=*/true};<br>
> +}<br>
> +<br>
> +SectionFilter ToolSectionFilter(object::ObjectFile const &O, uint64_t *Idx) {<br>
> + // Start at UINT64_MAX so that the first index returned after an increment is<br>
> + // zero (after the unsigned wrap).<br>
> + if (Idx)<br>
> + *Idx = UINT64_MAX;<br>
> + return SectionFilter(<br>
> + [Idx](object::SectionRef S) {<br>
> + FilterResult Result = checkSectionFilter(S);<br>
> + if (Idx != nullptr && Result.IncrementIndex)<br>
> + *Idx += 1;<br>
> + return Result.Keep;<br>
> + },<br>
> + O);<br>
> }<br>
><br>
> std::string getFileNameForError(const object::Archive::Child &C,<br>
> @@ -967,7 +995,7 @@ getRelocsMap(object::ObjectFile const &O<br>
> std::map<SectionRef, std::vector<RelocationRef>> Ret;<br>
> for (SectionRef Sec : Obj.sections()) {<br>
> section_iterator Relocated = Sec.getRelocatedSection();<br>
> - if (Relocated == Obj.section_end() || !shouldKeep(*Relocated))<br>
> + if (Relocated == Obj.section_end() || !checkSectionFilter(*Relocated).Keep)<br>
> continue;<br>
> std::vector<RelocationRef> &V = Ret[*Relocated];<br>
> for (const RelocationRef &R : Sec.relocations())<br>
> @@ -1676,7 +1704,8 @@ void printSectionHeaders(const ObjectFil<br>
> << left_justify("Name", NameWidth) << " Size "<br>
> << left_justify("VMA", AddressWidth) << " Type\n";<br>
><br>
> - for (const SectionRef &Section : ToolSectionFilter(*Obj)) {<br>
> + uint64_t Idx;<br>
> + for (const SectionRef &Section : ToolSectionFilter(*Obj, &Idx)) {<br>
> StringRef Name = unwrapOrError(Section.getName(), Obj->getFileName());<br>
> uint64_t VMA = Section.getAddress();<br>
> if (shouldAdjustVA(Section))<br>
> @@ -1691,14 +1720,14 @@ void printSectionHeaders(const ObjectFil<br>
> Type += Type.empty() ? "BSS" : " BSS";<br>
><br>
> if (HasLMAColumn)<br>
> - outs() << format("%3d %-*s %08" PRIx64 " ", (unsigned)Section.getIndex(),<br>
> - NameWidth, Name.str().c_str(), Size)<br>
> + outs() << format("%3d %-*s %08" PRIx64 " ", Idx, NameWidth,<br>
> + Name.str().c_str(), Size)<br>
> << format_hex_no_prefix(VMA, AddressWidth) << " "<br>
> << format_hex_no_prefix(getELFSectionLMA(Section), AddressWidth)<br>
> << " " << Type << "\n";<br>
> else<br>
> - outs() << format("%3d %-*s %08" PRIx64 " ", (unsigned)Section.getIndex(),<br>
> - NameWidth, Name.str().c_str(), Size)<br>
> + outs() << format("%3d %-*s %08" PRIx64 " ", Idx, NameWidth,<br>
> + Name.str().c_str(), Size)<br>
> << format_hex_no_prefix(VMA, AddressWidth) << " " << Type << "\n";<br>
> }<br>
> outs() << "\n";<br>
><br>
> Modified: llvm/trunk/tools/llvm-objdump/llvm-objdump.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-objdump/llvm-objdump.h?rev=374931&r1=374930&r2=374931&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-objdump/llvm-objdump.h?rev=374931&r1=374930&r2=374931&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/tools/llvm-objdump/llvm-objdump.h (original)<br>
> +++ llvm/trunk/tools/llvm-objdump/llvm-objdump.h Tue Oct 15 11:13:20 2019<br>
> @@ -31,6 +31,8 @@ extern cl::opt<bool> Demangle;<br>
><br>
> typedef std::function<bool(llvm::object::SectionRef const &)> FilterPredicate;<br>
><br>
> +/// A filtered iterator for SectionRefs that skips sections based on some given<br>
> +/// predicate.<br>
> class SectionFilterIterator {<br>
> public:<br>
> SectionFilterIterator(FilterPredicate P,<br>
> @@ -60,6 +62,8 @@ private:<br>
> llvm::object::section_iterator End;<br>
> };<br>
><br>
> +/// Creates an iterator range of SectionFilterIterators for a given Object and<br>
> +/// predicate.<br>
> class SectionFilter {<br>
> public:<br>
> SectionFilter(FilterPredicate P, llvm::object::ObjectFile const &O)<br>
> @@ -79,7 +83,15 @@ private:<br>
> };<br>
><br>
> // Various helper functions.<br>
> -SectionFilter ToolSectionFilter(llvm::object::ObjectFile const &O);<br>
> +<br>
> +/// Creates a SectionFilter with a standard predicate that conditionally skips<br>
> +/// sections when the --section objdump flag is provided.<br>
> +///<br>
> +/// Idx is an optional output parameter that keeps track of which section index<br>
> +/// this is. This may be different than the actual section number, as some<br>
> +/// sections may be filtered (e.g. symbol tables).<br>
> +SectionFilter ToolSectionFilter(llvm::object::ObjectFile const &O,<br>
> + uint64_t *Idx = nullptr);<br>
><br>
> Error getELFRelocationValueString(const object::ELFObjectFileBase *Obj,<br>
> const object::RelocationRef &Rel,<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>