On Dec 21, 2017, Jeff Law <l...@redhat.com> wrote: > On 12/11/2017 07:54 PM, Alexandre Oliva wrote: >> + ASM_GENERATE_INTERNAL_LABEL (label, "LVU", ied->view); >> + /* FIXME: this will resolve to a small number. Could we >> + possibly emit smaller data? Ideally we'd emit a >> + uleb128, but that would make the size of DIEs >> + impossible for the compiler to compute, since it's >> + the assembler that computes the value of the view >> + label in this case. Ideally, we'd have a single form >> + encompassing both the address and the view, and >> + indirecting them through a table might make things >> + easier, but even that would be more wasteful, >> + space-wise, than what we have now. */ >> + add_AT_lbl_id (die, DW_AT_GNU_entry_view, label);
> Do you have a sense of whether or not this really matters in practice? > how much bigger do we get due when we enable LVU? It's more of a matter of general design principle. DWARF strives to be compact, and outputting a very-likely small constant with most bits known to be zero is kind of against the rules. LVU proper doesn't run into this because the location view numbers are emitted as uleb128 constants in location list sections, never in the main debug section. How much it really grows depends on the representation: DWARF>5 loclists only get additional entries when at least one view number in a range pair is nonzero, and I have a sense that most view numbers in loclists are zero; for DWARF[2,5], we emit list of pairs corresponding to the ranges in each entry in the actual loclist, so we spend at least two bytes for both views, plus the pointer to the locviewlist in an additional attribute. Considering each range amounts to a pair of pointers plus at least two bytes for the location expression, and that each range gets a corresponding pair of views, and assuming all views will fit in a single uleb128 byte (128+ views at the same PC are very unlikely), the loclist section would grow by at most 20% with 32-bit pointers, and about half that much with 64-bit pointers. In reality, it ought to be a lot less than that, since location expressions's taking up a single byte (plus another byte representing the size) are common but hardly a majority of the cases. >> + /* Sanity check the block tree. This would catch a case in which >> + BLOCK got removed from the tree reachable from the outermost >> + lexical block, but got retained in markers. It would still link >> + back to its parents, but some ancestor would be missing a link >> + down the path to the sub BLOCK. If the block got removed, its >> + BLOCK_NUMBER will not be a usable value. */ >> + gcc_checking_assert (block_within_block_p (block, >> + DECL_INITIAL >> + (current_function_decl), >> + true)); >> + >> + gcc_assert (inlined_function_outer_scope_p (block)); >> + gcc_assert (!BLOCK_DIE (block)); >> + >> + /* If we can't represent it, don't bother. */ >> + if (!(dwarf_version >= 3 || !dwarf_strict)) >> + return; > Consider moving this check earlier. I don't think it's a hard > requirement, so if you put it after the asserts for a reason, then leave > it has is. The reason I put the check after the asserts was that I wanted to catch messed up block trees even if we wouldn't emit the entry point after all. Now, considering the block tree messing up was figured out, I guess moving the test earlier and saving the cycles if we're not emitting the annotation makes sense. Will do. > Generally I think this is fine (it's much simpler than the dwarf2 bits > of the LVU patch. Yeah. I wonder if there's anything I can do, now that I'm back from vacations, to help get the LVU patch reviewed. I suppose it might still be eligible for GCC 8, considering it was posted even before stage3, let alone stage4, and considering it only deals with debug info (mainly location lists), but then... unlike the SFN stuff, the additional information it outputs will only make a difference once debug info consumers learn to use it. So maybe we could afford to leave it out, or to bring it in but disabled. Thoughts? Does it make sense at this point for me to pester ^W entice some review to have a look at it? Thanks, -- Alexandre Oliva, freedom fighter http://FSFLA.org/~lxoliva/ You must be the change you wish to see in the world. -- Gandhi Be Free! -- http://FSFLA.org/ FSF Latin America board member Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer