Re: [SFN+LVU+IEPM v4 7/9] [LVU] Introduce location views

[Andreas Schwab]

On Feb 13 2018, Alexandre Oliva  wrote:

> The patch I posted last night should work around this problem, in that
> it will disable LVU by default if the assembler doesn't support .loc
> views, and then you won't get this error any more, unless you explicitly
> ask for location views.  If you can give it a try on ia64-linux-gnu,
> that would be appreciated.

That fixes the build failure on ia64.

Andreas.

-- 
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [SFN+LVU+IEPM v4 7/9] [LVU] Introduce location views

[Alexandre Oliva]

On Feb 12, 2018, Andreas Schwab  wrote:

> On Feb 12 2018, Alexandre Oliva  wrote:
>> On Feb 11, 2018, Andreas Schwab  wrote:
>> 
>>> On Feb 09 2018, Alexandre Oliva  wrote:
>> 
 +  if (list_head->vl_symbol && dwarf2out_locviews_in_attribute ())
 +{
 +  ASM_OUTPUT_LABEL (asm_out_file, list_head->vl_symbol);
>> 
>>> That needs to use ASM_OUTPUT_DEBUG_LABEL.
>> 
>> Note this is always output in the .debug_loclist section, not in code
>> sections, so I don't get why it should matter.  Care to clarify, please?

> Perhaps I'm misunderstanding it, but I see .LM labels emitted in the
> middle of code bundles, which breaks them apart.

That line would only output .LVUS symbols.

The only line that outputs LM symbols is 

  targetm.asm_out.internal_label (asm_out_file, LINE_CODE_LABEL, label_num);

It's not ASM_OUTPUT_DEBUG_LABEL, but it was there before.

What may have changed is that, using an assembler without .loc view
support, GCC would switch to internal line number tables, which requires
it to emit LM labels at points in which it would otherwise have output
.loc directives.

The patch I posted last night should work around this problem, in that
it will disable LVU by default if the assembler doesn't support .loc
views, and then you won't get this error any more, unless you explicitly
ask for location views.  If you can give it a try on ia64-linux-gnu,
that would be appreciated.  You might also want to give it a spin with
GNU as 2.30: the assembler view mismatch errors you'd get before that
patch should now be gone too, because we no longer trust min insn
lengths to compute view reset points internally.

If you force that on, with -ginternal-reset-location-views, you'll get
errors in at meast some of the cases in which the assembler finds the
GCC-computed insn length mismatches the assembler's, and then you can
fix the lengths in GCC, so that eventually we can mark this port as one
whose lengths can be used to this end.  See also the reset_location_view
target hook introduced in the same patch.

https://gcc.gnu.org/ml/gcc-patches/2018-02/msg00624.html

-- 
Alexandre Oliva, freedom fighterhttp://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

Re: [SFN+LVU+IEPM v4 7/9] [LVU] Introduce location views

[Andreas Schwab]

On Feb 12 2018, Alexandre Oliva  wrote:

> On Feb 11, 2018, Andreas Schwab  wrote:
>
>> On Feb 09 2018, Alexandre Oliva  wrote:
>
>>> +  if (list_head->vl_symbol && dwarf2out_locviews_in_attribute ())
>>> +{
>>> +  ASM_OUTPUT_LABEL (asm_out_file, list_head->vl_symbol);
>
>> That needs to use ASM_OUTPUT_DEBUG_LABEL.
>
> Note this is always output in the .debug_loclist section, not in code
> sections, so I don't get why it should matter.  Care to clarify, please?

Perhaps I'm misunderstanding it, but I see .LM labels emitted in the
middle of code bundles, which breaks them apart.

Andreas.

-- 
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [SFN+LVU+IEPM v4 7/9] [LVU] Introduce location views

[Alexandre Oliva]

On Feb 11, 2018, Andreas Schwab  wrote:

> On Feb 09 2018, Alexandre Oliva  wrote:

>> +  if (list_head->vl_symbol && dwarf2out_locviews_in_attribute ())
>> +{
>> +  ASM_OUTPUT_LABEL (asm_out_file, list_head->vl_symbol);

> That needs to use ASM_OUTPUT_DEBUG_LABEL.

Note this is always output in the .debug_loclist section, not in code
sections, so I don't get why it should matter.  Care to clarify, please?

-- 
Alexandre Oliva, freedom fighterhttp://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

Re: [SFN+LVU+IEPM v4 7/9] [LVU] Introduce location views

[Alexandre Oliva]

On Feb 11, 2018, Andreas Schwab  wrote:

>> +  if (list_head->vl_symbol && dwarf2out_locviews_in_attribute ())
>> +{
>> +  ASM_OUTPUT_LABEL (asm_out_file, list_head->vl_symbol);

> That needs to use ASM_OUTPUT_DEBUG_LABEL.

There's another use of the same macro that was already there, right
before the locview block output:

  ASM_OUTPUT_LABEL (asm_out_file, list_head->ll_symbol);

Shouldn't it be ASM_OUTPUT_DEBUG_LABEL too?


is fixing these enough to address the problem you reported?

Thanks,

-- 
Alexandre Oliva, freedom fighterhttp://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

Re: [SFN+LVU+IEPM v4 7/9] [LVU] Introduce location views

[Andreas Schwab]

On Feb 09 2018, Alexandre Oliva  wrote:

> @@ -9681,34 +9978,85 @@ gen_llsym (dw_loc_list_ref list)
>  static void
>  output_loc_list (dw_loc_list_ref list_head)
>  {
> +  int vcount = 0, lcount = 0;
> +
>if (list_head->emitted)
>  return;
>list_head->emitted = true;
>  
> +  if (list_head->vl_symbol && dwarf2out_locviews_in_attribute ())
> +{
> +  ASM_OUTPUT_LABEL (asm_out_file, list_head->vl_symbol);

That needs to use ASM_OUTPUT_DEBUG_LABEL.

Andreas.

-- 
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [SFN+LVU+IEPM v4 7/9] [LVU] Introduce location views

[Andreas Schwab]

On Feb 09 2018, Alexandre Oliva  wrote:

> [LVU] Introduce location views
>
> This patch introduces an option to enable the generation of location
> views along with location lists.  The exact format depends on the
> DWARF version: it can be a separate attribute (DW_AT_GNU_locviews) or
> (DW_LLE_view_pair) entries in DWARF5+ loclists.

This breaks ia64 bootstrap.  While building the stage3 compiler I get
this error:

/usr/local/gcc/gcc-20180211/Build/./prev-gcc/xg++ 
-B/usr/local/gcc/gcc-20180211/Build/./prev-gcc/ -B/usr/ia64-suse-linux/bin/ 
-nostdinc++ 
-B/usr/local/gcc/gcc-20180211/Build/prev-ia64-suse-linux/libstdc++-v3/src/.libs 
-B/usr/local/gcc/gcc-20180211/Build/prev-ia64-suse-linux/libstdc++-v3/libsupc++/.libs
  
-I/usr/local/gcc/gcc-20180211/Build/prev-ia64-suse-linux/libstdc++-v3/include/ia64-suse-linux
  -I/usr/local/gcc/gcc-20180211/Build/prev-ia64-suse-linux/libstdc++-v3/include 
 -I/usr/local/gcc/gcc-20180211/libstdc++-v3/libsupc++ 
-L/usr/local/gcc/gcc-20180211/Build/prev-ia64-suse-linux/libstdc++-v3/src/.libs 
-L/usr/local/gcc/gcc-20180211/Build/prev-ia64-suse-linux/libstdc++-v3/libsupc++/.libs
 -fno-PIE -c  -DUSE_LIBUNWIND_EXCEPTIONS -DIN_GCC_FRONTEND -g -O2 -DIN_GCC 
-fno-exceptions -fno-rtti -fasynchronous-unwind-tables -W -Wall -Wno-narrowing 
-Wwrite-strings -Wcast-qual -Wmissing-format-attribute -Woverloaded-virtual 
-pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -Werror 
-fno-common  -DHAVE_CONFIG_H -I. -Ilto -I../../gcc -I../../gcc/lto 
-I../../gcc/../include -I../../gcc/../libcpp/include  
-I../../gcc/../libdecnumber -I../../gcc/../libdecnumber/dpd -I../libdecnumber 
-I../../gcc/../libbacktrace   -o lto/lto-lang.o -MT lto/lto-lang.o -MMD -MP -MF 
lto/.deps/lto-lang.TPo ../../gcc/lto/lto-lang.c
/tmp/cccLpNWi.s: Assembler messages:
/tmp/cccLpNWi.s:1058101: Error: value of 89616 too large for field of 2 bytes 
at 502338
make[3]: *** [lto/lto-lang.o] Error 1
make[3]: Leaving directory `/usr/local/gcc/gcc-20180211/Build/gcc'
make[2]: *** [all-stage3-gcc] Error 2
make[2]: Leaving directory `/usr/local/gcc/gcc-20180211/Build'
make[1]: *** [stage3-bubble] Error 2
make[1]: Leaving directory `/usr/local/gcc/gcc-20180211/Build'
make: *** [all] Error 2

The erroneous line has "data2.ua .LM78805-.LM78804", where .LM78805
points to the start of a function, and .LM78804 is the last of a long
list of labels (with consecutive numbers) all pointing to the same
address in the middle of an unrelated function.  Also, most of the .LM
labels point to insns in the middle of a bundle without using the
bracket notation.

Andreas.

-- 
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [SFN+LVU+IEPM v4 7/9] [LVU] Introduce location views

[Alexandre Oliva]

On Feb  8, 2018, Jason Merrill  wrote:

> On Thu, Feb 8, 2018 at 7:56 AM, Alexandre Oliva  wrote:
>> On Feb  7, 2018, Jason Merrill  wrote:
>> 
>>> OK, that makes sense.  But I'm still uncomfortable with choosing an
>>> existing opcode for that purpose, which previously would have been
>>> chosen just for reasons of encoding complexity and size.
>> 
>> Well, there's a good reason we didn't used to output this opcode: it's
>> nearly always the case that you're better off using a special opcode or
>> DW_LNS_advance_pc, that encodes the offset as uleb128 instead of a fixed
>> size.  The only exceptions I can think of are offsets that have the most
>> significant bits set in the representable range for
>> DW_LNS_fixed_advance_pc (the uleb128 representation for
>> DW_LNS_advance_pc would end up taking an extra byte if insns don't get
>> more than byte alignment), and VLIW machines, in which the
>> DW_LNS_advance_pc operand needs to be multiplied by the ops-per-insns
>> (but also divided by the min-insn-length).  So, unless you're creating a
>> gap of 16KiB to 64KiB in the middle of a function on an ISA such as
>> x86*, that has insns as small as 1 byte, you'll only use
>> DW_LNS_fixed_advance_pc when the assembler can't encode uleb128 offsets,
>> as stated in the DWARF specs.

> Which is often true of non-gas assemblers, isn't it?

Uhh...  I don't know.  Anyway, without gas, we probably won't have view
supports in .loc directives either, and then we'll have to emit the line
number program ourselves, in which case we could (if we have accurate
insn lengths) compute offsets and output special opcodes or
byte-expanded uleb128 literals, even if the assembler doesn't support
leb128.  Sure enough, if we don't have accurate insn lengths, we'll have
to resort to something else; for small increments without assembler
support for uleb128, DW_LNS_fixed_advance_pc would probably be a more
compact option than DW_LNS_set_address, if we're sure the increment is
small enough.

Even then, if we were to do something along these lines, when we know
the offset is nonzero (i.e., we want to reset the view), we could output
DW_LNS_fixed_advance_pc with the desired offset minus 1, followed by a
special opcode that advances PC (resetting the view count) and emitting
the line table entry, rather than using DW_LNS_fixed_advance_pc with the
offset, followed by DW_LNS_copy to output the line number table without
resetting the view.  Same size, with the choice of resetting or not the
view counter.  As for when we don't know whether the offset could be
zero, we have only one choice: not resetting the view counter.

> So, if I've got this right: The most conservative approach to updating
> the address is DW_LNE_set_address, but we definitely want that to
> reset the view because it's used for e.g. starting a new function.

Yup.

> And if it resets the view, we need to be careful not to use it more
> than once for the same address.

I guess it's ok if we do that e.g. once as the one-past-the-end of a
function, and then again as the entry of another function, but other
than that, yeah, it shouldn't happen.

  [...] 
> And since we don't know whether the increment will be zero, we
> don't want it to reset the view.

Yup

> OK, that makes sense.  Though I expect this will come up again when
> the DWARF committee looks at the proposal.

Most likely.  The reasoning that led me down this path was quite
intricate indeed.

Please let it be known that I'd be glad to join the committee's
conversation when they're to discuss this proposal, if that would be of
any help.

Thanks,

-- 
Alexandre Oliva, freedom fighterhttp://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

Re: [SFN+LVU+IEPM v4 7/9] [LVU] Introduce location views

[Alexandre Oliva]

On Feb  8, 2018, Jason Merrill  wrote:

> On 02/07/2018 02:36 AM, Alexandre Oliva wrote:
>> +/* Output symbol LAB1 as an unsigned LEB128 quantity.  */

> Let's mention here that the value of LAB1 must be an assemble-time
> constant (such as a view counter), since we can't have LEB128
> relocations.

/* Output symbol LAB1 as an unsigned LEB128 quantity.  LAB1 should be
   an assembler-computed constant, e.g. a view number, because we
   can't have relocations in LEB128 quantities.  */

> With that, the patch looks OK.

Thanks, here's what I checked in.


[LVU] Introduce location views

This patch introduces an option to enable the generation of location
views along with location lists.  The exact format depends on the
DWARF version: it can be a separate attribute (DW_AT_GNU_locviews) or
(DW_LLE_view_pair) entries in DWARF5+ loclists.

Line number tables are also affected.  If the assembler is found, at
compiler build time, to support .loc views, we use them and
assembler-computed view labels, otherwise we output compiler-generated
line number programs with conservatively-computed view labels.  In
either case, we output view information next to line number changes
when verbose assembly output is requested.

This patch requires an LVU patch that modifies the exported API of
final_scan_insn.  It also expects the entire SFN patchset to be
installed first, although SFN is not a requirement for LVU.

for  include/ChangeLog

* dwarf2.def (DW_AT_GNU_locviews): New.
* dwarf2.h (enum dwarf_location_list_entry_type): Add
DW_LLE_GNU_view_pair.
(DW_LLE_view_pair): Define.

for  gcc/ChangeLog

* common.opt (gvariable-location-views): New.
(gvariable-location-views=incompat5): New.
* config.in: Rebuilt.
* configure: Rebuilt.
* configure.ac: Test assembler for view support.
* dwarf2asm.c (dw2_asm_output_symname_uleb128): New.
* dwarf2asm.h (dw2_asm_output_symname_uleb128): Declare.
* dwarf2out.c (var_loc_view): New typedef.
(struct dw_loc_list_struct): Add vl_symbol, vbegin, vend.
(dwarf2out_locviews_in_attribute): New.
(dwarf2out_locviews_in_loclist): New.
(dw_val_equal_p): Compare val_view_list of dw_val_class_view_lists.
(enum dw_line_info_opcode): Add LI_adv_address.
(struct dw_line_info_table): Add view.
(RESET_NEXT_VIEW, RESETTING_VIEW_P): New macros.
(DWARF2_ASM_VIEW_DEBUG_INFO): Define default.
(zero_view_p): New variable.
(ZERO_VIEW_P): New macro.
(output_asm_line_debug_info): New.
(struct var_loc_node): Add view.
(add_AT_view_list, AT_loc_list): New.
(add_var_loc_to_decl): Add view param.  Test it against last.
(new_loc_list): Add view params.  Record them.
(AT_loc_list_ptr): Handle loc and view lists.
(view_list_to_loc_list_val_node): New.
(print_dw_val): Handle dw_val_class_view_list.
(size_of_die): Likewise.
(value_format): Likewise.
(loc_list_has_views): New.
(gen_llsym): Set vl_symbol too.
(maybe_gen_llsym, skip_loc_list_entry): New.
(dwarf2out_maybe_output_loclist_view_pair): New.
(output_loc_list): Output view list or entries too.
(output_view_list_offset): New.
(output_die): Handle dw_val_class_view_list.
(output_dwarf_version): New.
(output_compilation_unit_header): Use it.
(output_skeleton_debug_sections): Likewise.
(output_rnglists, output_line_info): Likewise.
(output_pubnames, output_aranges): Update version comments.
(output_one_line_info_table): Output view numbers in asm comments.
(dw_loc_list): Determine current endview, pass it to new_loc_list.
Call maybe_gen_llsym.
(loc_list_from_tree_1): Adjust.
(add_AT_location_description): Create view list attribute if
needed, check it's absent otherwise.
(convert_cfa_to_fb_loc_list): Adjust.
(maybe_emit_file): Call output_asm_line_debug_info for test.
(dwarf2out_var_location): Reset views as needed.  Precompute
add_var_loc_to_decl args.  Call get_attr_min_length only if we have the
attribute.  Set view.
(new_line_info_table): Reset next view.
(set_cur_line_info_table): Call output_asm_line_debug_info for test.
(dwarf2out_source_line): Likewise.  Output view resets and labels to
the assembler, or select appropriate line info opcodes.
(prune_unused_types_walk_attribs): Handle dw_val_class_view_list.
(optimize_string_length): Catch it.  Adjust.
(resolve_addr): Copy vl_symbol along with ll_symbol.  Handle
dw_val_class_view_list, and remove it if no longer needed.
(hash_loc_list): Hash view numbers.
(loc_list_hasher::equal): Compare them.
(optimize_location_lists): Check whether a view list symbol is
needed, and 

Re: [SFN+LVU+IEPM v4 7/9] [LVU] Introduce location views

[Jason Merrill]

On 02/07/2018 02:36 AM, Alexandre Oliva wrote:

+/* Output symbol LAB1 as an unsigned LEB128 quantity.  */


Let's mention here that the value of LAB1 must be an assemble-time 
constant (such as a view counter), since we can't have LEB128 relocations.


With that, the patch looks OK.

Jason

Re: [SFN+LVU+IEPM v4 7/9] [LVU] Introduce location views

[Jason Merrill]

On Thu, Feb 8, 2018 at 7:56 AM, Alexandre Oliva  wrote:
> On Feb  7, 2018, Jason Merrill  wrote:
>
>> OK, that makes sense.  But I'm still uncomfortable with choosing an
>> existing opcode for that purpose, which previously would have been
>> chosen just for reasons of encoding complexity and size.
>
> Well, there's a good reason we didn't used to output this opcode: it's
> nearly always the case that you're better off using a special opcode or
> DW_LNS_advance_pc, that encodes the offset as uleb128 instead of a fixed
> size.  The only exceptions I can think of are offsets that have the most
> significant bits set in the representable range for
> DW_LNS_fixed_advance_pc (the uleb128 representation for
> DW_LNS_advance_pc would end up taking an extra byte if insns don't get
> more than byte alignment), and VLIW machines, in which the
> DW_LNS_advance_pc operand needs to be multiplied by the ops-per-insns
> (but also divided by the min-insn-length).  So, unless you're creating a
> gap of 16KiB to 64KiB in the middle of a function on an ISA such as
> x86*, that has insns as small as 1 byte, you'll only use
> DW_LNS_fixed_advance_pc when the assembler can't encode uleb128 offsets,
> as stated in the DWARF specs.

Which is often true of non-gas assemblers, isn't it?

> Well, now there's one more case for using
> it, and it's a rare one as well.  I didn't think it made sense to add
> yet another opcode with a fixed-size operand for that.

So, if I've got this right: The most conservative approach to updating
the address is DW_LNE_set_address, but we definitely want that to
reset the view because it's used for e.g. starting a new function.
And if it resets the view, we need to be careful not to use it more
than once for the same address.  Special opcodes are only generated by
the assembler from .loc directives, so we use symbolic views and let
the assembler decide whether or not they reset the view.  If we don't
have that assembler support, and don't want to use DW_LNE_set_address,
that leaves DW_LNS_advance_pc and DW_LNS_fixed_advance_pc.  The former
could be used by the assembler in cases when special opcodes can't
express the necessary change compactly, whereas the latter won't be,
so it's a better choice for this situation.  And since we don't know
whether the increment will be zero, we don't want it to reset the
view.

OK, that makes sense.  Though I expect this will come up again when
the DWARF committee looks at the proposal.

> But then again, even it was an opcode used more often, it wouldn't be a
> significant problem to assign it the special behavior of not resetting
> the view counter.  Views don't have to be reset for the whole thing to
> work, we just need some means for the compiler (who doesn't know all
> offsets) and debug info consumers (who do) to keep view numbers in sync.
> A single opcode for the compiler to signal to the consumer that it
> wasn't sure a smallish offset would be nonzero is enough, and
> DW_LNS_fixed_advance_pc provides us with just what we need, without any
> complications of having to compute a special opcode, or compute a
> compiler-unknown offset times min-insn-length and have that encoded in
> uleb128.
>
>> Thanks, it would be good to have this overview in a comment somewhere.
>
> You meant just these two paragraphs (like below), or the whole thing?

I meant just the two paragraphs, thanks.

Jason

Re: [SFN+LVU+IEPM v4 7/9] [LVU] Introduce location views

[Alexandre Oliva]

On Feb  7, 2018, Jason Merrill  wrote:

> OK, that makes sense.  But I'm still uncomfortable with choosing an
> existing opcode for that purpose, which previously would have been
> chosen just for reasons of encoding complexity and size.

Well, there's a good reason we didn't used to output this opcode: it's
nearly always the case that you're better off using a special opcode or
DW_LNS_advance_pc, that encodes the offset as uleb128 instead of a fixed
size.  The only exceptions I can think of are offsets that have the most
significant bits set in the representable range for
DW_LNS_fixed_advance_pc (the uleb128 representation for
DW_LNS_advance_pc would end up taking an extra byte if insns don't get
more than byte alignment), and VLIW machines, in which the
DW_LNS_advance_pc operand needs to be multiplied by the ops-per-insns
(but also divided by the min-insn-length).  So, unless you're creating a
gap of 16KiB to 64KiB in the middle of a function on an ISA such as
x86*, that has insns as small as 1 byte, you'll only use
DW_LNS_fixed_advance_pc when the assembler can't encode uleb128 offsets,
as stated in the DWARF specs.  Well, now there's one more case for using
it, and it's a rare one as well.  I didn't think it made sense to add
yet another opcode with a fixed-size operand for that.

But then again, even it was an opcode used more often, it wouldn't be a
significant problem to assign it the special behavior of not resetting
the view counter.  Views don't have to be reset for the whole thing to
work, we just need some means for the compiler (who doesn't know all
offsets) and debug info consumers (who do) to keep view numbers in sync.
A single opcode for the compiler to signal to the consumer that it
wasn't sure a smallish offset would be nonzero is enough, and
DW_LNS_fixed_advance_pc provides us with just what we need, without any
complications of having to compute a special opcode, or compute a
compiler-unknown offset times min-insn-length and have that encoded in
uleb128.


> Thanks, it would be good to have this overview in a comment somewhere.

You meant just these two paragraphs (like below), or the whole thing?


diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index 3cf79270b72c..56d3e14b81bf 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -3183,7 +3183,33 @@ static GTY(()) bitmap zero_view_p;
: (N) == 0)
 
 /* Return true iff we're to emit .loc directives for the assembler to
-   generate line number sections.  */
+   generate line number sections.
+
+   When we're not emitting views, all we need from the assembler is
+   support for .loc directives.
+
+   If we are emitting views, we can only use the assembler's .loc
+   support if it also supports views.
+
+   When the compiler is emitting the line number programs and
+   computing view numbers itself, it resets view numbers at known PC
+   changes and counts from that, and then it emits view numbers as
+   literal constants in locviewlists.  There are cases in which the
+   compiler is not sure about PC changes, e.g. when extra alignment is
+   requested for a label.  In these cases, the compiler may not reset
+   the view counter, and the potential PC advance in the line number
+   program will use an opcode that does not reset the view counter
+   even if the PC actually changes, so that compiler and debug info
+   consumer can keep view numbers in sync.
+
+   When the compiler defers view computation to the assembler, it
+   emits symbolic view numbers in locviewlists, with the exception of
+   views known to be zero (forced resets, or reset after
+   compiler-visible PC changes): instead of emitting symbols for
+   these, we emit literal zero and assert the assembler agrees with
+   the compiler's assessment.  We could use symbolic views everywhere,
+   instead of special-casing zero views, but then we'd be unable to
+   optimize out locviewlists that contain only zeros.  */
 
 static bool
 output_asm_line_debug_info (void)


-- 
Alexandre Oliva, freedom fighterhttp://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

Re: [SFN+LVU+IEPM v4 7/9] [LVU] Introduce location views

[Jason Merrill]

On 02/06/2018 11:02 PM, Alexandre Oliva wrote:

On Feb  6, 2018, Jason Merrill  wrote:

On 12/11/2017 09:52 PM, Alexandre Oliva wrote:



Why do we need to use a non-zero view identifier for a zero view?  Why
can't we always use 0 instead of the bitmap?


We assign view ids before we can determine whether the assigned view id
will be a zero view.  That's because we scan insns forward, and debug
binds take effect at the next .loc directive, which might be hundreds of
insns after the first reference (lots of intervening debug binds before
the insn that will take the next view), and insns that would cause the > .loc 
directive to be at a different address from that of the previous
.loc might be anywhere between those two loc-generating insns, before or
after the bind.  So, by the time we have to assign an id to the view, we
don't know whether we'll find an insn that sets RESETTING_VIEW_P before
we reach the loc-emitting insn that will take that view id.

It made more sense to me to assign the ids uniformly, and then mark
those that we find to be zero when we reach them, than to scan forward
(potentially O(n^2)) to tell in advance.  This also reduces differences
in view id tracking between gcc-internal and asm view assignment.


Makes sense, thanks.


DW_LNS_fixed_advance_pc is the only opcode that may change the
address without resetting the view.  It is available for compilers
to use when an address change is uncertain, e.g., when advancing
past opcodes that may expand to an empty sequence,
e.g. possibly-empty alignment sequences, optional no-operation
opcodes or the like.



+  if (debug_variable_location_views && table->view)
+   push_dw_line_info_entry (table, LI_adv_address, label_num);



It looks like you'll always use DW_LNS_fixed_advance_pc when we don't
have .loc support.  Does that mean that the view never gets reset?


No, table->view will be zero if we have crossed an insn that
RESET_NEXT_VIEW, and then we'll use a LI_set_address.


I'm uncomfortable with the special handling of this opcode; it isn't
special in DWARF5 except as a fallback if more compact encodings
aren't usable.  Currently GCC is even more conservative than this, and
always use a relocation (DW_LNS_set_address); if we can use D_L_f_a_p
instead, I would expect that to help with link times.  It seems wrong
to use it only in the context of view support.


We didn't use it before, but if we use, that's ok.  We don't *have* to
reset the view number to zero at a PC change.  It would be all right to
never reset it, as long as the compiler and the debug info consumer
agrees about it.  Since in some cases the compiler has to assign views
itself (when the assembler doesn't support views), but it can't tell
whether there will be a PC change (e.g. at an align), we need some
mechanism that predictably refrains from resetting the view number,
otherwise the compiler might pick a view number based on a possibly
incorrect assumption about whether the align changes PC or not, and then
it might not match what the consumer, that knows whether the PC
advanced, would compute.  To make it concrete:

# ...
.L352:
# .loc 1 533 view 3 (internal compiler tracking, no assembler support)
mov r12 <- whatever
# DEBUG x => r12
.balign 32
.L353:
# .loc 1 480 view 

Consider you're the compiler and you're generating a view-augmented
loclist for variable x.  You have to indicate the view number at .L353
for the range that starts there, and possibly for the range that ends
there.

But is the view number 4 or 0?  Namely, does .balign advance PC or not?
The compiler can't possibly know.  So if it guesses .balign does advance
PC and assign a view number zero at .L353, but it guesses wrong, that
will be indistinguishable from view number zero at say .L350, because
the PC is the same.  The debug info consumer will see no PC change and
assign view number 4 to the line number table entry correspoding to the
.loc directive with view , but it won't find any loclist referencing
that view number.  Conversely, if the compiler guessed .balign did not
advance PC, it would assign view number 4 to .L353, but then, with your
suggestion, the debug info consumer would instead assign it view number
zero, and we'd be out of sync.

That's why we need an opcode that enables the compiler and the debug
info consumer to remain in sync, disregarding a potential PC change that
would cause a view reset, because the compiler can't predict whether or
not there will be an actual PC change.


Would it make sense to say for *all* opcodes that the view is reset if
and only if the address actually changes?


I don't see how to keep compiler and consumer in sync with this
arrangement.  That's why I introduced the one exceptional opcode.


OK, that makes sense.  But I'm still uncomfortable with choosing an 
existing opcode for that purpose, which previously would have been 
chosen just for reasons of encoding complexity and size.



How are we coordinating the line number 

Re: [SFN+LVU+IEPM v4 7/9] [LVU] Introduce location views

[Alexandre Oliva]

On Jan 25, 2018, Alexandre Oliva  wrote:

> On Jan 24, 2018, Jakub Jelinek  wrote:
>> On Tue, Dec 12, 2017 at 12:52:18AM -0200, Alexandre Oliva wrote:
>>> +DW_LLE_GNU_view_pair = 0x09,
>>> +#define DW_LLE_view_pair DW_LLE_GNU_view_pair

>> This looks wrong.  The proposal has not been accepted yet, so you
>> really can't know if DW_LLE_view_pair will be like that or whether it
>> will have value of 9.  Unfortunately, the DWARF standard doesn't specify a
>> vendor range for DW_LLE_* values.  I'd use 0xf0 or so, and don't pretend
>> there is DW_LLE_view_pair at all, just use DW_LLE_GNU_view_pair everywhere.
>> Jason, what do you think?

> My reasoning was that, since we'd only emit this as an
> explicitly-requested backward-incompatible extension to DWARF-5 (by
> specifying -gdwarf-6 in this patch, but the option spelling could be
> changed), any LLE number whatsoever would do.  The point of the #define
> was to write the code in GCC so that, once DW_LLE_view_pair was
> standardized (if it ever was), we'd just set up an enum for it and we'd
> be done, but that would only happen at DWARF6+ anyway, so we'd be able
> to tell, since we'd then have actual DWARF6, rather than DWARF5 with an
> incompatible extensions (which is what we emit with the current
> -gdwarf-6 option; see below).

Also...  I have implemented DW_LLE_view_pair support in binutils's debug
info dumping code, based on the proposed extension, and that has already
been released, so changing the number in GCC would make it incompatible
with the already-released binutils.

You may notice I've renamed the GCC option to emit this extension in the
latest version of the patch.

-- 
Alexandre Oliva, freedom fighterhttp://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

Re: [SFN+LVU+IEPM v4 7/9] [LVU] Introduce location views

[Alexandre Oliva]

Here's the updated version of the LVU patch, integrating changes
requested or proposed by yourself and by Jakub.


for  include/ChangeLog

* dwarf2.def (DW_AT_GNU_locviews): New.
* dwarf2.h (enum dwarf_location_list_entry_type): Add
DW_LLE_GNU_view_pair.
(DW_LLE_view_pair): Define.

for  gcc/ChangeLog

* common.opt (gvariable-location-views): New.
(gvariable-location-views=incompat5): New.
* config.in: Rebuilt.
* configure: Rebuilt.
* configure.ac: Test assembler for view support.
* dwarf2asm.c (dw2_asm_output_symname_uleb128): New.
* dwarf2asm.h (dw2_asm_output_symname_uleb128): Declare.
* dwarf2out.c (var_loc_view): New typedef.
(struct dw_loc_list_struct): Add vl_symbol, vbegin, vend.
(dwarf2out_locviews_in_attribute): New.
(dwarf2out_locviews_in_loclist): New.
(dw_val_equal_p): Compare val_view_list of dw_val_class_view_lists.
(enum dw_line_info_opcode): Add LI_adv_address.
(struct dw_line_info_table): Add view.
(RESET_NEXT_VIEW, RESETTING_VIEW_P): New macros.
(DWARF2_ASM_VIEW_DEBUG_INFO): Define default.
(zero_view_p): New variable.
(ZERO_VIEW_P): New macro.
(output_asm_line_debug_info): New.
(struct var_loc_node): Add view.
(add_AT_view_list, AT_loc_list): New.
(add_var_loc_to_decl): Add view param.  Test it against last.
(new_loc_list): Add view params.  Record them.
(AT_loc_list_ptr): Handle loc and view lists.
(view_list_to_loc_list_val_node): New.
(print_dw_val): Handle dw_val_class_view_list.
(size_of_die): Likewise.
(value_format): Likewise.
(loc_list_has_views): New.
(gen_llsym): Set vl_symbol too.
(maybe_gen_llsym, skip_loc_list_entry): New.
(dwarf2out_maybe_output_loclist_view_pair): New.
(output_loc_list): Output view list or entries too.
(output_view_list_offset): New.
(output_die): Handle dw_val_class_view_list.
(output_dwarf_version): New.
(output_compilation_unit_header): Use it.
(output_skeleton_debug_sections): Likewise.
(output_rnglists, output_line_info): Likewise.
(output_pubnames, output_aranges): Update version comments.
(output_one_line_info_table): Output view numbers in asm comments.
(dw_loc_list): Determine current endview, pass it to new_loc_list.
Call maybe_gen_llsym.
(loc_list_from_tree_1): Adjust.
(add_AT_location_description): Create view list attribute if
needed, check it's absent otherwise.
(convert_cfa_to_fb_loc_list): Adjust.
(maybe_emit_file): Call output_asm_line_debug_info for test.
(dwarf2out_var_location): Reset views as needed.  Precompute
add_var_loc_to_decl args.  Call get_attr_min_length only if we have the
attribute.  Set view.
(new_line_info_table): Reset next view.
(set_cur_line_info_table): Call output_asm_line_debug_info for test.
(dwarf2out_source_line): Likewise.  Output view resets and labels to
the assembler, or select appropriate line info opcodes.
(prune_unused_types_walk_attribs): Handle dw_val_class_view_list.
(optimize_string_length): Catch it.  Adjust.
(resolve_addr): Copy vl_symbol along with ll_symbol.  Handle
dw_val_class_view_list, and remove it if no longer needed.
(hash_loc_list): Hash view numbers.
(loc_list_hasher::equal): Compare them.
(optimize_location_lists): Check whether a view list symbol is
needed, and whether the locview attribute is present, and
whether they match.  Remove the locview attribute if no longer
needed.
(index_location_lists): Call skip_loc_list_entry for test.
(dwarf2out_finish): Call output_asm_line_debug_info for test.
Use output_dwarf_version.
* dwarf2out.h (enum dw_val_class): Add dw_val_class_view_list.
(struct dw_val_node): Add val_view_list.
* final.c (SEEN_NEXT_VIEW): New.
(set_next_view_needed): New.
(clear_next_view_needed): New.
(maybe_output_next_view): New.
(final_start_function): Rename to...
(final_start_function_1): ... this.  Take pointer to FIRST,
add SEEN parameter.  Emit param bindings in the initial view.
(final_start_function): Reintroduce SEEN-less interface.
(final): Rename to...
(final_1): ... this.  Take SEEN parameter.  Output final pending
next view at the end.
(final): Reintroduce seen-less interface.
(final_scan_insn): Output pending next view before switching
sections or ending a block.  Mark the next view as needed when
outputting variable locations.  Notify debug backend of section
changes, and of location view changes.
(rest_of_handle_final): Adjust.
* 

Re: [SFN+LVU+IEPM v4 7/9] [LVU] Introduce location views

[Alexandre Oliva]

Here's an incremental patch with the changes I made in response to your
requests.  I'll post the complete updated patch momentarily.

diff --git a/gcc/common.opt b/gcc/common.opt
index 55d9cdd714ff..7e024fdab124 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -2957,9 +2957,12 @@ Common Driver Report Var(flag_gtoggle)
 Toggle debug information generation.
 
 gvariable-location-views
-Common Driver Var(debug_variable_location_views) Init(2)
+Common Driver Var(debug_variable_location_views, 1) Init(2)
 Augment variable location lists with progressive views.
 
+gvariable-location-views=incompat5
+Common Driver RejectNegative Var(debug_variable_location_views, -1) Init(2)
+
 gvms
 Common Driver JoinedOrMissing Negative(gxcoff)
 Generate debug information in VMS format.
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 7f09fcb64968..045aab78ce51 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -7220,8 +7220,10 @@ compiling with optimization (@option{-Os}, @option{-O}, 
@option{-O2},
 @dots{}), and outputting DWARF 2 debug information at the normal level.
 
 @item -gvariable-location-views
+@item -gvariable-location-views=incompat5
 @item -gno-variable-location-views
 @opindex gvariable-location-views
+@opindex gvariable-location-views=incompat5
 @opindex gno-variable-location-views
 Augment variable location lists with progressive view numbers implied
 from the line number table.  This enables debug information consumers to
@@ -7234,8 +7236,16 @@ number tables and location lists are fully 
backward-compatible, so they
 can be consumed by debug information consumers that are not aware of
 these augmentations, but they won't derive any benefit from them either.
 This is enabled by default when outputting DWARF 2 debug information at
-the normal level, as long as @code{-fvar-tracking-assignments} is
-enabled and @code{-gstrict-dwarf} is not.
+the normal level, as long as @option{-fvar-tracking-assignments} is
+enabled and @option{-gstrict-dwarf} is not.
+
+There is a proposed representation for view numbers that is not backward
+compatible with the location list format introduced in DWARF 5, that can
+be enabled with @option{-gvariable-location-views=incompat5}.  This
+option may be removed in the future, is only provided as a reference
+implementation of the proposed representation.  Debug information
+consumers are not expected to support this extended format, and they
+would be rendered unable to decode location lists using it.
 
 @item -gz@r{[}=@var{type}@r{]}
 @opindex gz
diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index 9ef7fa3516d4..3cf79270b72c 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -1350,7 +1350,7 @@ dwarf_stack_op_name (unsigned int op)
 static inline bool
 dwarf2out_locviews_in_attribute ()
 {
-  return debug_variable_location_views && dwarf_version <= 5;
+  return debug_variable_location_views == 1;
 }
 
 /* Return TRUE iff we're to output location view lists as part of the
@@ -1362,7 +1362,7 @@ dwarf2out_locviews_in_loclist ()
 #ifndef DW_LLE_view_pair
   return false;
 #else
-  return debug_variable_location_views && dwarf_version >= 6;
+  return debug_variable_location_views == -1;
 #endif
 }
 
@@ -3164,7 +3164,7 @@ skeleton_chain_node;
 #endif
 
 /* A bit is set in ZERO_VIEW_P if we are using the assembler-supported
-   view computation, and it is refers to a view identifier for which
+   view computation, and it refers to a view identifier for which we
will not emit a label because it is known to map to a view number
zero.  We won't allocate the bitmap if we're not using assembler
support for location views, but we have to make the variable
@@ -27344,23 +27344,40 @@ dwarf2out_source_line (unsigned int line, unsigned 
int column,
  zero_view_p = BITMAP_GGC_ALLOC ();
  bitmap_set_bit (zero_view_p, 0);
}
- if (RESETTING_VIEW_P (table->view))
+ if (!RESETTING_VIEW_P (table->view))
+   {
+ /* When we're using the assembler to compute view
+numbers, we output symbolic labels after "view" in
+.loc directives, and the assembler will set them for
+us, so that we can refer to the view numbers in
+location lists.  The only exceptions are when we know
+a view will be zero: "-0" is a forced reset, used
+e.g. in the beginning of functions, whereas "0" tells
+the assembler to check that there was a PC change
+since the previous view, in a way that implicitly
+resets the next view.  */
+ fputs (" view ", asm_out_file);
+ char label[MAX_ARTIFICIAL_LABEL_BYTES];
+ ASM_GENERATE_INTERNAL_LABEL (label, "LVU", table->view);
+ assemble_name (asm_out_file, label);
+ table->view = ++lvugid;
+   }
+ else
{
  if (!table->in_use)
fputs (" 

Re: [SFN+LVU+IEPM v4 7/9] [LVU] Introduce location views

[Alexandre Oliva]

On Jan 30, 2018, Richard Sandiford  wrote:

>> But it is my understanding that both of the following are correct:
>> 
>> return (verylongcondition
>> && otherlongcondition__);
>> 
>> return verylongcondition
>>   && otherlongcondition__;
>> 
>> The first, because the parenthesized expression is continued with
>> indentation to match the parenthesis, the second because the return
>> statement is continued with the correct indentation for the continuation
>> of a statement.

> Thought it had to be the first.  When they talk about indenting leading
> operators, the conventions say:

>   Insert extra parentheses so that Emacs will indent the code properly.

> which at least implies that not inserting parantheses and indenting by
> two spaces isn't "properly".

Hmm, in my reading, the "properly" there was to contrast with the IMHO
improper, excessive manual indentation in the example that follows it:

  v = rup->ru_utime.tv_sec*1000 + rup->ru_utime.tv_usec/1000
  + rup->ru_stime.tv_sec*1000 + rup->ru_stime.tv_usec/1000;

but I think it's just as legitimate to instead understand that the above
indentation is proper, and that the only problem with it is that
mechanical reindent would turn it into

  v = rup->ru_utime.tv_sec*1000 + rup->ru_utime.tv_usec/1000
+ rup->ru_stime.tv_sec*1000 + rup->ru_stime.tv_usec/1000;

which, in *this* case, would be wrong, because you'd have '=' and '+'
operators, of different precedence, at the same level of indentation.

But in the case of return, there's no such issue with operator
precedence.

I'll raise this issue at bug-standa...@gnu.org and request
clarification.

> And I think most GCC code does stick to that (i.e. use the bracketed form).

Let's see what GNU decides, then we can decide whether to add a
GCC-specific constraint, if GNU doesn't impose it already.

Thanks!

-- 
Alexandre Oliva, freedom fighterhttp://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

Re: [SFN+LVU+IEPM v4 7/9] [LVU] Introduce location views

[Alexandre Oliva]

On Feb  6, 2018, Jason Merrill  wrote:

> On 12/11/2017 09:52 PM, Alexandre Oliva wrote:
>> +/* output symbol LAB1 as an unsigned LEB128 quantity.  */
>> +
>> +void
>> +dw2_asm_output_symname_uleb128 (const char *lab1 ATTRIBUTE_UNUSED,
>> +const char *comment, ...)

> I'm having trouble understanding the use of symbols for views.  The
> configure test for gas support doesn't obviously define a symbol
> anywhere; is it implicitly defined by its use in the location
> directive? This could use more documentation in dwarf2out.

Yeah.  It's the .loc statement that assigns a view number to the view
number symbol.  That's documented in the gas manual, but I guess it
won't hurt to add something to dwarf2out too.  Will do.

>> +   view computation, and it is refers to a view identifier for which

> "it refers"

Thanks

> Why do we need to use a non-zero view identifier for a zero view?  Why
> can't we always use 0 instead of the bitmap?

We assign view ids before we can determine whether the assigned view id
will be a zero view.  That's because we scan insns forward, and debug
binds take effect at the next .loc directive, which might be hundreds of
insns after the first reference (lots of intervening debug binds before
the insn that will take the next view), and insns that would cause the
.loc directive to be at a different address from that of the previous
.loc might be anywhere between those two loc-generating insns, before or
after the bind.  So, by the time we have to assign an id to the view, we
don't know whether we'll find an insn that sets RESETTING_VIEW_P before
we reach the loc-emitting insn that will take that view id.

It made more sense to me to assign the ids uniformly, and then mark
those that we find to be zero when we reach them, than to scan forward
(potentially O(n^2)) to tell in advance.  This also reduces differences
in view id tracking between gcc-internal and asm view assignment.


>> DW_LNS_fixed_advance_pc is the only opcode that may change the
>> address without resetting the view.  It is available for compilers
>> to use when an address change is uncertain, e.g., when advancing
>> past opcodes that may expand to an empty sequence,
>> e.g. possibly-empty alignment sequences, optional no-operation
>> opcodes or the like.

>> +  if (debug_variable_location_views && table->view)
>> +push_dw_line_info_entry (table, LI_adv_address, label_num);

> It looks like you'll always use DW_LNS_fixed_advance_pc when we don't
> have .loc support.  Does that mean that the view never gets reset?

No, table->view will be zero if we have crossed an insn that
RESET_NEXT_VIEW, and then we'll use a LI_set_address.

> I'm uncomfortable with the special handling of this opcode; it isn't
> special in DWARF5 except as a fallback if more compact encodings
> aren't usable.  Currently GCC is even more conservative than this, and
> always use a relocation (DW_LNS_set_address); if we can use D_L_f_a_p
> instead, I would expect that to help with link times.  It seems wrong
> to use it only in the context of view support.

We didn't use it before, but if we use, that's ok.  We don't *have* to
reset the view number to zero at a PC change.  It would be all right to
never reset it, as long as the compiler and the debug info consumer
agrees about it.  Since in some cases the compiler has to assign views
itself (when the assembler doesn't support views), but it can't tell
whether there will be a PC change (e.g. at an align), we need some
mechanism that predictably refrains from resetting the view number,
otherwise the compiler might pick a view number based on a possibly
incorrect assumption about whether the align changes PC or not, and then
it might not match what the consumer, that knows whether the PC
advanced, would compute.  To make it concrete:

# ...
.L352:
# .loc 1 533 view 3 (internal compiler tracking, no assembler support)
mov r12 <- whatever
# DEBUG x => r12
.balign 32
.L353:
# .loc 1 480 view 

Consider you're the compiler and you're generating a view-augmented
loclist for variable x.  You have to indicate the view number at .L353
for the range that starts there, and possibly for the range that ends
there.

But is the view number 4 or 0?  Namely, does .balign advance PC or not?
The compiler can't possibly know.  So if it guesses .balign does advance
PC and assign a view number zero at .L353, but it guesses wrong, that
will be indistinguishable from view number zero at say .L350, because
the PC is the same.  The debug info consumer will see no PC change and
assign view number 4 to the line number table entry correspoding to the
.loc directive with view , but it won't find any loclist referencing
that view number.  Conversely, if the compiler guessed .balign did not
advance PC, it would assign view number 4 to .L353, but then, with your
suggestion, the debug info consumer would instead assign it view number
zero, and we'd be out of sync.

That's why we need an 

Re: [SFN+LVU+IEPM v4 7/9] [LVU] Introduce location views

[Jason Merrill]

On 12/11/2017 09:52 PM, Alexandre Oliva wrote:

+/* output symbol LAB1 as an unsigned LEB128 quantity.  */
+
+void
+dw2_asm_output_symname_uleb128 (const char *lab1 ATTRIBUTE_UNUSED,
+   const char *comment, ...)


I'm having trouble understanding the use of symbols for views.  The 
configure test for gas support doesn't obviously define a symbol 
anywhere; is it implicitly defined by its use in the location directive? 
 This could use more documentation in dwarf2out.



+   view computation, and it is refers to a view identifier for which


"it refers"

Why do we need to use a non-zero view identifier for a zero view?  Why 
can't we always use 0 instead of the bitmap?



  Rationale: location lists can refer to address and view, not
  op_index, so views are reset at address changes, not at op_index
  changes.  Opcodes that advance op_index only will only reset the
  view when they happen to advance the address, e.g. by exceeding
  maximum_operations_per_instruction in op_index.

  DW_LNS_fixed_advance_pc is the only opcode that may change the
  address without resetting the view.  It is available for compilers
  to use when an address change is uncertain, e.g., when advancing
  past opcodes that may expand to an empty sequence,
  e.g. possibly-empty alignment sequences, optional no-operation
  opcodes or the like.



+  if (debug_variable_location_views && table->view)
+   push_dw_line_info_entry (table, LI_adv_address, label_num);


It looks like you'll always use DW_LNS_fixed_advance_pc when we don't 
have .loc support.  Does that mean that the view never gets reset?


I'm uncomfortable with the special handling of this opcode; it isn't 
special in DWARF5 except as a fallback if more compact encodings aren't 
usable.  Currently GCC is even more conservative than this, and always 
use a relocation (DW_LNS_set_address); if we can use D_L_f_a_p instead, 
I would expect that to help with link times.  It seems wrong to use it 
only in the context of view support.


Would it make sense to say for *all* opcodes that the view is reset if 
and only if the address actually changes?


How are we coordinating the line number table and location list versions 
of the view counter?


Jason

Re: [SFN+LVU+IEPM v4 7/9] [LVU] Introduce location views

[Richard Sandiford]

Alexandre Oliva  writes:
> On Jan 26, 2018, Jakub Jelinek  wrote:
>
>> Having to tweak debug info consumers so that they treat DW_LLE_* of 9
>> one way for .debug_loclist of version 5 and another way for .debug_loclist
>> of version 6 isn't a good idea.
>
> Maybe we don't have to do that.  The reason I implemented the proposed
> format was to have a reference implementation, but since it's an
> extension to a non-extensible part of DWARF5, it's hard to justify
> consumer implementations for that.
>
>> Why don't you emit the DW_LLE_GNU_view_pair stuff in .debug_loclists
>> already with -gdwarf-5, if needed paired without some TU attribute that says
>> that views are emitted?
>
> Because that would make the loclists unreadable for any DWARF5-compliant
> consumer.
>
>
>> Haven't looked for it it in the coding standards, but it is something
>> I've been asked to do in my code by various reviewers over the years and
>> what I've been asking others in my patch reviews from others too.
>
> Thanks.  It was the first (well, second; you'd requested similar changes
> another time before) I heard of that.  I'm still unconvinced the way I
> used is not compliant, but I'll keep that in mind.  Hopefully I won't
> run into someone who insists the other one (the one you reject) is the
> only correct one ;-)
>
>> I feel strongly about indenting stuff right,
>
> I'm with you on that, we just have different ideas about what "right"
> stands for ;-)
>
>> which if it wouldn't fit would be
>>   return verylongcondition
>> && otherlongcondition__;
>> rather than
>>   return verylongcondition
>>   && otherlongcondition__;
>> or similar, it would surprise me if it is not in the coding standard.
>
> I agree neither of these two forms is correct.
>
> But it is my understanding that both of the following are correct:
>
>   return (verylongcondition
>   && otherlongcondition__);
>
>   return verylongcondition
> && otherlongcondition__;
>
> The first, because the parenthesized expression is continued with
> indentation to match the parenthesis, the second because the return
> statement is continued with the correct indentation for the continuation
> of a statement.

Thought it had to be the first.  When they talk about indenting leading
operators, the conventions say:

  Insert extra parentheses so that Emacs will indent the code properly.

which at least implies that not inserting parantheses and indenting by
two spaces isn't "properly".

And I think most GCC code does stick to that (i.e. use the bracketed form).

Thanks,
Richard

Re: [SFN+LVU+IEPM v4 7/9] [LVU] Introduce location views

[Alexandre Oliva]

On Jan 26, 2018, Jakub Jelinek  wrote:

> Having to tweak debug info consumers so that they treat DW_LLE_* of 9
> one way for .debug_loclist of version 5 and another way for .debug_loclist
> of version 6 isn't a good idea.

Maybe we don't have to do that.  The reason I implemented the proposed
format was to have a reference implementation, but since it's an
extension to a non-extensible part of DWARF5, it's hard to justify
consumer implementations for that.

> Why don't you emit the DW_LLE_GNU_view_pair stuff in .debug_loclists
> already with -gdwarf-5, if needed paired without some TU attribute that says
> that views are emitted?

Because that would make the loclists unreadable for any DWARF5-compliant
consumer.


> Haven't looked for it it in the coding standards, but it is something
> I've been asked to do in my code by various reviewers over the years and
> what I've been asking others in my patch reviews from others too.

Thanks.  It was the first (well, second; you'd requested similar changes
another time before) I heard of that.  I'm still unconvinced the way I
used is not compliant, but I'll keep that in mind.  Hopefully I won't
run into someone who insists the other one (the one you reject) is the
only correct one ;-)

> I feel strongly about indenting stuff right,

I'm with you on that, we just have different ideas about what "right"
stands for ;-)

> which if it wouldn't fit would be
>   return verylongcondition
>  && otherlongcondition__;
> rather than
>   return verylongcondition
>   && otherlongcondition__;
> or similar, it would surprise me if it is not in the coding standard.

I agree neither of these two forms is correct.

But it is my understanding that both of the following are correct:

  return (verylongcondition
  && otherlongcondition__);

  return verylongcondition
&& otherlongcondition__;

The first, because the parenthesized expression is continued with
indentation to match the parenthesis, the second because the return
statement is continued with the correct indentation for the continuation
of a statement.

>> Personally, I don't see that line breaks before operators are only
>> permitted when the operand that follows wouldn't fit; the standards are

> Yes, you can break it, but why, it doesn't make the code more readable,
> but less in this case.

I thought it made it more readable, especially in the context of the
patch.  I guess this means we'll have to agree to disagree.

> So, what is the reason not to emit the format you are proposing already with
> -gdwarf-5, perhaps with some extra attribute on the TU (of course not when
> -gstrict-dwarf)?

See above.  We don't want to create unreadable loclists for
standard-compliant consumers, do we?

-- 
Alexandre Oliva, freedom fighterhttp://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

Re: [SFN+LVU+IEPM v4 7/9] [LVU] Introduce location views

[Jakub Jelinek]

On Thu, Jan 25, 2018 at 05:58:37PM -0200, Alexandre Oliva wrote:
> > This looks wrong.  The proposal has not been accepted yet, so you
> > really can't know if DW_LLE_view_pair will be like that or whether it
> > will have value of 9.  Unfortunately, the DWARF standard doesn't specify a
> > vendor range for DW_LLE_* values.  I'd use 0xf0 or so, and don't pretend
> > there is DW_LLE_view_pair at all, just use DW_LLE_GNU_view_pair everywhere.
> > Jason, what do you think?
> 
> My reasoning was that, since we'd only emit this as an
> explicitly-requested backward-incompatible extension to DWARF-5 (by
> specifying -gdwarf-6 in this patch, but the option spelling could be
> changed), any LLE number whatsoever would do.  The point of the #define
> was to write the code in GCC so that, once DW_LLE_view_pair was
> standardized (if it ever was), we'd just set up an enum for it and we'd
> be done, but that would only happen at DWARF6+ anyway, so we'd be able
> to tell, since we'd then have actual DWARF6, rather than DWARF5 with an
> incompatible extensions (which is what we emit with the current
> -gdwarf-6 option; see below).

Having to tweak debug info consumers so that they treat DW_LLE_* of 9
one way for .debug_loclist of version 5 and another way for .debug_loclist
of version 6 isn't a good idea.
Why don't you emit the DW_LLE_GNU_view_pair stuff in .debug_loclists
already with -gdwarf-5, if needed paired without some TU attribute that says
that views are emitted?

> >> +static inline bool
> >> +dwarf2out_locviews_in_attribute ()
> >> +{
> >> +  return debug_variable_location_views
> >> +&& dwarf_version <= 5;
> 
> > Formatting, should be
> >   return debug_variable_location_views && dwarf_version <= 5;
> > or
> >   return (debug_variable_location_views
> >   && dwarf_version <= 5);
> > if it wouldn't fit (but it does).
> 
> Hmm...  Where does that mandate come from?, if you don't mind my asking.
> I have just revised both GNU Coding Standards, and GCC-specific
> conventions, to make sure I hadn't missed a change or some long-ago
> established convention, and I couldn't find anything that supports that
> "should".

Haven't looked for it it in the coding standards, but it is something
I've been asked to do in my code by various reviewers over the years and
what I've been asking others in my patch reviews from others too.

I'm personally not using emacs and so the extra ()s isn't something I'd
want myself, it is just something others asked for multiple times.
I feel strongly about indenting stuff right, which if it wouldn't fit would
be
  return verylongcondition
 && otherlongcondition__;
rather than
  return verylongcondition
  && otherlongcondition__;
or similar, it would surprise me if it is not in the coding standard.

> Personally, I don't see that line breaks before operators are only
> permitted when the operand that follows wouldn't fit; the standards are

Yes, you can break it, but why, it doesn't make the code more readable,
but less in this case.

> No, that means something else, namely emit location view lists in a
> separate attribute, matching corresponding ranges.
> 
> Maybe we shouldn't even have an option to emit that at this point, or
> make it a very different spelling.  But I'd argue there's no point in
> discarding the code that implements the format that was proposed for
> standardization; at the very least it serves as a reference.  That's
> also an argument for retaining the ability to emit it somehow, even if
> with an option that we document as to-be-dropped if/when there's a
> standard representation.

So, what is the reason not to emit the format you are proposing already with
-gdwarf-5, perhaps with some extra attribute on the TU (of course not when
-gstrict-dwarf)?
Debuggers are still barely catching up with DWARF5, so even if they would be
upset by the DW_LLE_GNU_view_pair stuff, they can be still tweaked to ignore
those (skip over them) if they don't want to deal with the views.

Jakub

Re: [SFN+LVU+IEPM v4 7/9] [LVU] Introduce location views

[Alexandre Oliva]

On Jan 24, 2018, Jakub Jelinek  wrote:

> On Tue, Dec 12, 2017 at 12:52:18AM -0200, Alexandre Oliva wrote:
>> --- a/include/dwarf2.h
>> +++ b/include/dwarf2.h
>> @@ -298,6 +298,14 @@ enum dwarf_location_list_entry_type
>> DW_LLE_start_end = 0x07,
>> DW_LLE_start_length = 0x08,
>> 
>> + /*
>> 
>> +   has the proposal for now; only available to list members.
>> +
>> +   A (possibly updated) copy of the proposal is available at
>> +   .  */
>> +DW_LLE_GNU_view_pair = 0x09,
>> +#define DW_LLE_view_pair DW_LLE_GNU_view_pair
>> +

> This looks wrong.  The proposal has not been accepted yet, so you
> really can't know if DW_LLE_view_pair will be like that or whether it
> will have value of 9.  Unfortunately, the DWARF standard doesn't specify a
> vendor range for DW_LLE_* values.  I'd use 0xf0 or so, and don't pretend
> there is DW_LLE_view_pair at all, just use DW_LLE_GNU_view_pair everywhere.
> Jason, what do you think?

My reasoning was that, since we'd only emit this as an
explicitly-requested backward-incompatible extension to DWARF-5 (by
specifying -gdwarf-6 in this patch, but the option spelling could be
changed), any LLE number whatsoever would do.  The point of the #define
was to write the code in GCC so that, once DW_LLE_view_pair was
standardized (if it ever was), we'd just set up an enum for it and we'd
be done, but that would only happen at DWARF6+ anyway, so we'd be able
to tell, since we'd then have actual DWARF6, rather than DWARF5 with an
incompatible extensions (which is what we emit with the current
-gdwarf-6 option; see below).


>> --- a/gcc/dwarf2asm.c
>> +++ b/gcc/dwarf2asm.c
>> @@ -767,6 +767,33 @@ dw2_asm_output_data_sleb128 (HOST_WIDE_INT value,
>> va_end (ap);
>> }
>> 
>> +/* output symbol LAB1 as an unsigned LEB128 quantity.  */

> Capital O in Output please.

Thanks, fixed.

>> +static inline bool
>> +dwarf2out_locviews_in_attribute ()
>> +{
>> +  return debug_variable_location_views
>> +&& dwarf_version <= 5;

> Formatting, should be
>   return debug_variable_location_views && dwarf_version <= 5;
> or
>   return (debug_variable_location_views
> && dwarf_version <= 5);
> if it wouldn't fit (but it does).

Hmm...  Where does that mandate come from?, if you don't mind my asking.
I have just revised both GNU Coding Standards, and GCC-specific
conventions, to make sure I hadn't missed a change or some long-ago
established convention, and I couldn't find anything that supports that
"should".

I don't mind adjusting code to match your preferences (if that's what it
is), especially in modules you maintain, but I'd like to make sure I
haven't missed some requirement in the coding standards.  Or maybe, if
it's more than a personal preference and rather a group preference, we
should add that at least to the GCC coding conventions, if most of us
agree with it.

Personally, I don't see that line breaks before operators are only
permitted when the operand that follows wouldn't fit; the standards are
more permissive than that.  And I don't see that the parentheses are
mandatory either; GNU recommends them so that one doesn't have to indent
the subsequent lines by hand (or risk automatically reintendation to
undo that manual work), but it doesn't seem to mandate the indentation
or the paretheses, and the indentation one gets in their absence is IIUC
permitted and occasionally even desirable.  Am I missing something?

Anyway, consider the requested changes done.

> Do we really need to introduce -gdwarf-6 at this point?
> -gdwarf-5 -gvariable-location-views should be sufficient, isn't it?

No, that means something else, namely emit location view lists in a
separate attribute, matching corresponding ranges.

Maybe we shouldn't even have an option to emit that at this point, or
make it a very different spelling.  But I'd argue there's no point in
discarding the code that implements the format that was proposed for
standardization; at the very least it serves as a reference.  That's
also an argument for retaining the ability to emit it somehow, even if
with an option that we document as to-be-dropped if/when there's a
standard representation.

-- 
Alexandre Oliva, freedom fighterhttp://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

Re: [SFN+LVU+IEPM v4 7/9] [LVU] Introduce location views

[Jakub Jelinek]

On Tue, Dec 12, 2017 at 12:52:18AM -0200, Alexandre Oliva wrote:
> --- a/include/dwarf2.h
> +++ b/include/dwarf2.h
> @@ -298,6 +298,14 @@ enum dwarf_location_list_entry_type
>  DW_LLE_start_end = 0x07,
>  DW_LLE_start_length = 0x08,
>  
> +/* 
> 
> +   has the proposal for now; only available to list members.
> +
> +   A (possibly updated) copy of the proposal is available at
> +   .  */
> +DW_LLE_GNU_view_pair = 0x09,
> +#define DW_LLE_view_pair DW_LLE_GNU_view_pair
> +

This looks wrong.  The proposal has not been accepted yet, so you
really can't know if DW_LLE_view_pair will be like that or whether it
will have value of 9.  Unfortunately, the DWARF standard doesn't specify a
vendor range for DW_LLE_* values.  I'd use 0xf0 or so, and don't pretend
there is DW_LLE_view_pair at all, just use DW_LLE_GNU_view_pair everywhere.
Jason, what do you think?

> --- a/gcc/dwarf2asm.c
> +++ b/gcc/dwarf2asm.c
> @@ -767,6 +767,33 @@ dw2_asm_output_data_sleb128 (HOST_WIDE_INT value,
>va_end (ap);
>  }
>  
> +/* output symbol LAB1 as an unsigned LEB128 quantity.  */

Capital O in Output please.

> +static inline bool
> +dwarf2out_locviews_in_attribute ()
> +{
> +  return debug_variable_location_views
> +&& dwarf_version <= 5;

Formatting, should be
  return debug_variable_location_views && dwarf_version <= 5;
or
  return (debug_variable_location_views
  && dwarf_version <= 5);
if it wouldn't fit (but it does).

> +static inline bool
> +dwarf2out_locviews_in_loclist ()
> +{
> +#ifndef DW_LLE_view_pair
> +  return false;
> +#else
> +  return debug_variable_location_views
> +&& dwarf_version >= 6;

Likewise.

> +
> +static bool
> +output_asm_line_debug_info (void)
> +{
> +  return DWARF2_ASM_VIEW_DEBUG_INFO
> +|| (DWARF2_ASM_LINE_DEBUG_INFO && !debug_variable_location_views);

Likewise.

> +  dw2_asm_output_data (1, DW_LLE_view_pair,
> +"DW_LLE_view_pair");

This also fits on a single line.

> +/* Output the dwarf version number.  */
> +
> +static void
> +output_dwarf_version ()
> +{
> +  /* ??? For now, if -gdwarf-6 is specified, we output version 5 with
> + views in loclist.  That will change eventually.  */
> +  if (dwarf_version == 6)
> +{
> +  static bool once;
> +  if (!once)
> + {
> +   warning (0,
> +"-gdwarf-6 is output as version 5 with incompatibilities");
> +   once = true;
> + }
> +  dw2_asm_output_data (2, 5, "DWARF version number");
> +}

Do we really need to introduce -gdwarf-6 at this point?
-gdwarf-5 -gvariable-location-views should be sufficient, isn't it?
We don't know at all what will it look like in 3 or how many years.
My preference would be to keep all those dwarf_version == 6 related changes
out, including this output_dwarf_version function etc.

> +  const char *label = NOTE_DURING_CALL_P (loc_note)
> + ? last_postcall_label : last_label;

Again wrong formatting,
  const char *label
= NOTE_DURING_CALL_P (loc_note) ? last_postcall_label : last_label;
is better.

> +  return !DECL_IGNORED_P (current_function_decl)
> +&& debug_variable_location_views
> +&& insn && GET_CODE (insn) == NOTE
> +&& NOTE_KIND (insn) == NOTE_INSN_VAR_LOCATION;

Formatting.

Jakub

Re: [SFN+LVU+IEPM v4 7/9] [LVU] Introduce location views

[Alexandre Oliva]

On Dec 11, 2017, Jeff Law  wrote:

> On 11/09/2017 07:34 PM, Alexandre Oliva wrote:
>> This patch introduces an option to enable the generation of location
>> views along with location lists.  The exact format depends on the
>> void
>> +dw2_asm_output_symname_uleb128 (const char *lab1 ATTRIBUTE_UNUSED,
>> +const char *comment, ...)
> Needs a function comment.  Yes I realize others are missing in that
> file.  But I think it's best to avoid making the problem worse.

'k, added.

> The stuff outside dwarf2*.c is OK.  We really need Jason or Jakub to
> comment on the meat of the dwarf2 bits.

Here's an updated version of the patch, also including the comments and
other changes (*) that richi had requested.

(*) the earlier version of the patch added an #include that was had
already been rendered unnecessary; it is no longer added by this version.


[LVU] Introduce location views

This patch introduces an option to enable the generation of location
views along with location lists.  The exact format depends on the
DWARF version: it can be a separate attribute (DW_AT_GNU_locviews) or
(DW_LLE_view_pair) entries in DWARF5+ loclists.

Line number tables are also affected.  If the assembler is found, at
compiler build time, to support .loc views, we use them and
assembler-computed view labels, otherwise we output compiler-generated
line number programs with conservatively-computed view labels.  In
either case, we output view information next to line number changes
when verbose assembly output is requested.

This patch requires an LVU patch that modifies the exported API of
final_scan_insn.  It also expects the entire SFN patchset to be
installed first, although SFN is not a requirement for LVU.

for  include/ChangeLog

* dwarf2.def (DW_AT_GNU_locviews): New.
* dwarf2.h (enum dwarf_location_list_entry_type): Add
DW_LLE_GNU_view_pair.
(DW_LLE_view_pair): Define.

for  gcc/ChangeLog

* common.opt (gvariable-location-views): New.
* config.in: Rebuilt.
* configure: Rebuilt.
* configure.ac: Test assembler for view support.
* dwarf2asm.c (dw2_asm_output_symname_uleb128): New.
* dwarf2asm.h (dw2_asm_output_symname_uleb128): Declare.
* dwarf2out.c (var_loc_view): New typedef.
(struct dw_loc_list_struct): Add vl_symbol, vbegin, vend.
(dwarf2out_locviews_in_attribute): New.
(dwarf2out_locviews_in_loclist): New.
(dw_val_equal_p): Compare val_view_list of dw_val_class_view_lists.
(enum dw_line_info_opcode): Add LI_adv_address.
(struct dw_line_info_table): Add view.
(RESET_NEXT_VIEW, RESETTING_VIEW_P): New macros.
(DWARF2_ASM_VIEW_DEBUG_INFO): Define default.
(zero_view_p): New variable.
(ZERO_VIEW_P): New macro.
(output_asm_line_debug_info): New.
(struct var_loc_node): Add view.
(add_AT_view_list, AT_loc_list): New.
(add_var_loc_to_decl): Add view param.  Test it against last.
(new_loc_list): Add view params.  Record them.
(AT_loc_list_ptr): Handle loc and view lists.
(view_list_to_loc_list_val_node): New.
(print_dw_val): Handle dw_val_class_view_list.
(size_of_die): Likewise.
(value_format): Likewise.
(loc_list_has_views): New.
(gen_llsym): Set vl_symbol too.
(maybe_gen_llsym, skip_loc_list_entry): New.
(dwarf2out_maybe_output_loclist_view_pair): New.
(output_loc_list): Output view list or entries too.
(output_view_list_offset): New.
(output_die): Handle dw_val_class_view_list.
(output_dwarf_version): New.
(output_compilation_unit_header): Use it.
(output_skeleton_debug_sections): Likewise.
(output_rnglists, output_line_info): Likewise.
(output_pubnames, output_aranges): Update version comments.
(output_one_line_info_table): Output view numbers in asm comments.
(dw_loc_list): Determine current endview, pass it to new_loc_list.
Call maybe_gen_llsym.
(loc_list_from_tree_1): Adjust.
(add_AT_location_description): Create view list attribute if
needed, check it's absent otherwise.
(convert_cfa_to_fb_loc_list): Adjust.
(maybe_emit_file): Call output_asm_line_debug_info for test.
(dwarf2out_var_location): Reset views as needed.  Precompute
add_var_loc_to_decl args.  Call get_attr_min_length only if we have the
attribute.  Set view.
(new_line_info_table): Reset next view.
(set_cur_line_info_table): Call output_asm_line_debug_info for test.
(dwarf2out_source_line): Likewise.  Output view resets and labels to
the assembler, or select appropriate line info opcodes.
(prune_unused_types_walk_attribs): Handle dw_val_class_view_list.
(optimize_string_length): Catch it.  Adjust.
(resolve_addr): Copy vl_symbol along with 

Re: [SFN+LVU+IEPM v4 7/9] [LVU] Introduce location views

[Jeff Law]

On 11/09/2017 07:34 PM, Alexandre Oliva wrote:
> This patch introduces an option to enable the generation of location
> views along with location lists.  The exact format depends on the
> DWARF version: it can be a separate attribute (DW_AT_GNU_locviews) or
> (DW_LLE_view_pair) entries in DWARF5+ loclists.
> 
> Line number tables are also affected.  If the assembler is found, at
> compiler build time, to support .loc views, we use them and
> assembler-computed view labels, otherwise we output compiler-generated
> line number programs with conservatively-computed view labels.  In
> either case, we output view information next to line number changes
> when verbose assembly output is requested.
> 
> This patch requires an LVU patch that modifies the exported API of
> final_scan_insn.  It also expects the entire SFN patchset to be
> installed first, although SFN is not a requirement for LVU.
> 
> for  include/ChangeLog
> 
>   * dwarf2.def (DW_AT_GNU_locviews): New.
>   * dwarf2.h (enum dwarf_location_list_entry_type): Add
>   DW_LLE_GNU_view_pair.
>   (DW_LLE_view_pair): Define.
> 
> for  gcc/ChangeLog
> 
>   * common.opt (gvariable-location-views): New.
>   * config.in: Rebuilt.
>   * configure: Rebuilt.
>   * configure.ac: Test assembler for view support.
>   * dwarf2asm.c (dw2_asm_output_symname_uleb128): New.
>   * dwarf2asm.h (dw2_asm_output_symname_uleb128): Declare.
>   * dwarf2out.c (var_loc_view): New typedef.
>   (struct dw_loc_list_struct): Add vl_symbol, vbegin, vend.
>   (dwarf2out_locviews_in_attribute): New.
>   (dwarf2out_locviews_in_loclist): New.
>   (dw_val_equal_p): Compare val_view_list of dw_val_class_view_lists.
>   (enum dw_line_info_opcode): Add LI_adv_address.
>   (struct dw_line_info_table): Add view.
>   (RESET_NEXT_VIEW, RESETTING_VIEW_P): New macros.
>   (DWARF2_ASM_VIEW_DEBUG_INFO): Define default.
>   (zero_view_p): New variable.
>   (ZERO_VIEW_P): New macro.
>   (output_asm_line_debug_info): New.
>   (struct var_loc_node): Add view.
>   (add_AT_view_list, AT_loc_list): New.
>   (add_var_loc_to_decl): Add view param.  Test it against last.
>   (new_loc_list): Add view params.  Record them.
>   (AT_loc_list_ptr): Handle loc and view lists.
>   (view_list_to_loc_list_val_node): New.
>   (print_dw_val): Handle dw_val_class_view_list.
>   (size_of_die): Likewise.
>   (value_format): Likewise.
>   (loc_list_has_views): New.
>   (gen_llsym): Set vl_symbol too.
>   (maybe_gen_llsym, skip_loc_list_entry): New.
>   (dwarf2out_maybe_output_loclist_view_pair): New.
>   (output_loc_list): Output view list or entries too.
>   (output_view_list_offset): New.
>   (output_die): Handle dw_val_class_view_list.
>   (output_dwarf_version): New.
>   (output_compilation_unit_header): Use it.
>   (output_skeleton_debug_sections): Likewise.
>   (output_rnglists, output_line_info): Likewise.
>   (output_pubnames, output_aranges): Update version comments.
>   (output_one_line_info_table): Output view numbers in asm comments.
>   (dw_loc_list): Determine current endview, pass it to new_loc_list.
>   Call maybe_gen_llsym.
>   (loc_list_from_tree_1): Adjust.
>   (add_AT_location_description): Create view list attribute if
>   needed, check it's absent otherwise.
>   (convert_cfa_to_fb_loc_list): Adjust.
>   (maybe_emit_file): Call output_asm_line_debug_info for test.
>   (dwarf2out_var_location): Reset views as needed.  Precompute
>   add_var_loc_to_decl args.  Call get_attr_min_length only if we have the
>   attribute.  Set view.
>   (new_line_info_table): Reset next view.
>   (set_cur_line_info_table): Call output_asm_line_debug_info for test.
>   (dwarf2out_source_line): Likewise.  Output view resets and labels to
>   the assembler, or select appropriate line info opcodes.
>   (prune_unused_types_walk_attribs): Handle dw_val_class_view_list.
>   (optimize_string_length): Catch it.  Adjust.
>   (resolve_addr): Copy vl_symbol along with ll_symbol.  Handle
>   dw_val_class_view_list, and remove it if no longer needed.
>   (hash_loc_list): Hash view numbers.
>   (loc_list_hasher::equal): Compare them.
>   (optimize_location_lists): Check whether a view list symbol is
>   needed, and whether the locview attribute is present, and
>   whether they match.  Remove the locview attribute if no longer
>   needed.
>   (index_location_lists): Call skip_loc_list_entry for test.
>   (dwarf2out_finish): Call output_asm_line_debug_info for test.
>   Use output_dwarf_version.
>   * dwarf2out.h (enum dw_val_class): Add dw_val_class_view_list.
>   (struct dw_val_node): Add val_view_list.
>   * final.c: Include langhooks.h.
>   (SEEN_NEXT_VIEW): New.
>   (set_next_view_needed): New.
>   (clear_next_view_needed): New.
> 

Re: [SFN+LVU+IEPM v4 7/9] [LVU] Introduce location views

[Alexandre Oliva]

On Nov 13, 2017, Richard Biener  wrote:

> What does final.c need langhooks for?

Thanks for catching this.

At some point I had a check on whether there could be being stmt markers
emitted by the language, but that's long gone, in part because of LTO;
that's now computed dynamically, on a per-function basis, so the check
was modified to match, but the include added back then lingered.
Consider it gone (I'm dropping it in the SFN and SLI branches, so it
won't be in the next version of the patchset)

> I'd appreciate a DWARF person reviewing the dwarf bits, some new
> static fns seem to lack their toplevel comment.

Thanks for pointing this out.  I've added the following comments:


/* Return true iff we're to emit .loc directives for the assembler to
   generate line number sections.  */

>> +output_asm_line_debug_info (void)


/* Add a view list attribute to DIE.  It must have a DW_AT_location
   attribute, because the view list complements the location list.  */

>> +add_AT_view_list (dw_die_ref die, enum dwarf_attribute attr_kind)


/* Return a pointer to the location list referenced by the attribute.
   If the named attribute is a view list, look up the corresponding
   DW_AT_location attribute and return its location list.  */

>> AT_loc_list_ptr (dw_attr_node *a)


/* Return the location attribute value associated with a view list
   attribute value.  */

>> +view_list_to_loc_list_val_node (dw_val_node *val)


> The middle-end pieces are ok.

Thanks!


-- 
Alexandre Oliva, freedom fighterhttp://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

Re: [SFN+LVU+IEPM v4 7/9] [LVU] Introduce location views

[Richard Biener]

On Fri, Nov 10, 2017 at 3:34 AM, Alexandre Oliva  wrote:
> This patch introduces an option to enable the generation of location
> views along with location lists.  The exact format depends on the
> DWARF version: it can be a separate attribute (DW_AT_GNU_locviews) or
> (DW_LLE_view_pair) entries in DWARF5+ loclists.
>
> Line number tables are also affected.  If the assembler is found, at
> compiler build time, to support .loc views, we use them and
> assembler-computed view labels, otherwise we output compiler-generated
> line number programs with conservatively-computed view labels.  In
> either case, we output view information next to line number changes
> when verbose assembly output is requested.
>
> This patch requires an LVU patch that modifies the exported API of
> final_scan_insn.  It also expects the entire SFN patchset to be
> installed first, although SFN is not a requirement for LVU.

What does final.c need langhooks for?  Asking because needing langhooks
at that point in time is bad (LTO, etc.).  I'd appreciate a DWARF
person reviewing
the dwarf bits, some new static fns seem to lack their toplevel comment.

The middle-end pieces are ok.

Thanks,
Richard.

> for  include/ChangeLog
>
> * dwarf2.def (DW_AT_GNU_locviews): New.
> * dwarf2.h (enum dwarf_location_list_entry_type): Add
> DW_LLE_GNU_view_pair.
> (DW_LLE_view_pair): Define.
>
> for  gcc/ChangeLog
>
> * common.opt (gvariable-location-views): New.
> * config.in: Rebuilt.
> * configure: Rebuilt.
> * configure.ac: Test assembler for view support.
> * dwarf2asm.c (dw2_asm_output_symname_uleb128): New.
> * dwarf2asm.h (dw2_asm_output_symname_uleb128): Declare.
> * dwarf2out.c (var_loc_view): New typedef.
> (struct dw_loc_list_struct): Add vl_symbol, vbegin, vend.
> (dwarf2out_locviews_in_attribute): New.
> (dwarf2out_locviews_in_loclist): New.
> (dw_val_equal_p): Compare val_view_list of dw_val_class_view_lists.
> (enum dw_line_info_opcode): Add LI_adv_address.
> (struct dw_line_info_table): Add view.
> (RESET_NEXT_VIEW, RESETTING_VIEW_P): New macros.
> (DWARF2_ASM_VIEW_DEBUG_INFO): Define default.
> (zero_view_p): New variable.
> (ZERO_VIEW_P): New macro.
> (output_asm_line_debug_info): New.
> (struct var_loc_node): Add view.
> (add_AT_view_list, AT_loc_list): New.
> (add_var_loc_to_decl): Add view param.  Test it against last.
> (new_loc_list): Add view params.  Record them.
> (AT_loc_list_ptr): Handle loc and view lists.
> (view_list_to_loc_list_val_node): New.
> (print_dw_val): Handle dw_val_class_view_list.
> (size_of_die): Likewise.
> (value_format): Likewise.
> (loc_list_has_views): New.
> (gen_llsym): Set vl_symbol too.
> (maybe_gen_llsym, skip_loc_list_entry): New.
> (dwarf2out_maybe_output_loclist_view_pair): New.
> (output_loc_list): Output view list or entries too.
> (output_view_list_offset): New.
> (output_die): Handle dw_val_class_view_list.
> (output_dwarf_version): New.
> (output_compilation_unit_header): Use it.
> (output_skeleton_debug_sections): Likewise.
> (output_rnglists, output_line_info): Likewise.
> (output_pubnames, output_aranges): Update version comments.
> (output_one_line_info_table): Output view numbers in asm comments.
> (dw_loc_list): Determine current endview, pass it to new_loc_list.
> Call maybe_gen_llsym.
> (loc_list_from_tree_1): Adjust.
> (add_AT_location_description): Create view list attribute if
> needed, check it's absent otherwise.
> (convert_cfa_to_fb_loc_list): Adjust.
> (maybe_emit_file): Call output_asm_line_debug_info for test.
> (dwarf2out_var_location): Reset views as needed.  Precompute
> add_var_loc_to_decl args.  Call get_attr_min_length only if we have 
> the
> attribute.  Set view.
> (new_line_info_table): Reset next view.
> (set_cur_line_info_table): Call output_asm_line_debug_info for test.
> (dwarf2out_source_line): Likewise.  Output view resets and labels to
> the assembler, or select appropriate line info opcodes.
> (prune_unused_types_walk_attribs): Handle dw_val_class_view_list.
> (optimize_string_length): Catch it.  Adjust.
> (resolve_addr): Copy vl_symbol along with ll_symbol.  Handle
> dw_val_class_view_list, and remove it if no longer needed.
> (hash_loc_list): Hash view numbers.
> (loc_list_hasher::equal): Compare them.
> (optimize_location_lists): Check whether a view list symbol is
> needed, and whether the locview attribute is present, and
> whether they match.  Remove the locview attribute if no longer
> needed.
>

[SFN+LVU+IEPM v4 7/9] [LVU] Introduce location views

[Alexandre Oliva]

This patch introduces an option to enable the generation of location
views along with location lists.  The exact format depends on the
DWARF version: it can be a separate attribute (DW_AT_GNU_locviews) or
(DW_LLE_view_pair) entries in DWARF5+ loclists.

Line number tables are also affected.  If the assembler is found, at
compiler build time, to support .loc views, we use them and
assembler-computed view labels, otherwise we output compiler-generated
line number programs with conservatively-computed view labels.  In
either case, we output view information next to line number changes
when verbose assembly output is requested.

This patch requires an LVU patch that modifies the exported API of
final_scan_insn.  It also expects the entire SFN patchset to be
installed first, although SFN is not a requirement for LVU.

for  include/ChangeLog

* dwarf2.def (DW_AT_GNU_locviews): New.
* dwarf2.h (enum dwarf_location_list_entry_type): Add
DW_LLE_GNU_view_pair.
(DW_LLE_view_pair): Define.

for  gcc/ChangeLog

* common.opt (gvariable-location-views): New.
* config.in: Rebuilt.
* configure: Rebuilt.
* configure.ac: Test assembler for view support.
* dwarf2asm.c (dw2_asm_output_symname_uleb128): New.
* dwarf2asm.h (dw2_asm_output_symname_uleb128): Declare.
* dwarf2out.c (var_loc_view): New typedef.
(struct dw_loc_list_struct): Add vl_symbol, vbegin, vend.
(dwarf2out_locviews_in_attribute): New.
(dwarf2out_locviews_in_loclist): New.
(dw_val_equal_p): Compare val_view_list of dw_val_class_view_lists.
(enum dw_line_info_opcode): Add LI_adv_address.
(struct dw_line_info_table): Add view.
(RESET_NEXT_VIEW, RESETTING_VIEW_P): New macros.
(DWARF2_ASM_VIEW_DEBUG_INFO): Define default.
(zero_view_p): New variable.
(ZERO_VIEW_P): New macro.
(output_asm_line_debug_info): New.
(struct var_loc_node): Add view.
(add_AT_view_list, AT_loc_list): New.
(add_var_loc_to_decl): Add view param.  Test it against last.
(new_loc_list): Add view params.  Record them.
(AT_loc_list_ptr): Handle loc and view lists.
(view_list_to_loc_list_val_node): New.
(print_dw_val): Handle dw_val_class_view_list.
(size_of_die): Likewise.
(value_format): Likewise.
(loc_list_has_views): New.
(gen_llsym): Set vl_symbol too.
(maybe_gen_llsym, skip_loc_list_entry): New.
(dwarf2out_maybe_output_loclist_view_pair): New.
(output_loc_list): Output view list or entries too.
(output_view_list_offset): New.
(output_die): Handle dw_val_class_view_list.
(output_dwarf_version): New.
(output_compilation_unit_header): Use it.
(output_skeleton_debug_sections): Likewise.
(output_rnglists, output_line_info): Likewise.
(output_pubnames, output_aranges): Update version comments.
(output_one_line_info_table): Output view numbers in asm comments.
(dw_loc_list): Determine current endview, pass it to new_loc_list.
Call maybe_gen_llsym.
(loc_list_from_tree_1): Adjust.
(add_AT_location_description): Create view list attribute if
needed, check it's absent otherwise.
(convert_cfa_to_fb_loc_list): Adjust.
(maybe_emit_file): Call output_asm_line_debug_info for test.
(dwarf2out_var_location): Reset views as needed.  Precompute
add_var_loc_to_decl args.  Call get_attr_min_length only if we have the
attribute.  Set view.
(new_line_info_table): Reset next view.
(set_cur_line_info_table): Call output_asm_line_debug_info for test.
(dwarf2out_source_line): Likewise.  Output view resets and labels to
the assembler, or select appropriate line info opcodes.
(prune_unused_types_walk_attribs): Handle dw_val_class_view_list.
(optimize_string_length): Catch it.  Adjust.
(resolve_addr): Copy vl_symbol along with ll_symbol.  Handle
dw_val_class_view_list, and remove it if no longer needed.
(hash_loc_list): Hash view numbers.
(loc_list_hasher::equal): Compare them.
(optimize_location_lists): Check whether a view list symbol is
needed, and whether the locview attribute is present, and
whether they match.  Remove the locview attribute if no longer
needed.
(index_location_lists): Call skip_loc_list_entry for test.
(dwarf2out_finish): Call output_asm_line_debug_info for test.
Use output_dwarf_version.
* dwarf2out.h (enum dw_val_class): Add dw_val_class_view_list.
(struct dw_val_node): Add val_view_list.
* final.c: Include langhooks.h.
(SEEN_NEXT_VIEW): New.
(set_next_view_needed): New.
(clear_next_view_needed): New.
(maybe_output_next_view): New.
(final_start_function): Rename to...