Re: Problem in cxx_fundamental_alignment_p?

[Dodji Seketeli]

Hello Bernd,

Bernd Schmidt  writes:

> I came across what I think is a bug in cxx_fundamental_alignment_p.
>
> User alignments are specified in units of bytes. This is documented,
> and we can also see the following in handle_aligned_attribute, for the
> case when we have no args:
>   align_expr = size_int (ATTRIBUTE_ALIGNED_VALUE / BITS_PER_UNIT);
> Then, we compute the log of that alignment in check_user_alignment:
>   i = check_user_alignment (align_expr, true)
> That's the log of the alignment in bytes, as we can see a little
> further down:
>   SET_TYPE_ALIGN (*type, (1U << i) * BITS_PER_UNIT);
>
> Then, we call check_cxx_fundamental_alignment_constraints, which
> recomputes the alignment (in bytes) from that log:
>   unsigned requested_alignment = 1U << align_log;
> It then calls cxx_fundamental_alignment_p, where we compare it to
> TYPE_ALIGN values, which are specified in bits. So I think we have a
> mismatch there.
>
> Does that sound right?

Yes, I think you are right on all account.

> The patch below was bootstrapped and tested on x86_64-linux, without
> issues,

The patch looks good to me, thanks.

> but I'm not convinced this code is covered by any testcase.

Hmhm, looking at the test cases, accompanying the initial patch that
introduced that code, I agree.  I guess we should probably add a test case that
uses [[gnu::aligned (val)]], with 'val' being an alignment that is
greater than MAX (TYPE_ALIGN_UNIT (long_long_integer_type_node),
  TYPE_ALIGN_UNIT (long_double_type_node))
in the g++.dg/cpp0x/gen-attrs-*.C series of tests?


-- 
Dodji

Re: Status of rich location work (was Re: [PATCH 06/10] Track expression ranges in C frontend)

[Dodji Seketeli]

> Talking about risks: the reduction of the space for ordinary maps by a
> factor of 32, by taking up 5 bits for the packed range information
> optimization (patch 10):
>  https://gcc.gnu.org/ml/gcc-patches/2015-10/msg02539.html
> CCing Dodji: Dodji; is this reasonable?

FWIW, I am definitely to get this (patch 10/10 of the series) if other
agrees.  I just have some minor questions to ask about that patch and
I replied to the patch to ask.

As for the "reduction of the space for ordinary maps by a factor of 32,
by taking up 5 bits for the packed range information" that you mention,
I think it's a trade off I'd live with.

Ultimately, if it shows that we really move out of space with this, we
should probably explore the impact of just moving to a 64 bits size for
source_location.

Until then, a possible mitigation strategy could be to add an option to
disable the range tracking altogether (even at the preprocessor's lexer
level), to provide an escape path to users running low on resources.  A
bit what we do with -ftrack-macro-expansion=0.

Cheers,

-- 
Dodji

Re: [PATCH 10/10] Compress short ranges into source_location

[Dodji Seketeli]

[...]

> diff --git a/libcpp/line-map.c b/libcpp/line-map.c

[...]

> +
> +  /* Any ordinary locations ought to be "pure" at this point: no
> + compressed ranges.  */
> +  linemap_assert (locus < RESERVED_LOCATION_COUNT
> +   || locus >= LINE_MAP_MAX_LOCATION_WITH_COLS
> +   || locus >= LINEMAPS_MACRO_LOWEST_LOCATION (set)
> +   || pure_location_p (set, locus));

Just for my own education, why aren't the tests

locus < RESERVED_LOCATION_COUNT
|| locus >= LINE_MAP_MAX_LOCATION_WITH_COLS
|| locus >= LINEMAPS_MACRO_LOWEST_LOCATION (set)

not part of pure_location_p() ?  I mean, would it make sense to say that
a locus that that satisfies that condition is pure?

By the way, I like this great piece of code of yours, kudos!

Cheers,

-- 
Dodji

Re: [PATCH 5/5] Add plugin to recursively dump the source-ranges in a tree (v2)

[Dodji Seketeli]

David Malcolm  a écrit:

> This patch adds a test plugin that recurses down an expression tree,
> printing diagnostics showing the ranges of each node in the tree.
>
> It corresponds to:
>   [PATCH 15/22] Add plugin to recursively dump the source-ranges in a tree
> https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00741.html
> from v1 of the patch kit.
>
> Changes in v2:
>   * the output no longer contains the PARAM_DECL and INTEGER_CST
> leaves since we no longer have range data for them; updated
> the expected output accordingly.
>   * slightly updated to eliminate use of SOURCE_RANGE
>
> Updated screenshot:
>   
> https://dmalcolm.fedorapeople.org/gcc/2015-09-22/diagnostic-test-show-trees-1.html
>
> gcc/testsuite/ChangeLog:
>   * gcc.dg/plugin/diagnostic-test-show-trees-1.c: New file.
>   * gcc.dg/plugin/diagnostic_plugin_show_trees.c: New file.
>   * gcc.dg/plugin/plugin.exp (plugin_test_list): Add
>   diagnostic_plugin_show_trees.c and
>   diagnostic-test-show-trees-1.c.

For what it's worth, this looks good to me.

Thanks!

-- 
Dodji

Re: [PATCH] v3 of diagnostic_show_locus and rich_location (was Re: [PATCH 2/5] Reimplement diagnostic_show_locus, introducing rich_location classes (v2))

[Dodji Seketeli]

[Note to libcpp, C, and Fortran maintainers: we still need your input :-)]

Hello,

David Malcolm  writes:

[...]

> Here's the revised comment I put in the attached patch:

[...]

> +   The class caches the lookup of the color codes for the above.
> +
> +   The class also has responsibility for tracking which of the above is
> +   active, filtering out unnecessary changes.  This allows layout::print_line
> +   to simply request a colorization code for *every* character it prints
> +   thorough this class, and have the filtering be done for it here.

You probably meant "*through* this class" ?

> */

> Hopefully that comment explains the possible states the colorizer can
> have.

Yes it does, great comment, thank you.


> FWIW I have a follow-up patch to add support for fix-it hints, so they
> might be another kind of colorization state.
> (see https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00732.html for the
> earlier version of said patch, in v1 of the kit).

Yeah, I'll comment on that one separatly.

>> Also, I am thinking that there should maybe be a layout::state type,
>> which would have two notional properties (for now): range_index and
>> draw_caret_p. So that this function:
>> 
>> +bool
>> +layout::get_state_at_point (/* Inputs.  */
>> +int row, int column,
>> +int first_non_ws, int last_non_ws,
>> +/* Outputs.  */
>> +int *out_range_idx,
>> +bool *out_draw_caret_p)
>> 
>> Would take just one output parameter, e.g, a reference to
>> layout::state.
>
> Fixed, though I called it "struct point_state", given that it's coming
> from get_state_at_point.  I passed it by pointer, since AFAIK our coding
> standards don't yet approve of the use of references in the codebase
> (outside of places where we need them e.g. container classes).

Great.  Thanks.

>
> I also added a unit test for a rich_location with two caret locations
> (mimicking one of the Fortran examples), to give us coverage for this
> case:
>
> +void test_multiple_carets (void)
> +{
> +#if 0
> +   x = x + y /* { dg-warning "8: test" } */
> +/* { dg-begin-multiline-output "" }
> +x = x + y
> +A   B
> +   { dg-end-multiline-output "" } */
> +#endif
> +}
>
> where the "A" and "B" as caret chars are coming from new code in the
> show_locus unittest plugin.

Yeah, saw that.  Excellent, thanks.

[...]

>> +  if (0)
>> +show_ruler (context, line_width, m_x_offset);
>> 
>> This should probably be removed from the final code to be committed.
>
> FWIW, the ruler is very helpful to me when debugging the locus-printing
> (e.g. when adding fix-it-hints), and if we remove that if (0) call, we
> get:
>
> warning: ‘void show_ruler(diagnostic_context*, int, int)’ defined but
> not used [-Wunused-function]
>
> which will break bootstrap, so perhaps it instead should be an option?
> "-fdiagnostics-show-ruler" or somesuch?
>
> I don't know that it would be helpful to end-users though.
>
> I'd prefer to just keep it in the code with the
>   if (0)
> as-is, since it's useful "scaffolding" for hacking on the code.
>

OK, I understand; though, as Manuel noted elsewhere, you might rename
that function debug_show_ruler and declare it as:

DEBUG_FUNCTION static void
debug_show_ruler (diagnostic_context *context, int max_width, int x_offset)
{
  /* ...  */
}
to comply with that is generally done in the compiler.

[...]

>> +/* Get the column beyond the rightmost one that could contain a caret or
>> +   range marker, given that we stop rendering at trailing whitespace.  */
>> +
>> +int
>> +layout::get_x_bound_for_row (int row, int caret_column,
>> + int last_non_ws)
>> 
>> Please describe what the parameters mean here, especially last_non_ws.
>> I had to read its code to know that last_non_ws was the *column* of
>> the last non white space character.
>
> I renamed it to "last_non_ws_column", and fleshed out the comment

OK.

[...]

>>  void
>>  diagnostic_show_locus (diagnostic_context * context,
>> const diagnostic_info *diagnostic)
>> @@ -75,16 +710,25 @@ diagnostic_show_locus (diagnostic_context * context,
>>  return;
>> 
>> +  /* The GCC 5 routine. */
>> 
>> I'd say the GCC <= 5 routine ;-)
>
>> +  else
>> +/* The GCC 6 routine.  */
>> 
>> And here, the GCC > 5 routine.
>
> Changed to "GCC < 6" and "GCC >= 6", on the pedantic grounds that e.g.
> 5.1 > 5

OK.

>
>> I would be surprised to see this patch in particular incur any
>> noticeable increase in time and space consumption, but, have you noticed
>> anythying related to that during bootstrap?
>
> I hadn't noticed it, but I wasn't timing.  I'll have a look.

Ok, thanks.

> One possible nit here is that the patch expands locations when
> constructing rich_location instances, and it does that for warnings
> before the logic to ignore them.  So there may be some extra calls there
> that 

Re: [RFC PATCH] parse #pragma GCC diagnostic in libcpp

[Dodji Seketeli]

Manuel López-Ibáñez <lopeziba...@gmail.com> writes:

> On 25 September 2015 at 17:14, Dodji Seketeli <do...@redhat.com> wrote:
>> The caller of do_pragma(), which is destringize_and_run() then detects
>> that pfile->directive_result.type is set, and then puts the tokens of
>> the pragma back into the input stream again.  So next time the FE
>> requests more tokens, it's going to get the same pragma tokens.
>>
>> So, maybe you could alter pragma_entry::is_deferred; change it into a
>> flag which type is an enum that says how the the pragma is to be
>> handled; either internally and its tokens shouldn't be visible to the FE
>> (this is what the current pragma_entry::is_internal means), internally
>> and the tokens would be visible to the FE, or deferred.
>>
>> Then do do_pragma() would be adjusted to change the if (p->is_deferred)
>> clause to allow the third handling kind I just talked about.

[...]

> behaving as if the pragma was unknown did work:
>
> @@ -1414,11 +1435,11 @@ do_pragma (cpp_reader *pfile)
> }
>  }
>
>if (p)
>  {
> -  if (p->is_deferred)
> +  if (p->type == DEFERRED)
> {
>   pfile->directive_result.src_loc = pragma_token_virt_loc;
>   pfile->directive_result.type = CPP_PRAGMA;
>   pfile->directive_result.flags = pragma_token->flags;
>   pfile->directive_result.val.pragma = p->u.ident;
> @@ -1439,11 +1460,12 @@ do_pragma (cpp_reader *pfile)
>   (*p->u.handler) (pfile);
>   if (p->allow_expansion)
> pfile->state.prevent_expansion++;
> }
>  }
> -  else if (pfile->cb.def_pragma)
> +
> +  if ((!p || p->type == INTERNAL_VISIBLE) && pfile->cb.def_pragma)
>  {
>if (count == 1 || pfile->context->prev == NULL)
> _cpp_backup_tokens (pfile, count);
>else
> {
>
> Yet, there is another problem. Now the FE sees the pragma and it warns
> with -Wunknown-pragma.

Couldn't we change the FE to make it not warn on pragma entries of type
INTERNAL_VISIBLE?

Thank you for looking into this.

-- 
Dodji

Re: [PATCH 2/5] Reimplement diagnostic_show_locus, introducing rich_location classes (v2)

[Dodji Seketeli]

Manuel López-Ibáñez <lopeziba...@gmail.com> a écrit:

> On 25 September 2015 at 10:51, Dodji Seketeli <do...@seketeli.org> wrote:
>> The line-map parts are OK to me too, but I have no power on those.  So I
>
> You are listed as "line map" maintainer in MAINTAINERS. I rooted for
> you! :)

Right, I meant the libcpp parts (which are not line-map.h), sorry :-)

Cheers,

-- 
Dodji

Re: [PATCH 4/5] Implement tree expression tracking in C FE (v2)

[Dodji Seketeli]

David Malcolm <dmalc...@redhat.com> a écrit:

> On Fri, 2015-09-25 at 16:06 +0200, Dodji Seketeli wrote:
>> Hello,
>> 
>> Similarly to a comment I made on the previous patch of the series,
>> 
>> +++ b/libcpp/include/line-map.h
>> 
>> [...]
>> 
>>  struct GTY(()) location_adhoc_data {
>>source_location locus;
>> +  source_range src_range;
>>void * GTY((skip)) data;
>>  };
>> 
>> Could we just change the type of locus and make it be a source_range
>> instead?  With the right converting constructor (in the source_range
>> class) that takes a source_location, the amount of churn should
>> hopefully be minimized, or maybe I am missing something ...
>
> Thanks.
>
> I think that the above is one place where we *would* want both locus and
> src_range.

Right, as opposed to the token case where conceptually, all we need is
the begining and the end of the token.  My confusion.

[...]

> As noted elsewhere, we might try to pack short ranges directly into the
> 32 bits of source_location:
>  https://gcc.gnu.org/ml/gcc-patches/2015-09/msg01826.html
> which would avoid having to use ad-hoc for such short ranges; ideally
> most tokens.  I'm experimenting with that (I don't have it fully working
> yet).

Right.  My personal inclination would be to make the general case of
storing all ranges in this adhoc data structure, or, even, into another
on-the-side data structure, similar to the adhoc one, but dedicated to
range associated to points, as you see fit.

Then when that works, consider the optimization of stuffing short ranges
into the 32 bits of source_location, depending on the memory profiles we
get.  But it's your call :-)

[...]

>> +location_t
>> +set_block (location_t loc, tree block)
>> +{
>> 
>> Likewise.  I am wondering if we shouldn't even change the name of this
>> function to better suit what it does: associate a tree to a location.
>
> "associate_tree_with_location" ?

If you find it cool, I am cool :-)

[...]

>> +++ b/gcc/c/c-tree.h
>> @@ -132,6 +132,9 @@ struct c_expr
>>   The type of an enum constant is a plain integer type, but this
>>   field will be the enum type.  */
>>tree original_type;
>> +
>> +  /* FIXME.  */
>> +  source_range src_range;
>>  };
>> 
>> Why a FIXME here?
>
> To remind myself before posting the patches that I need to give the
> field a descriptive comment, explaining what purpose it serves.
>
> Oops :)
>
> It probably should read something like this:
>
>   /* The source range of this C expression.  This might
>  be thought of as redundant, since ranges for
>  expressions can be stored in a location_t, but
>  not all tree nodes in expressions have a location_t.
>
>  Consider this source code:  
>
>   int test (int foo)
>   {
> return foo * 100;
> ^^^   ^^^
>}
>
> During C parsing, the ranges for "foo" and "100" are
> stored within this field of c_expr, but after parsing
> to GENERIC, all we have is a VAR_DECL and an
> INTEGER_CST (the former's location is in at the top of the
> function, and the latter has no location).
>
> For consistency, we store ranges for all expressions
> in this field, not just those that don't have a
> location_t. */
>   source_range src_range;

Great, thanks.

[...]

> [BTW, I'm about to disappear on a vacation from tomorrow until October
> 6th, and will be away from email and computers]

Thanks for the heads-up.

Cheers,

-- 
Dodji

Re: [RFC PATCH] parse #pragma GCC diagnostic in libcpp

[Dodji Seketeli]

Manuel López-Ibáñez  writes:

> Currently, #pragma GCC diagnostic is handled entirely by the FE. This
> has several drawbacks:
>
> * PR c++/53431 - C++ preprocessor ignores #pragma GCC diagnostic: The
> C++ parser lexes (and preprocesses) before handling the pragmas.
>
> * PR 53920 - "gcc -E" does not honor #pragma GCC diagnostic ignored
> "-Wunused-macro": Because -E does not invoke the FE code that parses
> the FE pragmas.
>
> * PR 64698 - preprocessor ignores #pragma GCC diagnostic when using
> -save-temps. Same issue as above.
>
> The following patch moves the handling of #pragma GCC diagnostic to
> libcpp but keeps the interface with the diagnostic machinery in the FE
> by using a call-back function.
>
> One serious problem with this approach is that the preprocessor will
> delete the pragmas from the preprocessed output, thus '-E',
> '-save-temps'  will not contain the pragmas and compiling the
> preprocessed file will trigger the warnings that they were meant to
> suppress.  Any ideas how to prevent libcpp from deleting the #pragmas?

[...]

Joseph Myers  writes:

> On Sun, 20 Sep 2015, Manuel López-Ibáñez wrote:

>> > I don't see any other way to fix these PRs, but I don't know how to
>> > keep the pragmas from being deleted by the preprocessor.
>
> I'd suppose you want a new type of pragma, that acts like a combination of 
> a deferred one and one for which a handler is called immediately by 
> libcpp.  libcpp would call the handler but also create a CPP_PRAGMA token.  
> The front-end code calling pragma handlers would need to know to do 
> nothing with such pragmas; the token would only be for textual 
> preprocessor output.

Right.  I was thinking about something along those lines as well.

So in concrete terms, in libcpp, once do_pragma() has handled the
pragma, it does two things that are interesting to us:

  * If the pragma is an internal one -- one that is handled immediately by
libcpp, then it keeps going, chewing away at more tokens.

  * If the pragma is a deffered one -- one that is to be handled later
by the FE, it updates pfile->directive_result.type to CPP_PRAGMA
(and sets up some ancillary state there too).

The caller of do_pragma(), which is destringize_and_run() then detects
that pfile->directive_result.type is set, and then puts the tokens of
the pragma back into the input stream again.  So next time the FE
requests more tokens, it's going to get the same pragma tokens.

So, maybe you could alter pragma_entry::is_deferred; change it into a
flag which type is an enum that says how the the pragma is to be
handled; either internally and its tokens shouldn't be visible to the FE
(this is what the current pragma_entry::is_internal means), internally
and the tokens would be visible to the FE, or deferred.

Then do do_pragma() would be adjusted to change the if (p->is_deferred)
clause to allow the third handling kind I just talked about.

I hope this helps.

Cheers,

-- 
Dodji

Re: [PATCH 3/5] Implement token range tracking within libcpp and the C FE (v2)

[Dodji Seketeli]

David Malcolm <dmalc...@redhat.com> a écrit:

> On Fri, 2015-09-25 at 11:13 +0200, Dodji Seketeli wrote:
[...]

>> Funny; I first overlooked this comment of you, and then when reading the
>> patch, I asked myself "why keep the existing location_t" ?  I mean, in
>> here:
>> 
>>  /* A preprocessing token.  This has been carefully packed and should
>> -   occupy 16 bytes on 32-bit hosts and 24 bytes on 64-bit hosts.  */
>> +   occupy 16 bytes on 32-bit hosts and 24 bytes on 64-bit hosts.
>> +   FIXME: the above comment is no longer true with this patch.  */
>>  struct GTY(()) cpp_token {
>>source_location src_loc;  /* Location of first char of token.  */
>> +  source_range src_range;   /* Source range covered by the token.  
>> */
>>ENUM_BITFIELD(cpp_ttype) type : CHAR_BIT;  /* token type */
>>unsigned short flags; /* flags - see above */
>>  
>> You could just change the type of src_loc and make it be a source_range.
>
> Interesting idea.
>
> For the general case of expressions, I want a location to mean a
> caret/point plus a range that contains it, but for tokens, the
> caret/point is always at the start of the range.  So maybe a src_range
> would do here.
>
> That said, in patches 3 and 4 of this kit I'm experimenting with
> representation; as I said in the blurb for patch 3: "See the
> cover-letter for this patch kit which describes how we might go back to
> using just a location_t, and stashing the range inside the location_t.
> I'm doing it this way for now to allow for more flexibility as I
> benchmark and explore implementation options."

Right.

> So patches 3 and 4 are rather more experimental than patches 1,2 and 5,
> as I find out what the different representations do to the time
> consumption of the compiler.

Understood.

> I like the idea of "source_location" and "location_t" becoming a
> representation of "(point/caret + range)" rather than just a
> point/caret, since this means that we can pass location_t around as
> before, but then we can extract ranges from them, and all of the
> existing diagnostics ought to "automagically" gain underlines "for
> free".

Me too.

>  I'm experimenting with ways to try to do that efficiently, with
> strategies for packing things into the 32-bits compactly; see e.g.:
>  https://gcc.gnu.org/ml/gcc-patches/2015-09/msg01826.html
>
> If so, then cpp_token's src_loc would remain a source_location;
> source_location itself becomes richer.
>
>> Source range could take a converting constructor, that takes a
>> source_location, so that the existing code that does "cpp_token.src_loc
>> = a_source_location;" keeps working.
>> 
>> But then, in the previous patch of the series, I see:
>> 
>> +/* A range of source locations.
>> +
>> +   Ranges are closed:
>> +   m_start is the first location within the range,
>> +   m_finish is the last location within the range.
>> +
>> +   We may need a more compact way to store these, but for now,
>> +   let's do it the simple way, as a pair.  */
>> +struct GTY(()) source_range
>> +{
>> +  source_location m_start;
>> +  source_location m_finish;
>> +
>> +  void debug (const char *msg) const;
>> +
>> +  /* We avoid using constructors, since various structs that
>> + don't yet have constructors will embed instances of
>> + source_range.  */
>> +
>> 
>> But what if we define a default constructor for that one (as well as one
>> that takes a source_location)?  Wouldn't that work for the embedding
>> case that you talk about in that comment?
>
> Perhaps, but I worry that it would lead to a cascade, where suddenly
> we'd need constructors for various other types.  I can try it, I
> guess.

If it leads to a cascade, then don't bother.  My point was precisely to
try to avoid the churn, while limiting the amount of data size increase
for cpp_token in general.

As you implied, if we can just stay with a source_location that carries
the information of a pointer plus a range, even better.

> [BTW, I'm about to disappear on a vacation from tomorrow until October
> 6th, and away from computers]

Thanks for the heads-up.

-- 
Dodji

Re: [PATCH 2/5] Reimplement diagnostic_show_locus, introducing rich_location classes (v2)

[Dodji Seketeli]

Hello David,

I like this!  Thank you very much for working on this.

I think this patch is in great shape, and once we agree on some of the
nits I have commented on below, I think it should go in. Oh, it also
needs the first patch (1/5, dejagnu first) to go in first, as this one
depends on it.  I defer to the dejagnu maintainers for that one.

The line-map parts are OK to me too, but I have no power on those.  So I
defer to the FE maintainers for that.  The diagnostics parts of the
Fortran, C++ and C FE look good to me too; these are just well contained
mechanical adjustments, but I defer to FE maintainers for the final
word.

Please find my comments below.

[...]

diff --git a/gcc/diagnostic-show-locus.c b/gcc/diagnostic-show-locus.c

[...]

+/* A class to inject colorization codes when printing the diagnostic locus,
+   tracking state as it goes.  */
+
+class colorizer
+{

[...]

+  void set_state (int state);
+  void begin_state (int state);
+  void finish_state (int state);

I think the concept of state could use a little bit of explanation, at
least to say that there are the same number of states, as the number
of ranges.  And that the 'state' argument to these functions really is
the range index.

Also, I am thinking that there should maybe be a layout::state type,
which would have two notional properties (for now): range_index and
draw_caret_p. So that this function:

+bool
+layout::get_state_at_point (/* Inputs.  */
+   int row, int column,
+   int first_non_ws, int last_non_ws,
+   /* Outputs.  */
+   int *out_range_idx,
+   bool *out_draw_caret_p)

Would take just one output parameter, e.g, a reference to
layout::state.


+layout::layout (diagnostic_context * context,
+   const diagnostic_info *diagnostic)

[...]

+  if (loc_range->m_finish.file != m_exploc.file)
+   continue;
+  if (loc_range->m_show_caret_p)
+   if (loc_range->m_finish.file != m_exploc.file)
+ continue;

I think the second if clause is redundant.

+  if (0)
+show_ruler (context, line_width, m_x_offset);

This should probably be removed from the final code to be committed.

[...]

+/* Get the column beyond the rightmost one that could contain a caret or
+   range marker, given that we stop rendering at trailing whitespace.  */
+
+int
+layout::get_x_bound_for_row (int row, int caret_column,
+int last_non_ws)

Please describe what the parameters mean here, especially last_non_ws.
I had to read its code to know that last_non_ws was the *column* of
the last non white space character.

[...]

+void
+layout::print_line (int row)

This function is neat.  I like it! :-)

[...]

 void
 diagnostic_show_locus (diagnostic_context * context,
   const diagnostic_info *diagnostic)
@@ -75,16 +710,25 @@ diagnostic_show_locus (diagnostic_context * context,
 return;

+  /* The GCC 5 routine. */

I'd say the GCC <= 5 routine ;-)

+  else
+/* The GCC 6 routine.  */

And here, the GCC > 5 routine.

I would be surprised to see this patch in particular incur any
noticeable increase in time and space consumption, but, have you noticed
anythying related to that during bootstrap?

Cheers,

-- 
Dodji

Re: PR pretty-print/67567 do not pass NULL as a string

[Dodji Seketeli]

Manuel López-Ibáñez  a écrit:

> 2015-09-15  Manuel López-Ibáñez  
>
> PR pretty-print/67567
> * resolve.c (resolve_fl_procedure): Work-around when iface->module
> == NULL.

This is OK, thanks.

-- 
Dodji

Re: [PATCH 3/5] Implement token range tracking within libcpp and the C FE (v2)

[Dodji Seketeli]

Hello,

David Malcolm  a écrit:

> This is an updated version of:
>   https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00736.html
>
> Changes in V2 of the patch:
>   * c_lex_with_flags: don't write through the range ptr if it's NULL
>   * don't add any fields to the C++ frontend's cp_token for now
>   * libcpp/lex.c: prevent usage of stale/uninitialized data in
> _cpp_temp_token and _cpp_lex_direct.
>
> This patch adds source *range* information to libcpp's cpp_token, and to
> c_token in the C frontend.
>
> As noted before, to minimize churn, I kept the existing location_t
> fields, though in theory these are always just equal to the start of
> the source range.

Funny; I first overlooked this comment of you, and then when reading the
patch, I asked myself "why keep the existing location_t" ?  I mean, in
here:

 /* A preprocessing token.  This has been carefully packed and should
-   occupy 16 bytes on 32-bit hosts and 24 bytes on 64-bit hosts.  */
+   occupy 16 bytes on 32-bit hosts and 24 bytes on 64-bit hosts.
+   FIXME: the above comment is no longer true with this patch.  */
 struct GTY(()) cpp_token {
   source_location src_loc; /* Location of first char of token.  */
+  source_range src_range;  /* Source range covered by the token.  */
   ENUM_BITFIELD(cpp_ttype) type : CHAR_BIT;  /* token type */
   unsigned short flags;/* flags - see above */
 
You could just change the type of src_loc and make it be a source_range.

Source range could take a converting constructor, that takes a
source_location, so that the existing code that does "cpp_token.src_loc
= a_source_location;" keeps working.

But then, in the previous patch of the series, I see:

+/* A range of source locations.
+
+   Ranges are closed:
+   m_start is the first location within the range,
+   m_finish is the last location within the range.
+
+   We may need a more compact way to store these, but for now,
+   let's do it the simple way, as a pair.  */
+struct GTY(()) source_range
+{
+  source_location m_start;
+  source_location m_finish;
+
+  void debug (const char *msg) const;
+
+  /* We avoid using constructors, since various structs that
+ don't yet have constructors will embed instances of
+ source_range.  */
+

But what if we define a default constructor for that one (as well as one
that takes a source_location)?  Wouldn't that work for the embedding
case that you talk about in that comment?

The reason why I am asking all this is, memory consumption.  Maybe
you've measured it and it's not relevant, but otherwise, if we could do
away with duplicating the start location and still miminize the churn,
maybe we should try.

-- 
Dodji

Re: [PATCH 4/5] Implement tree expression tracking in C FE (v2)

[Dodji Seketeli]

Hello,

Similarly to a comment I made on the previous patch of the series,

+++ b/libcpp/include/line-map.h

[...]

 struct GTY(()) location_adhoc_data {
   source_location locus;
+  source_range src_range;
   void * GTY((skip)) data;
 };

Could we just change the type of locus and make it be a source_range
instead?  With the right converting constructor (in the source_range
class) that takes a source_location, the amount of churn should
hopefully be minimized, or maybe I am missing something ...

[...]

diff --git a/libcpp/line-map.c b/libcpp/line-map.c

[...]

+source_range
+get_range_from_adhoc_loc (struct line_maps *set, source_location loc)
+{

Please add a comment for this function.

[...]

diff --git a/gcc/tree.c b/gcc/tree.c

+void
+set_source_range (tree *expr, location_t start, location_t finish)

Please add a comment for this function and its overloads.

[...]

+void
+set_c_expr_source_range (c_expr *expr,
+location_t start, location_t finish)
+{

Likewise.

+location_t
+set_block (location_t loc, tree block)
+{

Likewise.  I am wondering if we shouldn't even change the name of this
function to better suit what it does: associate a tree to a location.

+  source_range src_range;
+  if (IS_ADHOC_LOC (loc))
+/* FIXME: can we update in-place?  */
+src_range = get_range_from_adhoc_loc (line_table, loc);

Hmmh, at the moment, I don't think we can update an entry of the adhoc
map that associates {location, range, tree} as all three components
contribute to the hash value of the entry.  A new triplet means a new
entry.

My understanding is that initially the two elements of the pair
{location, tree} were contributing to the hash value because the same
location could very well belong to different blocks.

+  else
+src_range = source_range::from_location (loc);
+
+  return COMBINE_LOCATION_DATA (line_table, loc, src_range, block);
+}

[...]

@@ -6091,6 +6112,10 @@ c_parser_conditional_expression (c_parser *parser, 
struct c_expr *after,
 
   if (c_parser_next_token_is_not (parser, CPP_QUERY))
 return cond;
+  if (cond.value != error_mark_node)
+start = cond.src_range.m_start;
+  else
+start = UNKNOWN_LOCATION;

I think that "getting the start range of a c_expr" is an operation
that is generally useful; and it's also generally useful for that
operation to handle cases where the tree carried by the c_expr can be
an error_mark_node.  So maybe it would be appropriate to have a getter
function for that operation and use it here instead.

You would then use that operation in the other places of this patch
that are getting c_expr::src_range.start -- by the way, those other
places are not handling the error_mark_node case like the above.

[...]

+++ b/gcc/c/c-tree.h
@@ -132,6 +132,9 @@ struct c_expr
  The type of an enum constant is a plain integer type, but this
  field will be the enum type.  */
   tree original_type;
+
+  /* FIXME.  */
+  source_range src_range;
 };

Why a FIXME here?

+#define CAN_HAVE_RANGE_P(NODE) (CAN_HAVE_LOCATION_P (NODE))

Please add a comment for this.

+#define EXPR_LOCATION_RANGE(NODE) (get_expr_source_range (EXPR_CHECK ((NODE

Likewise.

+#define EXPR_HAS_RANGE(NODE) \
+(CAN_HAVE_RANGE_P (NODE) \
+ ? EXPR_LOCATION_RANGE (NODE).m_start != UNKNOWN_LOCATION \
+ : false)
+

Likewise.

[...]
 
+static inline source_range
+get_expr_source_range (tree expr)

Likewise.

+#define DECL_LOCATION_RANGE(NODE) \
+  (get_decl_source_range (DECL_MINIMAL_CHECK (NODE)))
+

Likewise.

+static inline source_range
+get_decl_source_range (tree decl)
+{

Likewise.

-- 
Dodji

Re: [PATCH] Import liboffloadmic from upstream

[Dodji Seketeli]

Ilya Verbin <iver...@gmail.com> writes:

> On Tue, Sep 01, 2015 at 09:58:22 +0200, Dodji Seketeli wrote:
>> Woops.  can you send me the exact two libraries so that I can see what's
>> going wrong?  You can quickly file an issue to
>> https://sourceware.org/bugzilla/enter_bug.cgi?product=libabigail or just
>> send me the two binaries by email. I'll quickly look into this.
>
> Done: https://sourceware.org/bugzilla/show_bug.cgi?id=18904

Thanks!  I think I have fixed the fixed the issue.  Sorry for the
inconvenience.

The ABI changes of the library according to abidiff are:
http://paste.fedoraproject.org/262507/14412012

Cheers,

-- 
Dodji

Re: [PATCH] Import liboffloadmic from upstream

[Dodji Seketeli]

Ilya Verbin  writes:

(...)

> abidiff: ../../src/abg-comparison.cc:10731: virtual void 
> abigail::comparison::fn_parm_diff::report(std::ostream&, const string&) 
> const: Assertion `get_type_diff() && get_type_diff()->to_be_reported()' 
> failed.
> Aborted (core dumped)

Woops.  can you send me the exact two libraries so that I can see what's
going wrong?  You can quickly file an issue to
https://sourceware.org/bugzilla/enter_bug.cgi?product=libabigail or just
send me the two binaries by email. I'll quickly look into this.

Sorry for the inconvenience.

Cheers,

-- 
Dodji

Re: [obvious fix] fix off-by-one error when printing the caret character

[Dodji Seketeli]

Manuel López-Ibáñez lopeziba...@gmail.com writes:


 Index: ChangeLog
 ===
 --- ChangeLog   (revision 223445)
 +++ ChangeLog   (working copy)
 @@ -1,3 +1,8 @@
 +2015-05-20  Manuel López-Ibáñez  m...@gcc.gnu.org
 +
 +   * diagnostic.c (diagnostic_print_caret_line): Fix off-by-one error
 +   when printing the caret character.
 +

This is OK, thanks!

Cheers,

-- 
Dodji

Re: [PATCH diagnostics/fortran] Handle two locations for the same diagnostic. Convert all gfc_warning_1 and gfc_notify_std_1 calls

[Dodji Seketeli]

Manuel López-Ibáñez lopeziba...@gmail.com writes:

 On 15 May 2015 at 10:39, Dodji Seketeli do...@redhat.com wrote:
 Manuel López-Ibáñez lopeziba...@gmail.com writes:
 -/* Expand the location of this diagnostic. Use this function for 
 consistency. */
 +/* Return the location associated to this diagnostic. WHICH specifies

 Here, I think only the 'W' (in WHICH) should be uppercase.

 I'm following the convention that parameter names are uppercase in the
 description of functions.

Oh, okay then.  My bad.  Sorry.

  /* The type of a text to be formatted according a format specification
 along with a list of things.  */
  struct text_info
  {
 +public:

 As this is a struct, the 'public' here is not necessary, as the members
 are public by default.

 I have a very poor memory for such details ;), since we are using
 'private:' already, does it really hurt to be explicit and use
 'public:' here?

It doesn't hurt, per se.  But I think it's a very common style to avoid
specifying the 'public' in this case, so I'd rather just remove it, yes.

[...]

OK to commit with this change then.

Cheers,

-- 
Dodji

Re: [PATCH diagnostics/fortran] Handle two locations for the same diagnostic. Convert all gfc_warning_1 and gfc_notify_std_1 calls

[Dodji Seketeli]

Manuel López-Ibáñez lopeziba...@gmail.com writes:

 Thanks for the review. I followed all your suggestions. For the
 accessor functions, I was not sure what type you would prefer, so I
 implemented them as C++ methods and made use of 'private' to be sure
 they are the only way to access the locations array. If you want me to
 change it, just tell me what you prefer. I also replaced
 diagnostic_same_locus with diagnostic_same_line.

Great, thank you.


 Bootstrapped  regression tested on x86_64-linux-gnu.

 OK?

Yes, but with the slight changes below.  Sorry I spotted them just now,
but they are quick to fix, I believe.


 Manuel.


[...]

 -/* Expand the location of this diagnostic. Use this function for 
 consistency. */
 +/* Return the location associated to this diagnostic. WHICH specifies

Here, I think only the 'W' (in WHICH) should be uppercase.


[...]


  /* The type of a text to be formatted according a format specification
 along with a list of things.  */
  struct text_info
  {
 +public:

As this is a struct, the 'public' here is not necessary, as the members
are public by default.

const char *format_spec;
va_list *args_ptr;
int err_no;  /* for %m */
 -  location_t *locus;
void **x_data;
 +
 +  inline location_t  set_location (unsigned int index_of_location)

I think it's less surprising to have the this function take two
parameters:  The index_of_location and the new new location.

So that it's used by doing: set_location (0, the_new_location);

 +  {
 +gcc_checking_assert (index_of_location  MAX_LOCATIONS_PER_MESSAGE);
 +return this-locations[index_of_location];
 +  }
 +
 +  inline location_t get_location (unsigned int index_of_location) const
 +  {
 +gcc_checking_assert (index_of_location  MAX_LOCATIONS_PER_MESSAGE);
 +return this-locations[index_of_location];
 +  }
 +
 +private:
 +  location_t locations[MAX_LOCATIONS_PER_MESSAGE];
  };
  

[...]

OK to commit with the changes above.

Thanks a lot!

-- 
Dodji

Re: [PATCH diagnostics/fortran] Handle two locations for the same diagnostic. Convert all gfc_warning_1 and gfc_notify_std_1 calls

[Dodji Seketeli]

Hello Manuel,

Sorry for my late reply, and thank you very much for working on this.

I have looked at the patch and I like it!

I guess I just have some few lateral nits to pick.

 The Fortran FE allows diagnostics with two different locations.
 Depending on whether these locations are on the same line or not, this
 may produce one or two caret lines. This is the last remaining issue
 left to make Fortran diagnostics use the common code.

Yes, I remember.

[...]

 I added support for this in the common diagnostics, although Fortran
 is the only user for now. The new common code should be flexible
 enough to support the Clang style (which I guess is likely to be what
 C/C++ FEs end up supporting sooner or later) while still supporting
 the Fortran style.

Good.

 Supporting this in the common diagnostics code requires having two
 locations in struct diagnostic_info and two pointers in struct
 text_info. That seems a waste and overtly complex. Thus, I moved the
 new location array directly to struct text_info and added an accessor
 function diagnostic_location().

Agreed.

[...]


 Index: gcc/pretty-print.h

[...]

 --- gcc/pretty-print.h(revision 222087)
 +++ gcc/pretty-print.h(working copy)
 @@ -33,11 +33,13 @@ along with GCC; see the file COPYING3.  
  struct text_info
  {
const char *format_spec;
va_list *args_ptr;
int err_no;  /* for %m */
 -  location_t *locus;
 +  /* This message can have associated two locations at most.  If the
 + first location is UNKNOWN_LOCATION, the second is not valid.  */
 +  location_t location[2];

Here, I would call the data member locations (note the 's' at the
end).

Also, I'd define a constant (a macro, sigh) named like e.g,
MAX_LOCATIONS_PER_MESSAGE that is set to '2', rather than carrying
forcing users of these locations to know that there are specifically
two locations here.

[...]

void **x_data;
  };
  


 Index: gcc/diagnostic.h

[..]

  /* A diagnostic is described by the MESSAGE to send, the FILE and LINE of
 its context and its KIND (ice, error, warning, note, ...)  See complete
 list in diagnostic.def.  */
  struct diagnostic_info
  {
 +  /* Text to be formatted. It also contains the location(s) for this
 + diagnostic.  */
text_info message;
 -  location_t location;
unsigned int override_column;

[...]

  
 -  /* Character used for caret diagnostics.  */
 -  char caret_char;
 +  /* Characters used for caret diagnostics.  */
 +  char caret_char[2];
  

Here, I'd call the data member caret_chars (with an 's' at the end)
and I'd use the same MAX_LOCATIONS_PER_MESSAGE constant as above,
rather that the '2' literal.

[...]

 --- gcc/tree-pretty-print.c   (revision 222087)
 +++ gcc/tree-pretty-print.c   (working copy)

[...]

 @@ -3618,12 +3618,11 @@ newline_and_indent (pretty_printer *pp, 
  
  void
  percent_K_format (text_info *text)
  {
tree t = va_arg (*text-args_ptr, tree), block;
 -  gcc_assert (text-locus != NULL);
 -  *text-locus = EXPR_LOCATION (t);
 +  text-location[0] = EXPR_LOCATION (t);

I guess I'd prefer to have an accessor (e.g,
source_location text_info_location(text_info, int index_of_location))
that returns the location and checks that we are accessing a location
that is below the MAX_LOCATIONS_PER_MESSAGE maximum, rather than just
doing text-locations[0] here.

And, likewise for the other similar spots that access
text_info::locations.

[...]

 --- gcc/diagnostic.c  (revision 222087)
 +++ gcc/diagnostic.c  (working copy)
 @@ -144,11 +144,12 @@ diagnostic_initialize (diagnostic_contex

[...]

context-show_caret = false;
diagnostic_set_caret_max_width (context, pp_line_cutoff 
 (context-printer));
 -  context-caret_char = '^';
 +  context-caret_char[0] = '^';
 +  context-caret_char[1] = '^';

I'd use a loop from O to MAX_LOCATIONS_PER_MESSAGE to initialize
this.  Or maybe rather a dedicated little function for this even; as
you see fit.

[...]

 @@ -239,11 +240,12 @@ diagnostic_set_info_translated (diagnost
   diagnostic_t kind)
  {
diagnostic-message.err_no = errno;
diagnostic-message.args_ptr = args;
diagnostic-message.format_spec = msg;
 -  diagnostic-location = location;
 +  diagnostic-message.location[0] = location;
 +  diagnostic-message.location[1] = UNKNOWN_LOCATION;

I'd use a loop from 1 to to MAX_LOCATIONS_PER_MESSAGE to set the
UNKNOWN_LOCATION.  I understand that the loop will step only one
iteration, but the goal is to be ready for when a front end is going
to need three or more locations per messages.  We'd then just need to
to adjust MAX_LOCATIONS_PER_MESSAGE and the whole thing would almost
Just Work ™.

[...]

  /* Print the physical source line corresponding to the location of
 -   this diagnostic, and a caret indicating the precise column.  */
 +   this diagnostic, and a caret indicating the precise column.  This
 +   function only prints two caret characters if the two locations given by
 +   DIAGNOSTIC are on the same 

Re: [PATCH] Quiet down -Wlogical-op a bit (PR c/61534)

[Dodji Seketeli]

Hi!

Marek Polacek pola...@redhat.com writes:

 This patch stifles -Wlogical-op a bit: don't warn if either operand
 comes from a macro expansion.  As the comment says, it doesn't fix the
 bug completely, but it's a simple improvement.

I cannot approve this patch, but for what it's worth, I like it and
would vote for it to go in.

 I did this by introducing a new macro.

Fair enough.

[...]

 Bootstrapped/regtested on x86_64-linux, ok for trunk?

[+1] from me.

Thanks!

-- 
Dodji

Re: [PATCH] Fix __has_{cpp_}attribute with -traditional-cpp (PR preprocessor/65238)

[Dodji Seketeli]

Jason Merrill ja...@redhat.com writes:

 On 03/20/2015 12:48 PM, Jakub Jelinek wrote:
 On Fri, Mar 20, 2015 at 12:30:44PM -0400, Jason Merrill wrote:
 On 03/11/2015 03:10 PM, Jakub Jelinek wrote:
 __has_{cpp_,}attribute builtin macros are effectively function-like macros
 taking one argument (and the ISO preprocessor expands macros in the 
 argument
 which is IMHO desirable), but the traditional preprocessor has been 
 crashing
 on them or reporting errors.

 Why do we want ISO preprocessor behavior in this specific situation?

 You mean that we would handle
 #define U unused
 #if __has_attribute(U)
 int u __attribute__((unused));
 #endif
 differently between ISO and traditional preprocessing?

 That would be surprising to users.

 Why surprising?  Don't users of the traditional preprocessor expect
 traditional preprocessor behavior?

One of the reasons why I thought it'd be nice to have the traditionnal
mode support the macro-expansion of the arguments here is that there
already are cases where the traditionnal mode supports ISO behaviour.
For instance, the documentation of cpp says:

10.3 Traditional miscellany
===

Here are some things to be aware of when using the traditional
preprocessor.

[...]

   * A true traditional C preprocessor does not recognize '#error' or
 '#pragma', and may not recognize '#elif'.  CPP supports all the
 directives in traditional mode that it supports in ISO mode,
 including extensions, with the exception that the effects of
 '#pragma GCC poison' are undefined.

So I thought this useful particular use case of __has_attribute(U) might
well be another of such case even if it's not a directive.

Just my 2 cents.

-- 
Dodji

Re: [PATCH] Fix __has_{cpp_}attribute with -traditional-cpp (PR preprocessor/65238)

[Dodji Seketeli]

Hello Jakub,

Jakub Jelinek ja...@redhat.com writes:

 __has_{cpp_,}attribute builtin macros are effectively function-like macros
 taking one argument (and the ISO preprocessor expands macros in the argument
 which is IMHO desirable), but the traditional preprocessor has been crashing
 on them or reporting errors.
 As the hook uses cpp_get_token and thus the ISO preprocessor, we need to set
 up things such that the argument and ()s around it are already preprocessed
 and ready to be reparsed by the ISO preprocessor (this is similar to how
 e.g. #if/#elif and various other directives are handled).

 Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

 2015-03-11  Jakub Jelinek  ja...@redhat.com

   PR preprocessor/65238
   * internal.h (_cpp_scan_out_logical_line): Add third argument.
   * directives.c (prepare_directive_trad): Pass false to it.
   * traditional.c (_cpp_read_logical_line_trad,
   _cpp_create_trad_definition): Likewise.
   (struct fun_macro): Add paramc field.
   (fun_like_macro): New function.
   (maybe_start_funlike): Handle NODE_BUILTIN macros.  Initialize
   macro-paramc field.
   (save_argument): Use macro-paramc instead of
   macro-node-value.macro-paramc.
   (push_replacement_text): Formatting fix.
   (recursive_macro): Use fun_like_macro helper.
   (_cpp_scan_out_logical_line): Likewise.  Add BUILTIN_MACRO_ARG
   argument.  Initialize fmacro.paramc field.  Handle builtin
   function-like macros.

   * c-c++-common/cpp/pr65238-1.c: New test.
   * gcc.dg/cpp/pr65238-2.c: New test.
   * gcc.dg/cpp/trad/pr65238-3.c: New test.
   * gcc.dg/cpp/trad/pr65238-4.c: New test.

I do not have the rights to ACK this but FWIW it looks OK to me.  Sorry
for the delay in reviewing this.

Thanks!

Cheers,

-- 
Dodji

Re: [PATCH] PR preprocessor/64803 - __LINE__ inside macro is not constant

[Dodji Seketeli]

Jakub Jelinek ja...@redhat.com writes:

 On Mon, Feb 02, 2015 at 03:41:50PM +0100, Dodji Seketeli wrote:
 libcpp/ChangeLog:
 
  * internal.h (cpp_reader::top_most_macro_node): New data member.
  * macro.c (enter_macro_context): Pass the location of the end of
  the top-most invocation of the function-like macro, or the
  location of the expansion point of the top-most object-like macro.
  (cpp_get_token_1): Store the top-most macro node in the new
  pfile-top_most_macro_node data member.
  (_cpp_pop_context): Clear the new cpp_reader::top_most_macro_node
  data member.
 
 gcc/testsuite/ChangeLog:
 
  * gcc.dg/cpp/builtin-macro-1.c: New test case.

 Ok, thanks.

Thanks.  The patch that finally passed bootstrap is the one below.  It's
slightly different in the condition I use to detect that we are popping
the context of the top-most macro expansion stored in
pfile-top_most_macro_node in _cpp_pop_context().  I now use:

+  if (macro == pfile-top_most_macro_node  context-prev == NULL)

And the context-prev == NULL means, this is the first macro expansion
context on the the stack.  I have also corrected a typo by
s/poping/popping/.  I don't know what I was thinking before.

Bootstrapped and tested on x86_64-unknown-linux-gnu against trunk.

libcpp/ChangeLog:

* internal.h (cpp_reader::top_most_macro_node): New data member.
* macro.c (enter_macro_context): Pass the location of the end of
the top-most invocation of the function-like macro, or the
location of the expansion point of the top-most object-like macro.
(cpp_get_token_1): Store the top-most macro node in the new
pfile-top_most_macro_node data member.
(_cpp_pop_context): Clear the new cpp_reader::top_most_macro_node
data member.

gcc/testsuite/ChangeLog:

* gcc.dg/cpp/builtin-macro-1.c: New test case.

Signed-off-by: Dodji Seketeli do...@redhat.com
---
 gcc/testsuite/gcc.dg/cpp/builtin-macro-1.c | 28 +++
 libcpp/internal.h  |  5 +
 libcpp/macro.c | 31 +++---
 3 files changed, 61 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/cpp/builtin-macro-1.c

diff --git a/gcc/testsuite/gcc.dg/cpp/builtin-macro-1.c 
b/gcc/testsuite/gcc.dg/cpp/builtin-macro-1.c
new file mode 100644
index 000..90c2883
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/builtin-macro-1.c
@@ -0,0 +1,28 @@
+/* Origin PR preprocessor/64803
+
+   This test ensures that the value the __LINE__ macro expands to is
+   constant and corresponds to the line of the closing parenthesis of
+   the top-most function-like macro expansion it's part of.
+
+   { dg-do run }
+   { do-options -no-integrated-cpp }  */
+
+#include assert.h
+
+#define C(a, b) a ## b
+#define L(x) C(L, x)
+#define M(a) int L(__LINE__) = __LINE__; assert(L(__LINE__) == __LINE__);
+
+int
+main()
+{
+  M(a
+);
+
+  assert(L20 == 20);   /* 20 is the line number of the
+  closing parenthesis of the
+  invocation of the M macro.  Please
+  adjust in case the layout of this
+  file changes.  */
+  return 0;
+}
diff --git a/libcpp/internal.h b/libcpp/internal.h
index 1a74020..96ccc19 100644
--- a/libcpp/internal.h
+++ b/libcpp/internal.h
@@ -421,6 +421,11 @@ struct cpp_reader
  macro invocation.  */
   source_location invocation_location;
 
+  /* This is the node representing the macro being expanded at
+ top-level.  The value of this data member is valid iff
+ in_macro_expansion_p() returns TRUE.  */
+  cpp_hashnode *top_most_macro_node;
+
   /* Nonzero if we are about to expand a macro.  Note that if we are
  really expanding a macro, the function macro_of_context returns
  the macro being expanded and this flag is set to false.  Client
diff --git a/libcpp/macro.c b/libcpp/macro.c
index 9571345..1e0a0b5 100644
--- a/libcpp/macro.c
+++ b/libcpp/macro.c
@@ -1228,7 +1228,24 @@ enter_macro_context (cpp_reader *pfile, cpp_hashnode 
*node,
 
   pfile-about_to_expand_macro_p = false;
   /* Handle built-in macros and the _Pragma operator.  */
-  return builtin_macro (pfile, node, location);
+  {
+source_location loc;
+if (/* The top-level macro invocation that triggered the expansion
+  we are looking at is with a standard macro ...*/
+   !(pfile-top_most_macro_node-flags  NODE_BUILTIN)
+   /* ... and it's a function-like macro invocation.  */
+pfile-top_most_macro_node-value.macro-fun_like)
+  /* Then the location of the end of the macro invocation is the
+location of the closing parenthesis.  */
+  loc = pfile-cur_token[-1].src_loc;
+else
+  /* Otherwise, the location of the end of the macro invocation is
+the location of the expansion point of that top-level macro
+invocation

Re: [PATCH] PR preprocessor/64803 - __LINE__ inside macro is not constant

[Dodji Seketeli]

Jakub Jelinek ja...@redhat.com writes:

 On Fri, Jan 30, 2015 at 10:19:26AM +0100, Dodji Seketeli wrote:
 [This is a P1 regression for gcc 5]
 libcpp/ChangeLog:
 
  * internal.h (cpp_reader::top_most_macro_node): New data member.
  * macro.c (enter_macro_context): Pass the location of the end of
  the top-most invocation of the function-like macro, or the
  location of the expansion point of the top-most object-like macro.
  (cpp_get_token_1): Store the top-most macro node in the new
  pfile-top_most_macro_node data member.

 The thing that worries me a little bit on the patch is that
 the new field is never cleared, only overwritten next time we attempt to
 expand a function-like? toplevel macro.  So outside of that it can be stale,
 point to a dead memory.  But if it is guaranteed it won't be accessed in
 that case, perhaps that is safe.

Yes, that is correct.  I didn't worry too much myself because
cpp_reader::top_most_macro_node has the same validity span as
cpp_reader::invocation.  But then, unlike top_most_macro_node,
cpp_reader::invocation is not a pointer, so it's rather harmless.

More precisely pfile-top_most_macro_node is (for now) only accessed
from within enter_macro_context; and there, normally,
pfile-top_most_macro_node is set.

But then I agree that we'd rather be safe than sorry.  So I have updated
the patch to clear that data member when the context of the top most
macro being expanded is popped.

I have just lightly tested it locally but a proper bootstrap  test is
currently underway.  Below is the patch I am currently bootstrapping.

libcpp/ChangeLog:

* internal.h (cpp_reader::top_most_macro_node): New data member.
* macro.c (enter_macro_context): Pass the location of the end of
the top-most invocation of the function-like macro, or the
location of the expansion point of the top-most object-like macro.
(cpp_get_token_1): Store the top-most macro node in the new
pfile-top_most_macro_node data member.
(_cpp_pop_context): Clear the new cpp_reader::top_most_macro_node
data member.

gcc/testsuite/ChangeLog:

* gcc.dg/cpp/builtin-macro-1.c: New test case.

Signed-off-by: Dodji Seketeli do...@redhat.com
---
 gcc/testsuite/gcc.dg/cpp/builtin-macro-1.c | 28 ++
 libcpp/internal.h  |  5 +
 libcpp/macro.c | 32 +++---
 3 files changed, 62 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/cpp/builtin-macro-1.c

diff --git a/gcc/testsuite/gcc.dg/cpp/builtin-macro-1.c 
b/gcc/testsuite/gcc.dg/cpp/builtin-macro-1.c
new file mode 100644
index 000..90c2883
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/builtin-macro-1.c
@@ -0,0 +1,28 @@
+/* Origin PR preprocessor/64803
+
+   This test ensures that the value the __LINE__ macro expands to is
+   constant and corresponds to the line of the closing parenthesis of
+   the top-most function-like macro expansion it's part of.
+
+   { dg-do run }
+   { do-options -no-integrated-cpp }  */
+
+#include assert.h
+
+#define C(a, b) a ## b
+#define L(x) C(L, x)
+#define M(a) int L(__LINE__) = __LINE__; assert(L(__LINE__) == __LINE__);
+
+int
+main()
+{
+  M(a
+);
+
+  assert(L20 == 20);   /* 20 is the line number of the
+  closing parenthesis of the
+  invocation of the M macro.  Please
+  adjust in case the layout of this
+  file changes.  */
+  return 0;
+}
diff --git a/libcpp/internal.h b/libcpp/internal.h
index 1a74020..96ccc19 100644
--- a/libcpp/internal.h
+++ b/libcpp/internal.h
@@ -421,6 +421,11 @@ struct cpp_reader
  macro invocation.  */
   source_location invocation_location;
 
+  /* This is the node representing the macro being expanded at
+ top-level.  The value of this data member is valid iff
+ in_macro_expansion_p() returns TRUE.  */
+  cpp_hashnode *top_most_macro_node;
+
   /* Nonzero if we are about to expand a macro.  Note that if we are
  really expanding a macro, the function macro_of_context returns
  the macro being expanded and this flag is set to false.  Client
diff --git a/libcpp/macro.c b/libcpp/macro.c
index 9571345..90ed11a 100644
--- a/libcpp/macro.c
+++ b/libcpp/macro.c
@@ -1228,7 +1228,24 @@ enter_macro_context (cpp_reader *pfile, cpp_hashnode 
*node,
 
   pfile-about_to_expand_macro_p = false;
   /* Handle built-in macros and the _Pragma operator.  */
-  return builtin_macro (pfile, node, location);
+  {
+source_location loc;
+if (/* The top-level macro invocation that triggered the expansion
+  we are looking at is with a standard macro ...*/
+   !(pfile-top_most_macro_node-flags  NODE_BUILTIN)
+   /* ... and it's a function-like macro invocation.  */
+pfile-top_most_macro_node-value.macro-fun_like)
+  /* Then the location

[PATCH, libcpp] Do not modify a token once it has been initialized

[Dodji Seketeli]

Hello,

While looking at PR preprocessor/64803, I noticed that builtin_macro
was changing the location a token after it was been handed out by
_cpp_lex_direct().  Use the combination of cpp_force_token_locations()
and cpp_stop_forcing_token_locations() that are intended for exactly
that use case.

Boostrapped and tested on x86_64-unknown-linux-gnu.

libcpp/ChangeLog:

* macro.c (builtin_macro): Use the combination of
cpp_force_token_locations() and cpp_stop_forcing_token_locations()
instead of modifying the location of the token after its
initialization.

OK to for trunk when stage 1 re-opens?

Cheers,

Signed-off-by: Dodji Seketeli do...@redhat.com
---
 libcpp/macro.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/libcpp/macro.c b/libcpp/macro.c
index 9571345..ca199ba 100644
--- a/libcpp/macro.c
+++ b/libcpp/macro.c
@@ -442,9 +442,12 @@ builtin_macro (cpp_reader *pfile, cpp_hashnode *node, 
source_location loc)
 
   /* Set pfile-cur_token as required by _cpp_lex_direct.  */
   pfile-cur_token = _cpp_temp_token (pfile);
+  /* force the location of the token emitted by _cpp_lex_direct() to
+ have the location LOC.  */
+  cpp_force_token_locations (pfile, loc);
   cpp_token *token = _cpp_lex_direct (pfile);
-  /* We should point to the expansion point of the builtin macro.  */
-  token-src_loc = loc;
+  cpp_stop_forcing_token_locations (pfile);
+
   if (pfile-context-tokens_kind == TOKENS_KIND_EXTENDED)
 {
   /* We are tracking tokens resulting from macro expansion.
-- 
Dodji

[PATCH] PR preprocessor/64803 - __LINE__ inside macro is not constant

[Dodji Seketeli]

[This is a P1 regression for gcc 5]

Hello,

Consider the example code mentionned in this PR:

 $ cat -n test.c
  1 #define C(a, b) a ## b
  2 #define L(x) C(L, x)
  3 #define M(a) goto L(__LINE__); __LINE__; L(__LINE__):
  4 M(a /* -- this is the line of the expansion point of M.  */
  5   ); /* -- this is the line of the end of the invocation of M.  */
 $

cc1 -quiet -E test.c yields:

 goto L5; 5; L4:
;

Notice how we have a 'L4' there, where it should be L5.  That is the issue.

My understanding is that during the *second* expansion of __LINE__
(the one between the two L(__LINE__)), builtin_macro() is called by
enter_macro_context() with the location of the expansion point of M
(which is at line 4).  Then _cpp_builtin_macro_text() expand __LINE__
into the line number of the location of the last token that has been
lexed, which is the location of the closing parenthesis of the
invocation of M, at line 5.  So that invocation of __LINE__ is
expanded into 5.

Now let's see why the last invocation of __LINE__ is expanded into 4.

In builtin_macro(), we have this code at some point:

   /* Set pfile-cur_token as required by _cpp_lex_direct.  */
   pfile-cur_token = _cpp_temp_token (pfile);
   cpp_token *token = _cpp_lex_direct (pfile);
   /* We should point to the expansion point of the builtin macro.  */
   token-src_loc = loc;

The first two statements insert a new token in the stream of lexed
token and pfile-cur_token[-1], is the new last token that has been
lexed.  But the location of pfile-cur_token[-1] is the same location
as the location of the previous pfile-cur_token[-1], by courtesy of
_cpp_temp_token().  So normally, in subsequent invocations of
builtin_macro(), the location of pfile-cur_token[-1] should always be
the location of the closing parenthesis of the invocation of M at line
5.  Except that that code in master now has the statement
token-src_loc = loc; on the next line.  That statement actually
sets the location of pfile-cur_token[-1] to 'loc'.  Which is the
location of the expansion point of M, which is on line 4.

So in the subsequent call to builtin_macro() (for the last expansion
of __LINE__ in L(__LINE__)), for _cpp_builtin_macro_text(),
pfile-cur_token[-1].src_loc is going to have a line number of 4.

I think the core issue here is that the location that is passed to
builtin_macro() from enter_macro_context() is not correct when we are
in presence of a top-most function-like macro invocation; in that
case, that location should be the location of the closing parenthesis
of the macro invocation.  Otherwise, if we are in presence of a
top-most object-like macro invocation then the location passed down
to builtin_macro should be the location of the expansion point of the
macro.

That way, in the particular case of the input code above, the location
received by builtin_macro() will always have line number 5.

Boostrapped and tested on x86_64-unknown-linux-gnu against trunk.

libcpp/ChangeLog:

* internal.h (cpp_reader::top_most_macro_node): New data member.
* macro.c (enter_macro_context): Pass the location of the end of
the top-most invocation of the function-like macro, or the
location of the expansion point of the top-most object-like macro.
(cpp_get_token_1): Store the top-most macro node in the new
pfile-top_most_macro_node data member.

gcc/testsuite/ChangeLog:

* gcc.dg/cpp/builtin-macro-1.c: New test case.

Signed-off-by: Dodji Seketeli do...@redhat.com
---
 gcc/testsuite/gcc.dg/cpp/builtin-macro-1.c | 28 
 libcpp/internal.h  |  5 +
 libcpp/macro.c | 27 ---
 3 files changed, 57 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/cpp/builtin-macro-1.c

diff --git a/gcc/testsuite/gcc.dg/cpp/builtin-macro-1.c 
b/gcc/testsuite/gcc.dg/cpp/builtin-macro-1.c
new file mode 100644
index 000..90c2883
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/builtin-macro-1.c
@@ -0,0 +1,28 @@
+/* Origin PR preprocessor/64803
+
+   This test ensures that the value the __LINE__ macro expands to is
+   constant and corresponds to the line of the closing parenthesis of
+   the top-most function-like macro expansion it's part of.
+
+   { dg-do run }
+   { do-options -no-integrated-cpp }  */
+
+#include assert.h
+
+#define C(a, b) a ## b
+#define L(x) C(L, x)
+#define M(a) int L(__LINE__) = __LINE__; assert(L(__LINE__) == __LINE__);
+
+int
+main()
+{
+  M(a
+);
+
+  assert(L20 == 20);   /* 20 is the line number of the
+  closing parenthesis of the
+  invocation of the M macro.  Please
+  adjust in case the layout of this
+  file changes.  */
+  return 0;
+}
diff --git a/libcpp/internal.h b/libcpp/internal.h
index 1a74020..96ccc19 100644
--- a/libcpp/internal.h

Re: [PATCH, libcpp] Do not modify a token once it has been initialized

[Dodji Seketeli]

Andreas Schwab sch...@linux-m68k.org writes:

 +  /* force the location of the token emitted by _cpp_lex_direct() to

 s/force/Force/

Thanks for noticing this, Andreas!

I have updated my local copy of the patch to fix that.

Cheers!

-- 
Dodji

Re: [PATCH] Don't emit backtrace from driver upon fatal compiler signals

[Dodji Seketeli]

Ian Lance Taylor i...@golang.org writes:

 On Thu, Jan 22, 2015 at 12:35 PM, Jakub Jelinek ja...@redhat.com wrote:

 2015-01-22  Jakub Jelinek  ja...@redhat.com

 * diagnostic-core.h (internal_error_no_backtrace): New prototype.
 * diagnostic.def (DK_ICE_NOBT): New kind.
 * diagnostic.c (diagnostic_action_after_output): Handle DK_ICE_NOBT
 like DK_ICE, but never print backtrace.
 (diagnostic_report_diagnostic): Handle DK_ICE_NOBT like DK_ICE.
 (internal_error_no_backtrace): New function.
 * gcc.c (execute): Use internal_error_no_backtrace instead of
 internal_error.
 fortran/
 * gfc-diagnostic.def (DK_ICE_NOBT): New kind.

 This is OK.

FWIW, this is fine from me too,

 Thanks for taking care of this.

Indeed.

Cheers,

-- 
Dodji

Re: [PATCH] -f{no-sanitize,{,no-}sanitize-recover}=all support

[Dodji Seketeli]

Jakub Jelinek ja...@redhat.com writes:

 On Mon, Jan 05, 2015 at 10:40:37PM +0100, Jakub Jelinek wrote:
  Are there any doc updates that need to happen as a result of this patch?
  Patch itself is fine for the trunk, just want to make sure the doc side is
  good too.
 
 You're right, I'll add documentation tomorrow and repost the patch.

 Make that tonight ;), here it is.  I also found a bug in the
 -fsanitize=float-cast-overflow documentation and -f{,no-}sanitize-recover
 and fixed that too to much the implementation.

 Ok?

[...]


 2015-01-05  Jakub Jelinek  ja...@redhat.com

   * opts.c (common_handle_option): Add support for
   -fno-sanitize=all and -f{,no-}sanitize-recover=all.
   * doc/invoke.texi: Document -fno-sanitize=all,
   -f{,no-}sanitize-recover=all.  Document that
   -fsanitize=float-cast-overflow is not enabled
   by -fsanitize=undefined.  Fix up documentation
   of -f{,no-}sanitize-recover.

   * c-c++-common/asan/sanitize-all-1.c: New test.
   * c-c++-common/ubsan/sanitize-all-1.c: New test.
   * c-c++-common/ubsan/sanitize-all-2.c: New test.
   * c-c++-common/ubsan/sanitize-all-3.c: New test.
   * c-c++-common/ubsan/sanitize-all-4.c: New test.

This is OK.  Thanks!

-- 
Dodji

Re: [PATCH fortran/diagnostics] Move gfc_error (buffered) to common diagnostics (try 2)

[Dodji Seketeli]

Manuel López-Ibáñez lopeziba...@gmail.com writes:

 New version using XNEW. Bootstrapped  tested on x86_64-linux-gnu.

 OK?

The diagnostics infrastructure changes are OK for me.  Thanks!

Cheers,

-- 
Dodji

Re: [RFC] diagnostics.c: For terminals, restrict messages to terminal width?

[Dodji Seketeli]

Tobias Burnus bur...@net-b.de writes:

 2014-12-06  Tobias Burnus  bur...@net-b.de
   Manuel L³pez-Ib¡±ez  m...@gcc.gnu.org

 gcc/
   * diagnostic.c (get_terminal_width): Renamed from getenv_columns,
   removed static, and additionally use ioctl to get width.
   (diagnostic_set_caret_max_width): Update call.
   * diagnostic.h (get_terminal_width): Add prototype.
   * opts.c (print_specific_help): Use it for x_help_columns.
   * doc/invoke.texi (fdiagnostics-show-caret): Document how the
   width is set.

 gcc/fortran/
   * error.c (gfc_get_terminal_width): Renamed from
   get_terminal_width and use same-named common function.
   (gfc_error_init_1): Update call.

The diagnostics infrastructure changes are OK for me.  Thanks!

Cheers,

-- 
Dodji

Re: [RFC] diagnostics.c: For terminals, restrict messages to terminal width?

[Dodji Seketeli]

Hello Tobias,

Thank you for this patch.  I have a few comments about it below.  Just
as a heads-up, I am asking questions to Manuel in there, as well as
referring to comments from FX's.  Please read below.

Tobias Burnus bur...@net-b.de writes:

 This patch fixes a Fortran diagnostic regression.

 With the current common diagnostic, the width shown with caret
 diagnostic is determined by:

 case OPT_fmessage_length_:
   pp_set_line_maximum_length (dc-printer, value);
   diagnostic_set_caret_max_width (dc, value);

 plus

  diagnostic_set_caret_max_width (diagnostic_context *context, int value)
  {
/* One minus to account for the leading empty space.  */
value = value ? value - 1
  : (isatty (fileno (pp_buffer (context-printer)-stream))
? getenv_columns () - 1: INT_MAX);

if (value = 0)
  value = INT_MAX;

context-caret_max_width = value;
  }

 where getenv_columns looks at the environment variable COLUMNS.

 Note that -fmessage-length= applies to the error message (wraps) _and_
 the caret diagnostic (truncates) while the COLUMNS variable _only_
 applies to the caret diagnostic. (BTW: The documentation currently
 does not mention COLUMNS.)

I guess we should adjust the documentation to mention COLUMNS.

Manuel, was there a particular reason to avoid mentioning the COLUMNS
environment variable in the documentation?

 On most terminals, which I tried, COLUMNS does not seem to be set. In
 Fortran, error.c's get_terminal_width has a similar check, but
 additionally it uses ioctl to determine the terminal width.

 I think with caret diagnostics, it is useful not to exceed the
 terminal width as having several empty lines before the ^ does not
 really improve the readability. Thus, I would propose to additionally
 use ioctl. Which rises two questions: (a) Should the COLUMNS
 environment variable or ioctl have a higher priority? [Fortran ranks
 ioctl higher; in the patch, for backward compatibilty, I rank COLUMNS
 higher.]

I agree.

 (b) Should ioctl be always used or only for Fortran?

I'd go for using it in the common diagnostics framework, unless there is
a sound motivated reason.  Manuel, do you remember why we didn't query the
TIOCGWINSZ ioctl property to get the terminal size when that capability
was available?

 Comments?

If the change comes with ChangeLog, passes bootstrap and nobody else
objects, I pre-approve this patch.

Thanks!

-- 
Dodji

Re: [PATCH fortran/diagnostics] Move gfc_error (buffered) to common diagnostics

[Dodji Seketeli]

Hello Manuel,

Manuel López-Ibáñez lopeziba...@gmail.com writes:

 New version of the patch. Tobias noticed several problems with the
 previous version:

 * Due to the use of placement-new for the buffered output_buffers
 pp_warning_buffer and pp_error_buffer, the pretty-printer destructor
 may end up trying to free something that it can't. Fixed here by not
 using placement new.

So:

[...]


  /* Report the number of warnings and errors that occurred to the caller.  */
  
 @@ -1525,11 +1625,14 @@ gfc_diagnostics_init (void)
  {
diagnostic_starter (global_dc) = gfc_diagnostic_starter;
diagnostic_finalizer (global_dc) = gfc_diagnostic_finalizer;
diagnostic_format_decoder (global_dc) = gfc_format_decoder;
global_dc-caret_char = '^';
 -  new (pp_warning_buffer) output_buffer ();
 +  pp_warning_buffer = new output_buffer ();

When I look at the code of the destructor the pretty_printer type
(pretty_printer::~pretty_printer) in gcc/pretty-print.c, I see that the
memory for the output buffer is de-allocated using XDELETE.  So I think
the memory for the output buffer should be allocated using XNEW and the
output_buffer type should instantiated using a placement new operator
that uses that XNEWed allocated memory.

Ultimately, I should sit down and make sure the memory management of the
pretty_printer type does away with this surprising placement new
business for good.

 +  pp_warning_buffer-flush_p = false;
 +  pp_error_buffer = new output_buffer ();
 +  pp_error_buffer-flush_p = false;
  }

Cheers,

-- 
Dodji

Re: [RFC] diagnostics.c: For terminals, restrict messages to terminal width?

[Dodji Seketeli]

Manuel López-Ibáñez lopeziba...@gmail.com writes:

[...]

 On 10 December 2014 at 12:10, Dodji Seketeli do...@redhat.com wrote:

[...]

 Manuel, was there a particular reason to avoid mentioning the COLUMNS
 environment variable in the documentation?

 Not that I remember. Perhaps the documentation should say something
 like: The line is truncated to fit into n characters only if the
 option -fmessage-length=n is given, or if the output is a TTY and the
 COLUMNS environment variable is set.

Agreed.  Thank you.

 (b) Should ioctl be always used or only for Fortran?

 I'd go for using it in the common diagnostics framework, unless there is
 a sound motivated reason.  Manuel, do you remember why we didn't query the
 TIOCGWINSZ ioctl property to get the terminal size when that capability
 was available?

 I was not aware this possibility even existed.

Ok :-) Let's go for this then.

[...]

 Note that Fortran has this:

 #ifdef GWINSZ_IN_SYS_IOCTL
 # include sys/ioctl.h
 #endif

 Not sure if this is needed for diagnostics.c or whether it needs some
 configure magick.

I would guess that it should work to use that same #ifdef
GWINSZ_IN_SYS_IOCTL ... #endif snippet in diagnostics.c because, looking
at gcc/configure.ac, I see that we use the autoconf macro
AC_HEADER_TIOCGWINSZ and the autoconf documentation for that macro
reads:

 -- Macro: AC_HEADER_TIOCGWINSZ
 If the use of `TIOCGWINSZ' requires `sys/ioctl.h', then define
 `GWINSZ_IN_SYS_IOCTL'.  Otherwise `TIOCGWINSZ' can be found in
 `termios.h'.

 Use:

  #ifdef HAVE_TERMIOS_H
  # include termios.h
  #endif

  #ifdef GWINSZ_IN_SYS_IOCTL
  # include sys/ioctl.h
  #endif

I am not sure why we were not using the termios.h case though.

 I also agree with FX that the function should be named something like
 get_terminal_width().

Agreed as well.

 In fact, I would argue that the Fortran version should be a wrapper
 around this one to make the output consistent between the new Fortran
 diagnostics and the old ones (*_1 variants) while the transition is in
 progress, even if that means changing the current ordering for
 Fortran. So far, there does not seem to be any reason to prefer one
 ordering over the other, but whatever it is chosen, it would be better
 to be consistent.

I prefer that the environment variable taking precedent is better from a
usability standpoint because it's easier for the user to force the
behaviour she wants by setting an environment variable than by messing
up with some system-wide configuration what would change the output of
querying the TIOCGWINSZ property using an ioctl.

So the patch you (Manual) are proposing looks fine to me, with the
environment variable taking precedence, *if* that is fine for Fortran,
of course.

[...]

Cheers,

-- 
Dodji

Re: [PATCH obvious/diagnostics] Honor override_column when printing the caret

[Dodji Seketeli]

Manuel López-Ibáñez lopeziba...@gmail.com writes:

 libcpp uses diagnostic-override_column to give a custom column number
 to diagnostics. This is taken into account when building the prefix,
 but it was missing when placing the caret.

 Before:

 /home/manuel/override_column.c:1:4: warning: /* within comment [-Wcomment]
  /* /* */
  ^

 After:

 /home/manuel/override_column.c:1:4: warning: /* within comment [-Wcomment]
  /* /* */
 ^


 Committed as obvious in r218295.

Thank you for this.

 2014-12-03  Manuel López-Ibáñez  m...@gcc.gnu.org

 * diagnostic.c (diagnostic_show_locus): Honor override_column when
 placing the caret.

 --- gcc/diagnostic.c(revision 218278)
 +++ gcc/diagnostic.c(working copy)
 @@ -308,10 +308,12 @@ diagnostic_show_locus (diagnostic_contex
|| diagnostic-location == context-last_location)
  return;

context-last_location = diagnostic-location;
s = expand_location_to_spelling_point (diagnostic-location);
 +  if (diagnostic-override_column)
 +s.column = diagnostic-override_column;
line = location_get_source_line (s, line_width);
if (line == NULL || s.column  line_width)
  return;

Thinking about it again, it would be nice to have a regression test for
this.  At some point, I guess we should do something for regression'
tests about the placement of the caret.  Oh well.

Cheers,

-- 
Dodji

Re: [PATCH linemap] Make some asserts fail gracefully

[Dodji Seketeli]

Manuel López-Ibáñez lopeziba...@gmail.com writes:


 +/* Assert that becomes a conditional expression when not checking.

For the sake of clarity towards newcomers, I'd say:

Assert that becomes a conditional expression when checking is
disabled at compilation time.

 +   Use this for conditions that should not happen but if they happen, it
 +   is better to handle them gracefully than ICE. Usage:

Similarly, I'd say:

Use this for conditions that should not happen but if they happen, it is
better to handle them gracefully rather than crash randomly later.
Usage:

OK with those changes and thank for your time and dedication.

Cheers,

-- 
Dodji

Re: [PATCH fortran/linemap] Add enough column hint to fit any possible offset

[Dodji Seketeli]

Hello Manuel, Tobias,

Manuel López-Ibáñez lopeziba...@gmail.com writes:

 This patch actually does not touch linemap but I will appreciate
 Dodji's comments about the approach.

Thanks :-)

 The problem is that in case of long lines, the column hint of 120
 might be too small, thus we do not have enough locations within one
 line to point to a higher column (like 132 in the testcase). Giving as
 column hint the length of the line seems the right fix.

I see.

 The other issue that triggers with this testcase is that, even though
 the column hint allows having line 6 + column 132, since we never
 generate a location except for 6:0, if a new map is created for the
 new line, then it will consider that the last location was 6:0 and the
 new map for line 7 will start at location+1, hence, no offset within
 line 6 can be represented.

Right.  This is done by design to allow for a kind of 'compression' of
the theoretical source location space.  That is, the number of source
locations known to the line map sub-system roughly becomes (roughly) the
number of tokens issued by the tokenizer, rather than the total number
of columns multiplied by the number of lines of the input program.

And of course, we are we today willing something in-between, I guess.
:-)

 My fix here is to create a dummy location for line_len -1 (the last
 column in the line). This forces the next map to start after this last
 column's location.

I think this makes sense.  I'll get to the theoretically unlimited line
length case that Tobias alludes to later below.

 I'm not sure if this latter fix is the approach we want to take.

I think this approach is fine.

 If so, then we may want to change linemap.c itself to force new maps
 to reserve enough locations for any offset in the line instead of
 doing last_location+1.

Hmmh, we must keep in mind that this whole line map thing should keep
working for cases where the primary way to interact with the source
location management sub-system is basically cpp_get_token().  So if that
condition is satisfied, then, why not.  I guess I'd need to look at
specifics of what you are proposing here before talking :-)

[...]

Tobias Burnus bur...@net-b.de writes:

[...]

 My fix here is to create a dummy location for line_len -1 (the last
 column in the line). This forces the next map to start after this
 last column's location.

 My feeling is that that doesn't work for
 -ffree-line-length-none/-ffixed-line-length-none. In that case,
 gfc_option.free_line_length == 0 and there is no limit to the number
 of characters per line. Thus, I fear that

b-location
 - = linemap_line_start (line_table, current_file-line++, 120);
 + = linemap_line_start (line_table, current_file-line++, line_len);


 might even set the line length to 0 – but I haven't checked. For all
 other values, it should work.

I am not sure what the value of line_len is when
gfc_option.free_line_length == 0.  If it's not zero, then we are good.
If it's zero (with the semantics of that zero meaning that the line
might be very big), then we might just get the actual length of the line
in terms of column count and pass that to linemap_line_start.

We must just keep in mind that the line subsystem has a hard limit on
column numbers (I think it's 100 000 columns at the moment).  Passed
that limit, we give up tracking column numbers in source locations; that
means that all columns numbers are set to zero.


I hope this is useful.

Cheers,

-- 
Dodji

Re: [RFC diagnostics/fortran] Move gfc_warning (buffered) to the common diagnostics machinery

[Dodji Seketeli]

Manuel López-Ibáñez lopeziba...@gmail.com writes:

 * Dodji: Do the common diagnostics part look reasonable?

Yes they do.

I just have one minor comment nit:

[...]

 Index: gcc/pretty-print.h

[...]

 +
 +  /* Nonzero means that text should be flushed when
 + appropriate. Otherwise, text is buffered until either
 + pp_pp_really_flush or pp_clear_output_area are called. */

I think you wanted to say pp_really_flush, not pp_pp_realy_flush.

 +  bool flush_p;
  };

[...]

Thanks!

-- 
Dodji

Re: RFA (diagnostics): PATCH to move deprecated source location to separate note

[Dodji Seketeli]

Jason Merrill ja...@redhat.com writes:

 When I was fixing attribute deprecated for C++ templates, I found it
 odd that the source location for the deprecated decl was embedded in
 the warning rather than in a separate inform.  This patch moves it
 out.

 Tested x86_64-pc-linux-gnu.  OK for trunk?

Yes.  Thanks!

-- 
Dodji

Re: [PR c/52952] More precise locations within format strings

[Dodji Seketeli]

Jeff Law l...@redhat.com writes:

 OK, let's go ahead and make that official.  Please update MAINTAINERS ;-)

I have just added a line for myself as a maintainer for the line map sub
system in the MAINTAINERS file.

Thanks!

-- 
Dodji

Re: [PATCH] Optimize ASAN_CHECK checks

[Dodji Seketeli]

Jakub Jelinek ja...@redhat.com writes:

 On Wed, Nov 12, 2014 at 02:09:59PM +0300, Yury Gribov wrote:
  For asan you're right, we can have addresses of decls there etc.
  If you have spare cycles, feel free to take over the patch and adjust it.
  
  I guess I'd wait when this gets to trunk?
 
 Ok, in that case I've bootstrapped/regtested on x86_64-linux/i686-linux what 
 I have with
 the ASAN_CHECK_NON_ZERO_LEN stuff removed from it (all non-INTEGER_CST
 lengths ignored).  Dodji, is this ok for trunk?

[...]

 +++ gcc/sanopt.c  2014-11-12 21:04:50.007325020 +0100
 
  /* This is used to carry information about basic blocks.  It is
 @@ -56,7 +57,29 @@ along with GCC; see the file COPYING3.
  
  struct sanopt_info
  {

[...]

 +  /* True if there is a block with HAS_FREEING_CALL_P flag set
 + on any a path between imm(BB) and BB.  */

s/a//.

Also, I'd rather say on any path between an immediate dominator of
BB, denoted imm(BB), and BB.  That way, subsequent uses of imm(BB)
makes sense more for the new comer.  This is only a suggestion.  If
you feel that formulation is obvious enough, then please ignore my
comment.

 +  bool imm_dom_path_with_freeing_call_p;

[...]

 };

[...]
 
 +/* Return true if there might be any call to free/munmap operation
 +   on any path in between DOM (which should be imm(BB)) and BB.  */
 +
 +static bool
 +imm_dom_path_with_freeing_call (basic_block bb, basic_block dom)
 +{

To ease maintainability, maybe we could assert that:

   gcc_assert (dom == get_immediate_dominator(CDI_DOMINATORS, bb));

?

And thus remove the assert that is at the caller site of this
function, later in maybe_optimize_asan_check_ifn:

 +   basic_block imm = get_immediate_dominator (CDI_DOMINATORS, last_bb);
 +   gcc_assert (imm);
 +   if (imm_dom_path_with_freeing_call (last_bb, imm))
 + break;


Also, when the 'dom' basic block is NULL, couldn't we just return
immediately?

[...]

 +}


[...]

 +/* Optimize away redundant ASAN_CHECK calls.  */
 +
 +static bool
 +maybe_optimize_asan_check_ifn (struct sanopt_ctx *ctx, gimple stmt)
 +{
 +  gcc_assert (gimple_call_num_args (stmt) == 4);
 +  tree ptr = gimple_call_arg (stmt, 1);
 +  tree len = gimple_call_arg (stmt, 2);
 +  basic_block bb = gimple_bb (stmt);
 +  sanopt_info *info = (sanopt_info *) bb-aux;
 +
 +  if (TREE_CODE (len) != INTEGER_CST)
 +return false;
 +  if (integer_zerop (len))
 +return false;
 +
 +  gimple_set_uid (stmt, info-freeing_call_events);

I am not sure, but I am wondering if we shouldn't save the previous uid
of 'stmt' here before setting it, and then restore it before getting out
of this function.

[...]

 +}
 +
  /* Try to optimize away redundant UBSAN_NULL checks.
 
 We walk blocks in the CFG via a depth first search of the dominator
 @@ -89,111 +402,77 @@ sanopt_optimize_walker (basic_block bb,

I think the comment of this sanopt_optimize_walker function should now
be adapted to say that it optimizes away redundant UBSAN_NULL *and*
ASAN_CHECK internal function calls.

  {

[...]

  }

[...]

OK with those changes.

Thanks.

-- 
Dodji

Re: [PATCH] Optimize ASAN_CHECK checks

[Dodji Seketeli]

Jakub Jelinek ja...@redhat.com writes:

 I am not sure, but I am wondering if we shouldn't save the previous uid
 of 'stmt' here before setting it, and then restore it before getting out
 of this function.

 No, gimple uids are AFAIK undefined at the start of passes, passes that use
 them are supposed to initialize them before use (new statements created
 during the pass will get 0 there by default), and don't have to clean them
 up anyway at the end of pass.

Yeah, this is what I figured by grepping other passes, but I wasn't sure
:-)

Maybe I should follow up with a doc patch for the (otherwise very terse)
comment of gimple_set_uid and gimple_uid accessors.

Thanks.

-- 
Dodji

Add more comments to some gimple property accessors

[Dodji Seketeli]

Hello,

Following the discussion at 
https://gcc.gnu.org/ml/gcc-patches/2014-11/msg01755.html,
I am proposing this documentation patch to add more comments to some
gimple accessors for properties that are meant to be undefined at patch
boundaries.  I haven't spotted all the properties in that case, but
perfect being the enemy of good, I thought i'd send these for now, at
least.

OK for trunk?

Cheers,

gcc/ChangeLog:

* gimple.h (gimple_set_visited, gimple_visited_p)
(gimple_set_plf, gimple_plf, gimple_set_uid, gimple_uid): Add more
comments to these accessors.

Signed-off-by: Dodji Seketeli do...@redhat.com
---
 gcc/gimple.h | 57 +++--
 1 file changed, 51 insertions(+), 6 deletions(-)

diff --git a/gcc/gimple.h b/gcc/gimple.h
index c7aaa81..27bb7b6 100644
--- a/gcc/gimple.h
+++ b/gcc/gimple.h
@@ -1585,7 +1585,17 @@ gimple_set_no_warning (gimple stmt, bool no_warning)
   stmt-no_warning = (unsigned) no_warning;
 }
 
-/* Set the visited status on statement STMT to VISITED_P.  */
+/* Set the visited status on statement STMT to VISITED_P.
+
+   Please note that this 'visited' property of the gimple statement is
+   supposed to be undefined at pass boundaries.  This means that a
+   given pass should not assume it contains any useful value when the
+   pass starts and thus can set it to any value it sees fit.
+
+   You can learn more about the visited property of the gimple
+   statement by reading the comments of the 'visited' data member of
+   struct gimple statement_base.
+ */
 
 static inline void
 gimple_set_visited (gimple stmt, bool visited_p)
@@ -1594,7 +1604,16 @@ gimple_set_visited (gimple stmt, bool visited_p)
 }
 
 
-/* Return the visited status for statement STMT.  */
+/* Return the visited status for statement STMT.
+
+   Please note that this 'visited' property of the gimple statement is
+   supposed to be undefined at pass boundaries.  This means that a
+   given pass should not assume it contains any useful value when the
+   pass starts and thus can set it to any value it sees fit.
+
+   You can learn more about the visited property of the gimple
+   statement by reading the comments of the 'visited' data member of
+   struct gimple statement_base.  */
 
 static inline bool
 gimple_visited_p (gimple stmt)
@@ -1603,7 +1622,15 @@ gimple_visited_p (gimple stmt)
 }
 
 
-/* Set pass local flag PLF on statement STMT to VAL_P.  */
+/* Set pass local flag PLF on statement STMT to VAL_P.
+
+   Please note that this PLF property of the gimple statement is
+   supposed to be undefined at pass boundaries.  This means that a
+   given pass should not assume it contains any useful value when the
+   pass starts and thus can set it to any value it sees fit.
+
+   You can learn more about the PLF property by reading the comment of
+   the 'plf' data member of struct gimple_statement_structure.  */
 
 static inline void
 gimple_set_plf (gimple stmt, enum plf_mask plf, bool val_p)
@@ -1615,7 +1642,15 @@ gimple_set_plf (gimple stmt, enum plf_mask plf, bool 
val_p)
 }
 
 
-/* Return the value of pass local flag PLF on statement STMT.  */
+/* Return the value of pass local flag PLF on statement STMT.
+
+   Please note that this 'plf' property of the gimple statement is
+   supposed to be undefined at pass boundaries.  This means that a
+   given pass should not assume it contains any useful value when the
+   pass starts and thus can set it to any value it sees fit.
+
+   You can learn more about the plf property by reading the comment of
+   the 'plf' data member of struct gimple_statement_structure.  */
 
 static inline unsigned int
 gimple_plf (gimple stmt, enum plf_mask plf)
@@ -1624,7 +1659,12 @@ gimple_plf (gimple stmt, enum plf_mask plf)
 }
 
 
-/* Set the UID of statement.  */
+/* Set the UID of statement.
+
+   Please note that this UID property is supposed to be undefined at
+   pass boundaries.  This means that a given pass should not assume it
+   contains any useful value when the pass starts and thus can set it
+   to any value it sees fit.  */
 
 static inline void
 gimple_set_uid (gimple g, unsigned uid)
@@ -1633,7 +1673,12 @@ gimple_set_uid (gimple g, unsigned uid)
 }
 
 
-/* Return the UID of statement.  */
+/* Return the UID of statement.
+
+   Please note that this UID property is supposed to be undefined at
+   pass boundaries.  This means that a given pass should not assume it
+   contains any useful value when the pass starts and thus can set it
+   to any value it sees fit.  */
 
 static inline unsigned
 gimple_uid (const_gimple g)
-- 

Dodji

Re: [PR c/52952] More precise locations within format strings

[Dodji Seketeli]

Joseph Myers jos...@codesourcery.com writes:

[...]

 Neither Per nor Tom are active in GCC anymore. If the FE maintainers
 do not feel comfortable reviewing line-map changes, could you nominate
 Dodji as line-map maintainer if he is willing to accept it? I think he
 is currently the person that understands that code best.

 I think Dodji as diagnostics maintainer is better placed than I am to 
 review line-map patches.

Thanks Joseph,

As Manuel said at:

Manuel López-Ibáñez lopeziba...@gmail.com writes:

[...]

 Then, do you mean that line-map falls under the reach of the
 diagnostics maintainer? I agree, but Dodji himself does not seem to be
 sure about this:

 https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02444.html

I have already reviewed that line-map patch.  But as I thought Tom, the
FE maintainers and the global maintainers were the qualified person to
actually approve the patch, I didn't want to act beyond my prerogatives.

Jeff Law l...@redhat.com writes:

 If the front-end maintainers think Dodji is the best person to handle
 this stuff, and Dodji is willing, then I think we should make add the
 linemap stuff to Dodji's plate.

Hey Jeff,

FWIW, I am willing to help with the line map sub-system, of course.
Thank you for your confidence.

Cheers,

-- 
Dodji

Re: [PATCH diagnostics] PR 53061 cleanup initialization

[Dodji Seketeli]

Hello Manuel,

Manuel López-Ibáñez lopeziba...@gmail.com writes:

 This is an old patch of mine that never got finished. I updated it following
 the suggestions of Gabriel here
 https://gcc.gnu.org/ml/gcc-patches/2012-04/msg00443.html

Thanks for looking at this again.

 Bootstrapped and tested on x86_64-linux-gnu.

 OK?

I think the patch is good.  I only have minor observations regarding
comments and one function naming.

 Index: gcc/doc/invoke.texi
 ===
 --- gcc/doc/invoke.texi   (revision 215890)
 +++ gcc/doc/invoke.texi   (working copy)
 @@ -3075,15 +3075,14 @@ information should be reported.  Note th
  honor these options.
  
  @table @gcctabopt
  @item -fmessage-length=@var{n}
  @opindex fmessage-length
 -Try to format error messages so that they fit on lines of about @var{n}
 -characters.  The default is 72 characters for @command{g++} and 0 for the 
 rest of
 -the front ends supported by GCC@.  If @var{n} is zero, then no
 -line-wrapping is done; each error message appears on a single
 -line.
 +Try to format error messages so that they fit on lines of about
 +@var{n} characters.  If @var{n} is zero, then no line-wrapping will be
 +done; each error message will appear on a single line.  This is the
 +default for all front ends.

Agreed.

  @item -fdiagnostics-show-location=once
  @opindex fdiagnostics-show-location
  Only meaningful in line-wrapping mode.  Instructs the diagnostic messages
  reporter to emit source location information @emph{once}; that is, in
 Index: gcc/c-family/c-opts.c
 ===
 --- gcc/c-family/c-opts.c (revision 215890)
 +++ gcc/c-family/c-opts.c (working copy)
 @@ -176,25 +176,14 @@ c_diagnostic_finalizer (diagnostic_conte
virt_loc_aware_diagnostic_finalizer (context, diagnostic);
pp_destroy_prefix (context-printer);
pp_newline_and_flush (context-printer);
  }
  
 -/* Common diagnostics initialization.  */
 +/* Common default settings for diagnostics.  */
  void
 -c_common_initialize_diagnostics (diagnostic_context *context)
 +c_common_diagnostics_defaults (diagnostic_context *context)

Please, call this c_common_diagnostics_set_defaults().

Having a verb in the function name (at least for functions that are not
accessors) makes it easier to read, I think.

  {
 -  /* This is conditionalized only because that is the way the front
 - ends used to do it.  Maybe this should be unconditional?  */
 -  if (c_dialect_cxx ())
 -{
 -  /* By default wrap lines at 80 characters.  Is getenv
 -  (COLUMNS) preferable?  */
 -  diagnostic_line_cutoff (context) = 80;
 -  /* By default, emit location information once for every
 -  diagnostic message.  */
 -  diagnostic_prefixing_rule (context) = DIAGNOSTICS_SHOW_PREFIX_ONCE;
 -}
diagnostic_finalizer (context) = c_diagnostic_finalizer;
context-opt_permissive = OPT_fpermissive;
  }

OK.

  /* Whether options from all C-family languages should be accepted
 Index: gcc/c-family/c-common.h
 ===
 --- gcc/c-family/c-common.h   (revision 215890)
 +++ gcc/c-family/c-common.h   (working copy)
 @@ -824,11 +824,11 @@ extern void set_compound_literal_name (t
  
  extern tree build_va_arg (location_t, tree, tree);
  
  extern const unsigned int c_family_lang_mask;
  extern unsigned int c_common_option_lang_mask (void);
 -extern void c_common_initialize_diagnostics (diagnostic_context *);
 +extern void c_common_diagnostics_defaults (diagnostic_context *);

c_common_diagnostics_defaults - c_common_diagnostics_set_defaults.

  extern bool c_common_complain_wrong_lang_p (const struct cl_option *);
  extern void c_common_init_options_struct (struct gcc_options *);
  extern void c_common_init_options (unsigned int, struct cl_decoded_option *);
  extern bool c_common_post_options (const char **);
  extern bool c_common_init (void);
 Index: gcc/c/c-objc-common.c
 ===
 --- gcc/c/c-objc-common.c (revision 215890)
 +++ gcc/c/c-objc-common.c (working copy)
 @@ -60,19 +60,11 @@ c_warn_unused_global_decl (const_tree de
  bool
  c_objc_common_init (void)
  {
c_init_decl_processing ();
  
 -  if (c_common_init () == false)
 -return false;
 -
 -  /* These were not defined in the Objective-C front end, but I'm
 - putting them here anyway.  The diagnostic format decoder might
 - want an enhanced ObjC implementation.  */
 -  diagnostic_format_decoder (global_dc) = c_tree_printer;
 -
 -  return true;
 +  return c_common_init ();
  }

OK.

  
  /* Called during diagnostic message formatting process to print a
 source-level entity onto BUFFER.  The meaning of the format specifiers
 is as follows:
 @@ -184,19 +176,20 @@ has_c_linkage (const_tree decl ATTRIBUTE
  }
  
  void
  c_initialize_diagnostics (diagnostic_context *context)
 

Re: C/C++ diagnostics guidelines

[Dodji Seketeli]

Manuel López-Ibáñez lopeziba...@gmail.com writes:

 On 17 October 2014 19:33, Joseph S. Myers jos...@codesourcery.com wrote:
 On Fri, 17 Oct 2014, Manuel López-Ibáñez wrote:

 Thus, I drafted some guidelines
 at:https://gcc.gnu.org/wiki/Better_Diagnostics#guidelines

 Please, could you take a look and comment whether I got it right/wrong?

 Yes, that looks right to me.

 Thanks!

 I added guidelines also about locations and warning options.

I like these very much.  Thank you for looking into this.

To take this further, I am thinking that these guidelines would be even
better served by standing on their own page.  If nobody objects, I can
create a  DiagnosticsGuidelines page in the wiki with the content that you 
added.

Cheers,

-- 
Dodji

Re: C/C++ diagnostics guidelines

[Dodji Seketeli]

Manuel López-Ibáñez lopeziba...@gmail.com writes:

 On 17 October 2014 20:04, Manuel López-Ibáñez lopeziba...@gmail.com wrote:
 On 17 October 2014 19:33, Joseph S. Myers jos...@codesourcery.com wrote:
 On Fri, 17 Oct 2014, Manuel López-Ibáñez wrote:

 Thus, I drafted some guidelines
 at:https://gcc.gnu.org/wiki/Better_Diagnostics#guidelines

 Please, could you take a look and comment whether I got it right/wrong?

 Yes, that looks right to me.

 Thanks!

 I added guidelines also about locations and warning options.

 I believe there are also some rules about when to use some special
 line-map functions that arise when warning about macros like NULL, but
 I am not aware of the specifics. It would be useful if someone added
 those.

 Dodji, Paolo? Do you know what I'm talking about?

Hmmh, I am not sure.  Do you have any example of warning about such
macros?

Cheers,

-- 
Dodji

Re: [PATCH diagnostics/fortran] dynamically generate locations from offset + handle %C

[Dodji Seketeli]

Hello Manuel,

Manuel López-Ibáñez lopeziba...@gmail.com writes:


 Dodji, are the linemap_asserts() appropriate?

Yes they are.  I have some additional comments though.

 libcpp/ChangeLog:

 2014-10-16  Manuel López-Ibáñez  m...@gcc.gnu.org

 PR fortran/44054
 * include/line-map.h (linemap_position_for_loc_and_offset):
 Declare.
 * line-map.c (linemap_position_for_loc_and_offset): New.
[...]

 --- libcpp/include/line-map.h (revision 216257)
 +++ libcpp/include/line-map.h (working copy)
 @@ -601,10 +601,17 @@ linemap_position_for_column (struct line
 column.  */
  source_location
  linemap_position_for_line_and_column (const struct line_map *,
 linenum_type, unsigned int);
  
 +/* Encode and return a source_location starting from location LOC
 +   and shifting it by OFFSET columns.  */
 +source_location
 +linemap_position_for_loc_and_offset (struct line_maps *set,
 +  source_location loc,
 +  unsigned int offset);
 +

OK.

[...]

 --- libcpp/line-map.c (revision 216257)
 +++ libcpp/line-map.c (working copy)

[...]

 +/* Encode and return a source_location starting from location LOC
 +   and shifting it by OFFSET columns.  */
 +

The comment is OK.  I would just add that this function currently only
works with non-virtual locations.

 +source_location
 +linemap_position_for_loc_and_offset (struct line_maps *set,
 +  source_location loc,
 +  unsigned int offset)
 +{
 +  const struct line_map * map = NULL;
 +
 +  /* This function does not support virtual locations yet.  */
 +  linemap_assert (!linemap_location_from_macro_expansion_p (set, loc));
 +
 +  if (offset == 0)
 +return loc;

Here, I'd replace the above condition and return status statement with:

if (offset == 0
/* Adding an offset to a reserved location (like
   UNKNOWN_LOCATION for the C/C++ FEs) does not really make
   sense.  So let's live the location intact in that case.  */
|| loc  RESERVED_LOCATION)
  return loc;

 +
 +  /* First, we find the real location and shift it.  */
 +  loc = linemap_resolve_location (set, loc, LRK_SPELLING_LOCATION, map);
 +  linemap_assert (MAP_START_LOCATION (map)  loc + offset);

OK.

First I'd add a comment above the assert that says:

/* The new location (loc + offset) should be higher than the first
   location encoded by MAP.  */

and I'd add another assert:

/* If MAP is not the last line map of its set, then the new location
   (loc + offset) should be less than the first location encoded by
   the next line map of the set.  */
if (map  LINEMAPS_LAST_ORDINARY_MAP(set))
  linemap_assert(MAP_START_LOCATION(map[1])  loc + offset);

 +
 +  offset += SOURCE_COLUMN (map, loc);
 +  linemap_assert (offset  (1u  map-d.ordinary.column_bits));
 +
 +  source_location r = 
 +linemap_position_for_line_and_column (map,
 +   SOURCE_LINE (map, loc),
 +   offset);
 +  linemap_assert (map == linemap_lookup (set, r));
 +  return r;
 +}
 +

OK.

So the line map part of the patch is OK from me if it passes bootstrap
with the added asserts.

Thank you for looking into this.

Cheers.

-- 
Dodji

Re: [PATCH diagnostics/fortran] dynamically generate locations from offset + handle %C

[Dodji Seketeli]

Sorry, I forgot to make it clear that I have no power to approve the
line-map changes.  I just gave my casual point view; so please take it
for what it is worth.

I am CC-ing Tom and Jason who can approve this.

Cheers.

Dodji Seketeli do...@redhat.com writes:

 Hello Manuel,

 Manuel López-Ibáñez lopeziba...@gmail.com writes:


 Dodji, are the linemap_asserts() appropriate?

 Yes they are.  I have some additional comments though.

 libcpp/ChangeLog:

 2014-10-16  Manuel López-Ibáñez  m...@gcc.gnu.org

 PR fortran/44054
 * include/line-map.h (linemap_position_for_loc_and_offset):
 Declare.
 * line-map.c (linemap_position_for_loc_and_offset): New.
 [...]

 --- libcpp/include/line-map.h(revision 216257)
 +++ libcpp/include/line-map.h(working copy)
 @@ -601,10 +601,17 @@ linemap_position_for_column (struct line
 column.  */
  source_location
  linemap_position_for_line_and_column (const struct line_map *,
linenum_type, unsigned int);
  
 +/* Encode and return a source_location starting from location LOC
 +   and shifting it by OFFSET columns.  */
 +source_location
 +linemap_position_for_loc_and_offset (struct line_maps *set,
 + source_location loc,
 + unsigned int offset);
 +

 OK.

 [...]

 --- libcpp/line-map.c(revision 216257)
 +++ libcpp/line-map.c(working copy)

 [...]

 +/* Encode and return a source_location starting from location LOC
 +   and shifting it by OFFSET columns.  */
 +

 The comment is OK.  I would just add that this function currently only
 works with non-virtual locations.

 +source_location
 +linemap_position_for_loc_and_offset (struct line_maps *set,
 + source_location loc,
 + unsigned int offset)
 +{
 +  const struct line_map * map = NULL;
 +
 +  /* This function does not support virtual locations yet.  */
 +  linemap_assert (!linemap_location_from_macro_expansion_p (set, loc));
 +
 +  if (offset == 0)
 +return loc;

 Here, I'd replace the above condition and return status statement with:

 if (offset == 0
 /* Adding an offset to a reserved location (like
UNKNOWN_LOCATION for the C/C++ FEs) does not really make
sense.  So let's live the location intact in that case.  */
 || loc  RESERVED_LOCATION)
   return loc;

 +
 +  /* First, we find the real location and shift it.  */
 +  loc = linemap_resolve_location (set, loc, LRK_SPELLING_LOCATION, map);
 +  linemap_assert (MAP_START_LOCATION (map)  loc + offset);

 OK.

 First I'd add a comment above the assert that says:

 /* The new location (loc + offset) should be higher than the first
location encoded by MAP.  */

 and I'd add another assert:

 /* If MAP is not the last line map of its set, then the new location
(loc + offset) should be less than the first location encoded by
the next line map of the set.  */
 if (map  LINEMAPS_LAST_ORDINARY_MAP(set))
   linemap_assert(MAP_START_LOCATION(map[1])  loc + offset);

 +
 +  offset += SOURCE_COLUMN (map, loc);
 +  linemap_assert (offset  (1u  map-d.ordinary.column_bits));
 +
 +  source_location r = 
 +linemap_position_for_line_and_column (map,
 +  SOURCE_LINE (map, loc),
 +  offset);
 +  linemap_assert (map == linemap_lookup (set, r));
 +  return r;
 +}
 +

 OK.

 So the line map part of the patch is OK from me if it passes bootstrap
 with the added asserts.

 Thank you for looking into this.

 Cheers.

-- 
Dodji

Re: [PATCH] cleanups in line-map

[Dodji Seketeli]

Manuel López-Ibáñez lopeziba...@gmail.com writes:

 A few cleanups in line-map code. Bootstrapped and regression tested on
 x86_64-linux-gnu.

Thanks for doing this.

 OK?

Yes, barring this little nit:

[...]

 Index: libcpp/line-map.c
 ===
 --- libcpp/line-map.c (revision 216098)
 +++ libcpp/line-map.c (working copy)
 @@ -29,12 +29,10 @@ along with this program; see the file CO
  static void trace_include (const struct line_maps *, const struct line_map 
 *);
  static const struct line_map * linemap_ordinary_map_lookup (struct line_maps 
 *,
   source_location);
  static const struct line_map* linemap_macro_map_lookup (struct line_maps *,
   source_location);
 -static source_location linemap_macro_map_loc_to_def_point
 -(const struct line_map*, source_location);

This is not redundant per se, is it?  It's just a forward declaration of
the function that is defined later.  Just like for
linemap_macro_map_loc_unwind_toward_spelling() below.  Or what am I
missing?  I'd prefer to see this forward declaration stay, FWIW.

Otherwise, this cleanup patch looks good to me.  If it was my call, I'd
say OK with that change.

Thank you for tackling this.

-- 
Dodji

Re: [PATCH 2/2] PR debug/63240 Add DWARF representation for C++11 defaulted member function.

[Dodji Seketeli]

Jason Merrill ja...@redhat.com writes:

 On 10/06/2014 08:50 PM, Siva Chandra wrote:
 On Sat, Oct 4, 2014 at 11:14 AM, Jason Merrill ja...@redhat.com wrote:
 On 10/03/2014 05:41 PM, Siva Chandra wrote:

 I understand that knowing whether a copy-ctor or a d-tor has been
 explicitly defaulted is not sufficient to determine the parameter
 passing ABI. However, why is it not necessary? I could be wrong, but
 doesn't the example I have given show that it is necessary?

 An implicitly declared 'tor can also be trivial.

 But, the question is whether it is required to determine the parameter
 passing ABI. If there is no special marker to indicate that the user
 declared 'tor is explicitly defaulted, then GDB could (in the absence
 of other properties which make the 'tor non-trivial) incorrectly
 conclude that the the 'tor is user defined, and hence not-trivial.

 I've been thinking that we should just mark the 'tor as trivial or not
 directly rather than hint at it.

FWIW, this would be my inclination too.  I think it would make the job
of the debug info consumer a lot easier.

Thanks.

-- 
Dodji

Re: [PATCH] Provide global var location info for asan

[Dodji Seketeli]

Hello,

Jakub Jelinek ja...@redhat.com writes:

 2014-09-24  Jakub Jelinek  ja...@redhat.com

   * ubsan.h (ubsan_get_source_location): New prototype.
   * ubsan.c (ubsan_source_location_type): New variable.
   Function renamed to ...
   (ubsan_get_source_location_type): ... this.  Cache
   return value in ubsan_source_location_type variable.
   (ubsan_source_location, ubsan_create_data): Use
   ubsan_get_source_location_type instead of
   ubsan_source_location_type.
   * asan.c (asan_protect_global): Don't protect globals
   with ubsan_get_source_location_type () type.
   (asan_add_global): Provide global decl location info
   if possible.

This is OK, thanks.

-- 
Dodji

Re: [RFC/PATCH] Fix-it hints

[Dodji Seketeli]

Hello Manuel,

Sorry for taking so long to reply to this.

FWIW, I like the direction of this.  I find fix-it hints cool in
general.  So thank you for working on this.

Manuel López-Ibáñez lopeziba...@gmail.com a écrit:

 This patch implements fix-it hints. See https://gcc.gnu.org/PR62314

 When the caret line is active (which is the default), this adds an
 additional source-line indicating how to fix the code:

 gcc/testsuite/g++.dg/template/crash83.C:5:21: error: an explicit
 specialization must be preceded by 'template '
  templatetypename = class A0:  struct B {}; // { dg-error
 explicit specialization|expected }
  ^
  template

It looks like your mail user agent wrapped a line above, making it hard
to read.  I suspect it should have been:

  templatetypename = class A0  struct B {}; // { dg-error explicit 
specialization|expected }
  ^
  template

 When the caret line is disabled with -fno-diagnostics-show-caret, the
 fix-it hint is printed as:

 gcc/testsuite/g++.dg/template/crash83.C:5:21: error: an explicit
 specialization must be preceded by 'template '
 gcc/testsuite/g++.dg/template/crash83.C:5:21: fixit: template

 The latter form may allow an IDE (such as emacs) to automatically
 apply the fix.

Nice.  Is the fixit: prefix used by other compilers too?  Or are there
variations from compiler to compiler?

 Currently, fix-it hints are limited to insertions at one single
 location, whereas Clang allows insertions, deletions, and replacements
 at arbitrary location ranges.

Do you have example of each of these kinds of fix-it hints? (deletions,
replacement at location ranges).  I think it'd be nice to have an idea
of what needs to be done, even if we are not doing it in extenso right
now.

 Opinions? Is the proposed interface/implementation acceptable?

Please read my comments below.

 Any other diagnostics that could use a fix-it hint? In principle, we
 should only give them when we are sure that the proposed fix will fix
 the error or silence a warning.  For example, the C++ parser often
 says 'x' expected before 'y' but adding x before y rarely fixes
 anything.

I am thinking that maybe the diagnostic about the missing ; after a
struct/class declaration might be a candidate for this fix-it hint
feature.

It's emitted by cp_parser_class_specifier_1() at:

if (CLASSTYPE_DECLARED_CLASS (type))
  error_at (loc, expected %;% after class definition);
else if (TREE_CODE (type) == RECORD_TYPE)
  error_at (loc, expected %;% after struct definition);
else if (TREE_CODE (type) == UNION_TYPE)
  error_at (loc, expected %;% after union definition);
else
  gcc_unreachable ();


[...]

 +
 +static int
 +adjust_column (int line_width, int max_width, int column)

Missing comments for this function.

[...]

 +static const char *
 +get_source_line_and_column (location_t loc, int *line_width, int *column)
 +{

Likewise.

[...]


  /* Print the physical source line corresponding to the location of
 this diagnostic, and a caret indicating the precise column.  */
  void
  diagnostic_show_locus (diagnostic_context * context,
  const diagnostic_info *diagnostic)
  {

[...]

context-last_location = diagnostic-location;
 -  s = expand_location_to_spelling_point (diagnostic-location);
 -  line = location_get_source_line (s, line_width);
 -  if (line == NULL || s.column  line_width)
 +  line = get_source_line_and_column (diagnostic-location,
 +  line_width, column);
 +  if (line == NULL)
  return;
  
max_width = context-caret_max_width;
 -  line = adjust_line (line, line_width, max_width, (s.column));
 +  line = adjust_line (line, line_width, max_width, column);

Apparently, each time we call get_source_line_and_column, we also call
adjust_line on it.  So maybe we want to have a
get_adjusted_source_line_and_column (or something like that) that does
it all?

[...]

 @@ -325,13 +345,13 @@ diagnostic_show_locus (diagnostic_contex
pp_newline (context-printer);
caret_cs = colorize_start (pp_show_color (context-printer), caret);
caret_ce = colorize_stop (pp_show_color (context-printer));
  
/* pp_printf does not implement %*c.  */
 -  size_t len = s.column + 3 + strlen (caret_cs) + strlen (caret_ce);
 +  size_t len = column + 3 + strlen (caret_cs) + strlen (caret_ce);
buffer = XALLOCAVEC (char, len);
 -  snprintf (buffer, len, %s %*c%s, caret_cs, s.column, context-caret_char,
 +  snprintf (buffer, len, %s %*c%s, caret_cs, column, context-caret_char,
   caret_ce);

Maybe you should factorize out the printing of a colored line starting
at given a column, rather than copy-pasting this in fixit_hint() later?

[...]

diagnostic_set_info (diagnostic, gmsgid, ap, location, DK_NOTE);
report_diagnostic (diagnostic);
va_end (ap);
  }
  
 +/* A fix-it hint at LOCATION.  Use this recommended 

Re: [RFC/PATCH] More precise diagnostic locations: dynamic locations for columns vs explicit offset

[Dodji Seketeli]

Manuel López-Ibáñez lopeziba...@gmail.com a écrit:

 In some situations, we would like to point to a location which was not
 encoded when tokenizing. This happens, for example, in two prominent
 cases:

 1) To get precise locations within strings
 (https://gcc.gnu.org/PR52952) for example, for Wformat warnings.

This feature would be very welcome indeed.


 2) In the Fortran FE, which gives quite precise location information
 by tracking the characters that it wants to warn about instead of
 relying on the line-map machinery.

So with this feature, the Fortran FE would then use the then more
generic diagnostics machinery, right?

 The most straightforward way to implement this is by adding variants
 of diagnostic functions that take an explicit offset argument and
 pass this offset through the whole diagnostics machinery. This is what
 I implemented in the patch format_offset.diff attached. The downside
 is that we would need to add even more variants (with/without offset)
 of various diagnostic functions and track the offset/no-offset cases
 explicitly.

I would be inclined to go for this route at first sight because of its
conceptual simplicity, even if it might be heavy in terms of the
number of entry points to maintain for users of the diagnostics
sub-system but then ...

 The nicer/cleaner alternative is to somehow (re)compute a single
 location value from a given location plus the new offset.

... I agree with this.  It's more elegant and maintainable to go this
way.  But it might involve some hair splitting.


 This is what I implemented in patch fortran-diagnostics-part3.diff
 in linemap_redo_position_for_column(). As far as I understand, this
 method only works reliably if the location+offset does not jump to a
 different line map, that is, if to_column  (1u 
 map-d.ordinary.column_bits). Otherwise, we may need to recompute
 all successive line-maps to accommodate the new location. The best
 way to do the latter (or to work-around that issue) is not clear to
 me at the moment.

I think it might be more involved than that.

There are two kinds of locations:

 1/ spelling locations.  They represent a real point in the source
 code.  For now, the beginning of a token.

 2/ virtual locations.  They are an abstract number, calculated in a
 convoluted way to encode the fact that a given token (rather, the
 location of that token) was e.g, possibly passed to a function-like
 macro that used it in its expansion, and that macro was expanded
 somewhere.  And from that number, we can get back to the macro into
 which it was used, expanded, where the macro was expanded and we can
 also get the original spelling location of the token we are looking
 at.

I might be maybe missing something, but if the location is not virtual
(case 1/), I *think* that in practice we are not likely to see that
location + column jumps to the next map, unless we are running low
on line maps space -- in which case, either columns tracking or even
line maps are turned off -- or the token we are looking at it
is *huge*.  In the later case, when we start tracking the location of
the *end* of tokens (as said in the roadmap), I think that later issue
is going to vanish because a given line map is going to be allocated big
enough to contain locations until at least the end of the last token
it contains.

If the location is virtual (case 2/), then the location + offset
value you are referring to is meaningless, unfortunately.  You must
get back to to the spelling location of that token first; that is, you
have to consider spelling_location(location) + offset, and we are
back to the first case (case 1/).


 Thus, I am putting forward these two alternative implementations and
 seeking comments/advice/help in deciding what would be the best way to
 fix this key missing piece of GCC diagnostics.

Thanks.

 Related to this, perhaps I should make a more general call for help.
 Despite the heroic, constant torrent of diagnostic fixes by Paolo,
 Marek and others, I have not seen much progress on the key
 infrastructure issues in the roadmap
 (https://gcc.gnu.org/wiki/Better_Diagnostics). We have had at least
 one major item per release since GCC 4.5, but I don't see any
 particular item being tackled for GCC 5.0. Are you planning to tackle
 any of them?

Unfortunately, it's unlikely that I'll have time to tackle any of
this.  I am quite busy on libabigail
(http://https://sourceware.org/libabigail/) in this cycle.  And it's
also important for us.  So I'd rather shoot for the next cycle.

But that shouldn't prevent interested hackers to jump in :-)

 I have a simple patch to implement Fix-it hints but it needs more
 work. Unfortunately, I have very little free time to dedicate to GCC
 nowadays, so I'm afraid I might not even be able to finish this in
 time. Any item in that list would be a nice major feature for GCC
 5.0.

Thank you for the effort you are putting in this, despite your tight
schedule.  This is really appreciated.

 Perhaps we need 

Re: [PATCH diagnostics/Fortran] Implement Fortran prefix/caret style using the common diagnostics machinery

[Dodji Seketeli]

Hello Manuel,

Manuel López-Ibáñez lopeziba...@gmail.com writes:

 Hi,

 This patch is relative to this one here:
 https://gcc.gnu.org/ml/gcc-patches/2014-08/msg01652.html

 It implements the Fortran style of prefix and caret line in the
 gfc_diagnostic_starter by using the common pretty-printer.

 This requires making configurable the caret character in the
 diagnostics machinery.

[...]


 2014-08-19  Manuel López-Ibáñez  m...@gcc.gnu.org

 PR fortran/44054
 * diagnostic.c: Set default caret.
 (diagnostic_show_locus): Use it. Tell pretty-printer that a new
 line is needed.
 * diagnostic.h (struct diagnostic_context):

 Bootstrapped and regression tested on x86_64-linux-gnu.

 OK?

The generic diagnostics part is OK.

Thanks!

Cheers.

-- 
Dodji

Re: [PATCH/PR c/59304] #pragma diagnostic pop after warning fails for options unspecified in the command-line and disabled by default

[Dodji Seketeli]

Hello Manuel,

Manuel López-Ibáñez lopeziba...@gmail.com writes:

 The idea is that when we see a change of classification, and the
 option was originally unspecified, we record the command-line status.
 This way, when doing pop later, we already check if the option was
 reclassified so we get the command-line status. This also works with
 -Wall, since all dependent options go through set_option and call
 diagnostic_classify_diagnostic. But we have to call it before changing
 the option status.

 Bootstrapped and regression tested on x86_64-linux-gnu.

 OK?

Yes, this looks OK to me.


 gcc/ChangeLog:

 2014-08-19  Manuel López-Ibáñez  m...@gcc.gnu.org

 PR c/59304
 * opts-common.c (set_option): Call diagnostic_classify_diagnostic
 before setting the option.
 * diagnostic.c (diagnostic_classify_diagnostic): Record
 command-line status.

 gcc/testsuite/ChangeLog:

 2014-08-19  Manuel López-Ibáñez  m...@gcc.gnu.org

 PR c/59304
 * gcc.dg/pr59304.c: New test.

Thank you!


-- 
Dodji

Re: [PATCH] Move caret printing to diagnostics_finalizer

[Dodji Seketeli]

Manuel López-Ibáñez lopeziba...@gmail.com writes:

 This patch is in preparation for further patches moving the Fortran FE
 to use the common diagnostics machinery.

 Fortran has its own way of printing the caret information, so we need
 a way to override the default in the diagnostics machinery. A simple
 way is to move the printing of the caret (plus the destruction of the
 prefix and printing a newline at the end)  to the
 diagnostic_finalizer, so Fortran can override the whole thing with its
 own finalizer.

 This means that the c-family finalizer needs to invoke the caret
 explicitly. The C++ finalizer was destroying the prefix even thought
 this was done by the common code anyway. Thus now there is only one
 finalizer for both C and C++.


 gcc/ChangeLog:

 2014-08-16  Manuel López-Ibáñez  manu.gnu.org

 * diagnostic.c (default_diagnostic_finalizer): Move caret printing
  to here ...
 (diagnostic_report_diagnostic): ... from here.
 * toplev.c (general_init): Move code to c-family.

 gcc/cp/ChangeLog:

 2014-08-16  Manuel López-Ibáñez  manu.gnu.org

 * error.c (cp_diagnostic_finalizer): Delete.
 (init_error): Do not set diagnostic_finalizer here.

 gcc/c-family/ChangeLog:

 2014-08-16  Manuel López-Ibáñez  manu.gnu.org

 * c-opts.c: Include tree-diagnostics.h.
 (c_diagnostic_finalizer): New.
 (c_common_initialize_diagnostics): Use it.

 Bootstrapped and regression tested on x86-64-linux.

 OK?

This is OK for me.

Please commit it if nobody objects in the next 48 hours.

Thank you :-)

Cheers.

-- 
Dodji

Re: [PATCH] Handle -fsanitize=leak more similarly to address/thread

[Dodji Seketeli]

Jakub Jelinek ja...@redhat.com writes:


 Right now when -fsanitize=leak adds -llsan, it adds it late on the command
 line, so e.g. -lstdc++ comes after it, which seems to be bad.
 The following patch puts it early on the link command line like we do for
 -lasan or -ltsan.  Bootstrapped/regtested on x86_64-linux and i686-linux,
 ok for trunk?

 2014-08-14  Jakub Jelinek  ja...@redhat.com

   * config/gnu-user.h (LIBLSAN_EARLY_SPEC): Define.
   * gcc.c (LIBLSAN_SPEC, LIBLSAN_EARLY_SPEC): Follow LIBTSAN*_SPEC.
   (SANITIZER_EARLY_SPEC): Include LIBLSAN_EARLY_SPEC for -fsanitize=leak.

This looks OK to me.

Thanks!

-- 
Dodji

Re: [PATCH] Fix UB in diagnostic.c

[Dodji Seketeli]

Marek Polacek pola...@redhat.com a écrit:

 On Wed, Aug 13, 2014 at 09:03:37PM +0200, Manuel López-Ibáñez wrote:
 I don't think this is the right fix. The problem is that we are trying
 to print the caret in a column that is larger than the line_width. We
 do this because the file given by the line directive has nothing to do
 with the actual code we are parsing. I think in that case it is ok to
 just give up and not print a caret. So something like:
 
 @@ -300,11 +299,11 @@ diagnostic_show_locus (diagnostic_contex
  return;
 
context-last_location = diagnostic-location;
s = expand_location_to_spelling_point (diagnostic-location);
line = location_get_source_line (s, line_width);
 -  if (line == NULL)
 +  if (line == NULL || s.column  line_width)
  return;
 
max_width = context-caret_max_width;
line = adjust_line (line, line_width, max_width, (s.column));
 
 Nonetheless, perhaps in addition adjust_line should have
 gcc_checking_assert(line_width = column).

 Yea, I think this would be much better, thanks.  Done in the
 following.

Yes, I agree with this as well.

 Bootstrapped/regtested on x86_64-linux, ok for trunk?

Yes, this is OK to me.

Thank you for looking into this.

Cheers.

-- 
Dodji

Re: [C PATCH] Diagnose predefined identifiers in pedantic mode

[Dodji Seketeli]

Marek Polacek pola...@redhat.com a écrit:


 diff --git gcc/testsuite/gcc.dg/concat.c gcc/testsuite/gcc.dg/concat.c
 index 0b9d6f6..e3bfd46 100644
 --- gcc/testsuite/gcc.dg/concat.c
 +++ gcc/testsuite/gcc.dg/concat.c
 @@ -1,6 +1,7 @@
  /* Copyright (C) 2001 Free Software Foundation, Inc.  */
  
  /* { dg-do compile } */
 +/* { dg-options  } */

Just for my own education, why this change?

 diff --git gcc/testsuite/gcc.dg/pr22458-1.c gcc/testsuite/gcc.dg/pr22458-1.c
 index 8b8032c..023fb21 100644
 --- gcc/testsuite/gcc.dg/pr22458-1.c
 +++ gcc/testsuite/gcc.dg/pr22458-1.c
 @@ -1,4 +1,6 @@
  /* { dg-error expected declaration or statement  { target *-*-* } 0 } */
 +/* { dg-options  } */
 +

Likewise.

 diff --git gcc/testsuite/gcc.dg/pr33676.c gcc/testsuite/gcc.dg/pr33676.c
 index 79c830e..c234470 100644
 --- gcc/testsuite/gcc.dg/pr33676.c
 +++ gcc/testsuite/gcc.dg/pr33676.c
 @@ -1,4 +1,5 @@
  /* { dg-do run } */
 +/* { dg-options  } */
  /* { dg-options -O0 -mtune=i386 -fomit-frame-pointer { target { { i?86-*-* 
 x86_64-*-* }  ia32 } } } */

Likewise.

Cheers.

-- 
Dodji

Re: [C PATCH] Diagnose predefined identifiers in pedantic mode

[Dodji Seketeli]

Marek Polacek pola...@redhat.com a écrit:

 Thise testcases use predefined identifiers, and without the
 dg-options, they would compile with -ansi -pedantic-errors and fail.
 Setting dg-options to  makes the -ansi -pedantic-errors go away.
 Setting it to e.g. -std=gnu99 would work as well.

Oh, I see.  Thanks.

Maybe a comment there would be helpful then :-)

Thanks.

-- 
Dodji

Re: [PATCH] PR preprocessor/61817 - Fix location of tokens in built-in macro expansion list

[Dodji Seketeli]

Hello,

I have initially sent a patch for this at
https://gcc.gnu.org/ml/gcc-patches/2014-07/msg01144.html.  But then a
patch for a similarly expressed issue
https://gcc.gnu.org/ml/gcc-patches/2014-07/msg01521.html (which I think
is the same underlying problem) was committed.  As the PR
preprocessor/61817 is still not fixed, I have updated my initial patch
over the one that was committed already and am submitted the result
below.  I have added more test cases in the process.


Consider this function-like macro definition in the inc.h file

---inc.h---
#define F() const int line = __LINE__
---8---

Now consider its use in a file test.c:

--cat -n test.c
 1  #include inc.h
 2
 3  void
 4  foo()
 5  {
 6F
 7  (
 8   )
 9  ;
10  }
---8-

Running test.c through cc1 -quiet -E yields:

-8
# 1 test.c
# 1 built-in
# 1 command-line
# 1 /usr/include/stdc-predef.h 1 3 4
# 1 command-line 2
# 1 test.c
# 1 inc.h 1
# 2 test.c 2

void
foo()
{
  const int line =

 8
;
}
-8--

Note how tokens const, int, line and = are all on the same
line (line 6) as the expansion point of the function-like F() macro,
but how the token 8, resulting from the expansion of the built-in
macro __FILE__ is on the same line as the closing parenthesis of the
invocation of F().

This is the problem.  The result of the __LINE__ macro should be 6
and should be on the same line (line 6) as the other tokens of the
expansion-list of F().

This issue actually holds for the expansion of all built-in macros.

So more generelly, I would describe the issue as such:

When expanded in a function-like macro, the location of resulting
tokens of a built-in macro is set to the closing parenthesis of the
enclosing function-like macro invocation, rather than being set to the
location of the expansion point of the invocation the enclosing
functin-like macro.

I think the problem is that enter_macro_context() is passed the
location of the expansion point of the macro that is about to be
expanded.  But when that macro is built-in, builtin_macro() that is
then called by enter_macro_context() doesn't use the macro expansion
point location when expanding the __LINE__ builtin (in, e.g,
_cpp_builtin_macro_text) .  Rather, what it used is the location of
the closing parenthesis of the enclosing function-like macro
invocation or, more generally, the location of the previous token in
the stream.

The patch proposes to make builtin_macro() use the expansion point
location that enter_macro_context() passed along.  That location is
then used by the different routines that need it (e.g, to expand
builtin macros for instance).

Also, for the particular case of __LINE__, make that built-in expand
to the line number of that expansion point.

Note that in the function builtin_macro, this patch avoids re-setting
token-src_loc because for ease of maintenance (and debugging), I
think it's preferable that the spelling location of tokens be set only
in place -- in the tokens lexer.  To control the value of that
location we use the function cpp_force_token_locations, as intended.

The patch bootstraps and passes tests on x86_64-unknown-linux-gnu
against trunk.

libcpp/
* macro.c (_cpp_builtin_macro_text): Honor the
cpp_reader::forced_token_location_p data member to force the value
__LINE__ expands to accordlingly.
(builtin_macro): Use cpp_force_token_locations() to set the
location of the resulting token to that expansion point location.
(enter_macro_context): Pass the expansion point locatoin to
builtin_macro.

gcc/testsuite/
* gcc.dg/cpp/builtin-macro-{0,1,2,3}.c: New test files.

Signed-off-by: Dodji Seketeli do...@redhat.com
---
 gcc/testsuite/gcc.dg/cpp/builtin-macro-0.c | 18 ++
 gcc/testsuite/gcc.dg/cpp/builtin-macro-1.c | 30 
 gcc/testsuite/gcc.dg/cpp/builtin-macro-2.c | 28 +++
 gcc/testsuite/gcc.dg/cpp/builtin-macro-3.c | 28 +++
 libcpp/macro.c | 56 +++---
 5 files changed, 148 insertions(+), 12 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/cpp/builtin-macro-0.c
 create mode 100644 gcc/testsuite/gcc.dg/cpp/builtin-macro-1.c
 create mode 100644 gcc/testsuite/gcc.dg/cpp/builtin-macro-2.c
 create mode 100644 gcc/testsuite/gcc.dg/cpp/builtin-macro-3.c

diff --git a/gcc/testsuite/gcc.dg/cpp/builtin-macro-0.c 
b/gcc/testsuite/gcc.dg/cpp/builtin-macro-0.c
new file mode 100644
index 000..f57dd5b
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/builtin-macro-0.c
@@ -0,0 +1,18 @@
+/* Origin: PR preprocessor/61817

Re: [PATCH Fortran/Diagnostics] Move Fortran to common diagnostics machinery

[Dodji Seketeli]

Hello Manuel,

Manuel López-Ibáñez lopeziba...@gmail.com writes:

 2014-08-03  Manuel López-Ibáñez  m...@gcc.gnu.org

 PR fortran/44054
 c-family/
 * c-format.c: Handle Fortran flags.
 * diagnostic.c (build_message_string): Make it extern.
 * diagnostic.h (build_message_string): Make it extern.

Small nit; the diagnostic.[ch] files are in gcc/, not in c-family.

The language-agnostic diagnostic infrastructure changes otherwise look
OK to me.

Thanks.

-- 
Dodji

Re: [PATCH 1/2] Support location tracking for built-in macro tokens

[Dodji Seketeli]

Richard Biener richard.guent...@gmail.com writes:

 On Tue, Jul 15, 2014 at 3:27 PM, Dodji Seketeli do...@redhat.com wrote:
 Hello,

 When a built-in macro is expanded, the location of the token in the
 epansion list is the location of the expansion point of the built-in
 macro.

 This patch creates a virtual location for that token instead,
 effectively tracking locations of tokens resulting from built-in macro
 tokens.

 No testcase?

Hmmh, it was tricky to write something specifically for this case.  But
then the test cases of the second patch of the series does exercise this
patch.

Cheers.

-- 
Dodji

[PATCH] PR preprocessor/61817 - Fix location of tokens in built-in macro expansion list

[Dodji Seketeli]

Hello,

Consider this function-like macro definition in this inc.h file:

---inc.h---
#define F() const int line = __LINE__
---8---

Now consider its use in a file test.c:

--cat -n test.c
 1  #include inc.h
 2
 3  void
 4  foo()
 5  {
 6F
 7  (
 8   )
 9  ;
10  }
---8-

Running test.c through cc1 -quiet -E yields:

-8
# 1 test.c
# 1 built-in
# 1 command-line
# 1 /usr/include/stdc-predef.h 1 3 4
# 1 command-line 2
# 1 test.c
# 1 inc.h 1
# 2 test.c 2

void
foo()
{
  const int line =

 8
;
}
-8--

Note how tokens const, int, line and = are all on the same
line (line 6) as the expansion point of the function-like F() macro,
but how the token 8, resulting from the expansion of the built-in
macro __FILE__ is on the same line as the closing parenthesis of the
invocation of F().

This is the problem.  The result of the __FILE__ macro should be 6
and should be on the same line (line 6) as the other tokens of the
expansion-list of F().

This issue actually holds for the expansion of all built-in macros.

So more generelly, I would describe the issue as such:

When expanded in a function-like macro, the location of resulting
tokens of a built-in macro is set to the closing parenthesis of the
enclosing function-like macro invocation, rather than being set to the
location of the expansion point of the invocation the enclosing
function-like macro.

I think the problem is that enter_macro_context() is passed the
location of the expansion point of the macro that is about to be
expanded.  But when that macro is built-in, builtin_macro() that is
then called by enter_macro_context() doesn't use the macro expansion
point location.  Rather, builtin_macro uses the location of the
closing parenthesis of the enclosing function-like macro invocation
or, more generally, the location of the previous token in the stream.

The patch proposes to pass builtin_macro() the expansion point
location that enter_macro_context() already knows about and use that
to set the location of the resulting token.

Also, for the particular case of __LINE__, it makes that built-in expand
to the line number or that expansion point.

With the patch applied, the output becomes:

$ ./cc1 -quiet -E test.c
# 1 ../../../master/test/test.c
# 1 built-in
# 1 command-line
# 1 /usr/include/stdc-predef.h 1 3 4
# 1 command-line 2
# 1 ../../../master/test/test.c
# 1 ../../../master/test/inc.h 1
# 2 ../../../master/test/test.c 2

void
foo()
{
  const int line = 6


;
}
$

Bootstrapped and tested against trunk on x86_64-unknown-linux-gnu.

libcpp/
* macro.c (_cpp_builtin_macro_text): Honor the
cpp_reader::forced_token_location_p data member to force the value
__LINE__ expands to accordlingly.
(builtin_macro): Take the expansion point location as parameter.
Use cpp_force_token_locations() to set the location of the
resulting token to that expansion point location.
(enter_macro_context): Pass the expansion point locatoin to
builtin_macro.

gcc/testsuite/
* gcc.dg/cpp/builtin-macro-{0,1}.c: New test files.

Signed-off-by: Dodji Seketeli do...@redhat.com
---
 gcc/testsuite/gcc.dg/cpp/builtin-macro-0.c | 18 +++
 gcc/testsuite/gcc.dg/cpp/builtin-macro-1.c | 30 +
 libcpp/macro.c | 52 --
 3 files changed, 90 insertions(+), 10 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/cpp/builtin-macro-0.c
 create mode 100644 gcc/testsuite/gcc.dg/cpp/builtin-macro-1.c

diff --git a/gcc/testsuite/gcc.dg/cpp/builtin-macro-0.c 
b/gcc/testsuite/gcc.dg/cpp/builtin-macro-0.c
new file mode 100644
index 000..f57dd5b
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/builtin-macro-0.c
@@ -0,0 +1,18 @@
+/* Origin: PR preprocessor/61817
+
+  In this test case, the diagnostic (happening for tokens resulting
+  from the expansion of the built-in token __FILE__) must point to the
+  expansion point of the enclosing function-like macro invocation.
+
+  { dg-do compile }
+  { dg-options -no-integrated-cpp }  */
+
+#define F() static const char* __FILE__
+
+void
+foo ()
+{
+  F /* { dg-error expected identifier } */
+(
+ );
+}
diff --git a/gcc/testsuite/gcc.dg/cpp/builtin-macro-1.c 
b/gcc/testsuite/gcc.dg/cpp/builtin-macro-1.c
new file mode 100644
index 000..280797b
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/builtin-macro-1.c
@@ -0,0 +1,30 @@
+/* Origin: PR preprocessor/61817
+
+   This test detects that the __LINE__ built-in macro expands to the
+   line number

Re: [PATCH] PR preprocessor/60723 - missing system-ness marks for macro

[Dodji Seketeli]

Hello,

Jason Merrill ja...@redhat.com writes:

 // preprocessed output
 # 3 test.cpp 3 4
 sys_token
 # 3 test.cpp
 3
 # 3 test.cpp 3 4
 sys_token

 Yeah.  For Built-in tokens that are expanded like that we only do track
 their the location of their expansion, not their spelling location.  So
 this behaviour is expected as well.

 But surely you can do something to avoid this useless line marker in
 this case?  A built-in token should never require a line change.

 What triggers the line change is the *expansion* of the built-in macro
 __LINE__.

 That is, the token 3.  As the existing location tracking facility
 doesn't track the locations for tokens originating from the expansion of
 built-in macros, the relationship 3 - __LINE__ is lost.

 So what is the location of 3 in this case?

The location of the token 3 is 3, in that case.  It's the line number
of the expansion point of the __LINE__ built-in macro.  The token 3
does not have a virtual location that allows us to unwind back to the
special built-in spelling location 1, that would mean that 3
originates from a built-in macro.

 It seems to me that it doesn't really have its own location, and so
 we should be able to use whatever the current location is and not emit
 a line note.

The issue is that the location for 3 is not virtual, so there is not
much we can do about it.

But then I have just wrote the support for making that 3 token have a
virtual location.  I am sending a small patchset as a follow-up to this
message.

Cheers.

-- 
Dodji

[PATCH 1/2] Support location tracking for built-in macro tokens

[Dodji Seketeli]

Hello,

When a built-in macro is expanded, the location of the token in the
epansion list is the location of the expansion point of the built-in
macro.

This patch creates a virtual location for that token instead,
effectively tracking locations of tokens resulting from built-in macro
tokens.

libcpp/
* include/line-map.h (line_maps::builtin_location): New data
member.
(line_map_init): Add a new parameter to initialize the new
line_maps::builtin_location data member.
* line-map.c (linemap_init): Initialize the
line_maps::builtin_location data member.
* macro.c (builtin_macro): Create a macro map and track the token
resulting from the expansion of a built-in macro.
gcc/
* input.h (is_location_from_builtin_token): New function
declaration.
* input.c (is_location_from_builtin_token): New function
definition.
* toplev.c (general_init): Tell libcpp what the pre-defined
spelling location for built-in tokens is.

Signed-off-by: Dodji Seketeli do...@redhat.com
---
 gcc/input.c   | 16 
 gcc/input.h   |  1 +
 gcc/toplev.c  |  2 +-
 libcpp/include/line-map.h | 12 ++--
 libcpp/line-map.c |  4 +++-
 libcpp/macro.c| 23 ++-
 6 files changed, 53 insertions(+), 5 deletions(-)

diff --git a/gcc/input.c b/gcc/input.c
index 63cd062..f3fd0e9 100644
--- a/gcc/input.c
+++ b/gcc/input.c
@@ -713,6 +713,22 @@ location_get_source_line (expanded_location xloc,
   return read ? buffer : NULL;
 }
 
+/* Test if the location originates from the spelling location of a
+   builtin-tokens.  That is, return TRUE if LOC is a (possibly
+   virtual) location of a built-in token that appears in the expansion
+   list of a macro.  Please note that this function also works on
+   tokens that result from built-in tokens.  For instance, the
+   function would return true if passed a token 4 that is the result
+   of the expansion of the built-in __LINE__ macro.  */
+bool
+is_location_from_builtin_token (source_location loc)
+{
+  const line_map *map = NULL;
+  loc = linemap_resolve_location (line_table, loc,
+ LRK_SPELLING_LOCATION, map);
+  return loc == BUILTINS_LOCATION;
+}
+
 /* Expand the source location LOC into a human readable location.  If
LOC is virtual, it resolves to the expansion point of the involved
macro.  If LOC resolves to a builtin location, the file name of the
diff --git a/gcc/input.h b/gcc/input.h
index d910bb8..1def793 100644
--- a/gcc/input.h
+++ b/gcc/input.h
@@ -36,6 +36,7 @@ extern GTY(()) struct line_maps *line_table;
 extern char builtins_location_check[(BUILTINS_LOCATION
  RESERVED_LOCATION_COUNT) ? 1 : -1];
 
+extern bool is_location_from_builtin_token (source_location);
 extern expanded_location expand_location (source_location);
 extern const char *location_get_source_line (expanded_location xloc,
 int *line_size);
diff --git a/gcc/toplev.c b/gcc/toplev.c
index e35b826..9e747e5 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -1157,7 +1157,7 @@ general_init (const char *argv0)
   init_ggc ();
   init_stringpool ();
   line_table = ggc_allocline_maps ();
-  linemap_init (line_table);
+  linemap_init (line_table, BUILTINS_LOCATION);
   line_table-reallocator = realloc_for_line_map;
   line_table-round_alloc_size = ggc_round_alloc_size;
   init_ttree ();
diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
index 9886314..0c8f588 100644
--- a/libcpp/include/line-map.h
+++ b/libcpp/include/line-map.h
@@ -315,6 +315,10 @@ struct GTY(()) line_maps {
   line_map_round_alloc_size_func round_alloc_size;
 
   struct location_adhoc_data_map location_adhoc_data_map;
+
+  /* The special location value that is used as spelling location for
+ built-in tokens.  */
+  source_location builtin_location;
 };
 
 /* Returns the pointer to the memory region where information about
@@ -447,8 +451,12 @@ extern source_location get_location_from_adhoc_loc (struct 
line_maps *,
 
 extern void rebuild_location_adhoc_htab (struct line_maps *);
 
-/* Initialize a line map set.  */
-extern void linemap_init (struct line_maps *);
+/* Initialize a line map set.  SET is the line map set to initialize
+   and BUILTIN_LOCATION is the special location value to be used as
+   spelling location for built-in tokens.  This BUILTIN_LOCATION has
+   to be strictly less than RESERVED_LOCATION_COUNT.  */
+extern void linemap_init (struct line_maps *set,
+ source_location builtin_location);
 
 /* Check for and warn about line_maps entered but not exited.  */
 
diff --git a/libcpp/line-map.c b/libcpp/line-map.c
index f9a7658..a4055c2 100644
--- a/libcpp/line-map.c
+++ b/libcpp/line-map.c
@@ -175,13 +175,15 @@ location_adhoc_data_fini (struct line_maps *set)
 /* Initialize a line map set

[PATCH 2/2] PR preprocessor/60723 - missing system-ness marks for macro tokens

[Dodji Seketeli]

Hello,

When a system macro is expanded in a non-system file during
out-of-line preprocessing, it can happen that the preprocessor forgets
to emit line markers to express the system-ness status of tokens that
come after the expansion of the macro.

That can lead to situations where the entire non-system file can be
considered as being a system file and thus have its warnings be
discarded during the compilation of the resulting preprocessed file.

My understanding is that this is due to the preprocessor not
systematically detecting (and reporting) the change in system-ness of
tokens.

And this is what this patch does.  Each time the system-ness of a
given token is different from the previous token that was emitted by
the preprocessor, it emits a line marker for the sole purpose of
marking the new system-ness of the subsequent tokens to come.

The patch also avoids emitting either adjacent line change markers or
line markers in case of tokens originating from built-in macro
expansion.

Bootstrapped and tested on x86_64-unknown-linux-gnu against trunk.

gcc/c-family/ChangeLog:
* c-ppoutput.c (struct print::prev_was_system_token): New data
member.
(init_pp_output): Initialize it.
(maybe_print_line_1, maybe_print_line, print_line_1, print_line)
(do_line_change): Return a flag saying if a line marker was
emitted or not.
(scan_translation_unit): Detect if the system-ness of the token we
are about to emit is different from the one of the previously
emitted token.  If so, emit a line marker.  Avoid emitting useless
adjacent line markers.  Avoid emitting line markers for tokens
originating from the expansion of built-in macros.
(scan_translation_unit_directives_only): Adjust.

gcc/testsuite/ChangeLog:
* gcc.dg/cpp/syshdr{4,5}.{c,h}: New test files.

Signed-off-by: Dodji Seketeli do...@redhat.com

git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@212194 
138bc75d-0d04-0410-961f-82ee72b054a4
Signed-off-by: Dodji Seketeli do...@redhat.com
---
 gcc/c-family/c-ppoutput.c  | 81 +++---
 gcc/testsuite/gcc.dg/cpp/syshdr4.c | 24 +++
 gcc/testsuite/gcc.dg/cpp/syshdr4.h |  8 
 gcc/testsuite/gcc.dg/cpp/syshdr5.c | 14 +++
 gcc/testsuite/gcc.dg/cpp/syshdr5.h |  6 +++
 5 files changed, 109 insertions(+), 24 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/cpp/syshdr4.c
 create mode 100644 gcc/testsuite/gcc.dg/cpp/syshdr4.h
 create mode 100644 gcc/testsuite/gcc.dg/cpp/syshdr5.c
 create mode 100644 gcc/testsuite/gcc.dg/cpp/syshdr5.h

diff --git a/gcc/c-family/c-ppoutput.c b/gcc/c-family/c-ppoutput.c
index f3b5fa4..0292d16 100644
--- a/gcc/c-family/c-ppoutput.c
+++ b/gcc/c-family/c-ppoutput.c
@@ -36,6 +36,8 @@ static struct
   unsigned char printed;   /* Nonzero if something output at line.  */
   bool first_time; /* pp_file_change hasn't been called yet.  */
   const char *src_file;/* Current source file.  */
+  bool prev_was_system_token;  /* True if the previous token was a
+  system token.*/
 } print;
 
 /* Defined and undefined macros being queued for output with -dU at
@@ -58,11 +60,11 @@ static void account_for_newlines (const unsigned char *, 
size_t);
 static int dump_macro (cpp_reader *, cpp_hashnode *, void *);
 static void dump_queued_macros (cpp_reader *);
 
-static void print_line_1 (source_location, const char*, FILE *);
-static void print_line (source_location, const char *);
-static void maybe_print_line_1 (source_location, FILE *);
-static void maybe_print_line (source_location);
-static void do_line_change (cpp_reader *, const cpp_token *,
+static bool print_line_1 (source_location, const char*, FILE *);
+static bool print_line (source_location, const char *);
+static bool maybe_print_line_1 (source_location, FILE *);
+static bool maybe_print_line (source_location);
+static bool do_line_change (cpp_reader *, const cpp_token *,
source_location, int);
 
 /* Callback routines for the parser.   Most of these are active only
@@ -156,6 +158,7 @@ init_pp_output (FILE *out_stream)
   print.outf = out_stream;
   print.first_time = 1;
   print.src_file = ;
+  print.prev_was_system_token = false;
 }
 
 /* Writes out the preprocessed file, handling spacing and paste
@@ -168,6 +171,7 @@ scan_translation_unit (cpp_reader *pfile)
 = cpp_get_options (parse_in)-lang != CLK_ASM
!flag_no_line_commands;
   bool in_pragma = false;
+  bool line_marker_emitted = false;
 
   print.source = NULL;
   for (;;)
@@ -200,7 +204,7 @@ scan_translation_unit (cpp_reader *pfile)
   do_line_adjustments
   !in_pragma)
{
- do_line_change (pfile, token, loc, false);
+ line_marker_emitted = do_line_change (pfile, token, loc, false);
  putc (' ', print.outf);
}
  else if (print.source-flags  PREV_WHITE

Re: [PATCH 1/2] Support location tracking for built-in macro tokens

[Dodji Seketeli]

I forgot to say that ...

Dodji Seketeli do...@redhat.com writes:

[...]

 When a built-in macro is expanded, the location of the token in the
 epansion list is the location of the expansion point of the built-in
 macro.

 This patch creates a virtual location for that token instead,
 effectively tracking locations of tokens resulting from built-in macro
 tokens.

 libcpp/
   * include/line-map.h (line_maps::builtin_location): New data
   member.
   (line_map_init): Add a new parameter to initialize the new
   line_maps::builtin_location data member.
   * line-map.c (linemap_init): Initialize the
   line_maps::builtin_location data member.
   * macro.c (builtin_macro): Create a macro map and track the token
   resulting from the expansion of a built-in macro.
 gcc/
   * input.h (is_location_from_builtin_token): New function
   declaration.
   * input.c (is_location_from_builtin_token): New function
   definition.
   * toplev.c (general_init): Tell libcpp what the pre-defined
   spelling location for built-in tokens is.

... that this was bootstrapped and tested against trunk on
x86_64-unknown-linux-gnu.

[...]

Cheers.

-- 
Dodji

Re: [PATCH] PR preprocessor/60723 - missing system-ness marks for macro

[Dodji Seketeli]

Hello Nicholas,

Nicholas Ormrod nicholas.orm...@hotmail.com writes:

[...]

 If you are also going to fix the locations of built-in tokens, it
 would also be worth adjusting their expansion locations. As mentioned
 in the bug report, built-in tokens are expanded at the closing paren
 of a macro call, whereas non-built-in tokens are expanded at the
 opening paren. This is weird.

Yes it is weird.

From what I understood while looking at this, this is also a separate
issue that ought to be addressed in a separate patch with a separate
test case.  I'll look at that.

Cheers.

-- 
Dodji

Re: [PATCH] PR preprocessor/60723 - missing system-ness marks for macro

[Dodji Seketeli]

Jason Merrill ja...@redhat.com writes:

 On 07/04/2014 05:13 AM, Dodji Seketeli wrote:
 // preprocessed output
 # 3 test.cpp 3 4
 sys_token
 # 3 test.cpp
 3
 # 3 test.cpp 3 4
 sys_token

 Yeah.  For Built-in tokens that are expanded like that we only do track
 their the location of their expansion, not their spelling location.  So
 this behaviour is expected as well.

 But surely you can do something to avoid this useless line marker in
 this case?  A built-in token should never require a line change.

What triggers the line change is the *expansion* of the built-in macro
__LINE__.

That is, the token 3.  As the existing location tracking facility
doesn't track the locations for tokens originating from the expansion of
built-in macros, the relationship 3 - __LINE__ is lost.

My understanding is that to avoid emitting the line marker in this case,
we'd need to track the 3 - __LINE__ relationship so that we can
detect that 3 is the expansion of the built-in macro__LINE__.

This would be a separate patch that I'd need to work on and test
separately I guess.

Should that prevent this patch to go in now?

-- 
Dodji

Re: [PATCH] PR preprocessor/60723 - missing system-ness marks for macro

[Dodji Seketeli]

Hello Nicholas,

Nicholas Ormrod nicholas.orm...@hotmail.com writes:

 I found time this morning to run your changes through our system. I
 patched our gcc-4.8.1 with your latest change, and ran it through our
 folly testsuite.

Thanks!

 One thing that I immediately noticed was that this increased the preprocessed 
 size substantially.
 When preprocessing my favorite .cpp file, its .ii grew from 137k lines
 to 145k, a 5% increase.

Yeah, the growth is expected.  It's interesting to have actual numbers,
thank you for that.

 All the folly code compiled and ran successfully under the changes.

 I looked at some of the preprocessed output. I was pleased to see that
 consecutive macros that expanded entirely to system tokens did not
 insert unnecessary line directives between them.

Cool!

 I did, however, notice that __LINE__ was treated as belonging to the
 calling file, even when its token appears in the system file. That is
 to say:

 CODE:

 // system macro
 #define FOO() sys_token __LINE__ sys_token

 // non-system callsite
 FOO()

 // preprocessed output
 # 3 test.cpp 3 4
 sys_token
 # 3 test.cpp
 3
 # 3 test.cpp 3 4
 sys_token

 :CODE

Yeah.  For Built-in tokens that are expanded like that we only do track
their the location of their expansion, not their spelling location.  So
this behaviour is expected as well.

 This seems to generalize to other builtin macros, like __FILE__.

Indeed.


 Otherwise, the code looks fine. There is only one thing I noticed:

 + if (do_line_adjustments
 +  !in_pragma
 +  !line_marker_emitted
 +  print.prev_was_system_token != !!in_system_header_at(loc))
 + /* The system-ness of this token is different from the one
 + of the previous token. Let's emit a line change to
 + mark the new system-ness before we emit the token. */
 + line_marker_emitted = do_line_change (pfile, token, loc, false);

 This line_marker_emitted assignment is immediately overwritten, two lines 
 below. However, from a
 maintainability perspective, this is probably a good assignment to
 keep.

Yeah, maintainability is why I kept it.  But if the maintainers feel
strongly about it I can, certainly just remove that assignment.

Thank you for your time on this!

Cheers.

-- 
Dodji

Re: [PATCH] PR preprocessor/60723 - missing system-ness marks for macro

[Dodji Seketeli]

Jason Merrill ja...@redhat.com writes:

 On 06/27/2014 03:27 AM, Dodji Seketeli wrote:
 +   print.prev_was_system_token != !!in_system_header_at(loc))
 +/* The system-ness of this token is different from the one
 +   of the previous token.  Let's emit a line change to
 +   mark the new system-ness before we emit the token.  */
 +line_marker_emitted = do_line_change(pfile, token, loc, false);

 Missing spaces before '('.  OK with that fixed.

Thanks.

It appeared that the patch was too eager to emit line changes, even for
cases (like when preprocessing asm files) where a new line between
tokens can be significant and turn a valid statement into an invalid
one.

I have updated the patch to prevent that and tested it again on
x86_64-unknown-linux-gnu.  Christophe Lyon (who reported this latest
issue) tested it on his ARM-based system that exhibited the issue.

The relevant hunk that changes is this one:

@@ -248,9 +252,20 @@ scan_translation_unit (cpp_reader *pfile)
  if (cpp_get_options (parse_in)-debug)
  linemap_dump_location (line_table, token-src_loc,
 print.outf);
+
+ if (do_line_adjustments
+  !in_pragma
+  !line_marker_emitted
+  print.prev_was_system_token != !!in_system_header_at(loc))
+   /* The system-ness of this token is different from the one
+  of the previous token.  Let's emit a line change to
+  mark the new system-ness before we emit the token.  */
+   line_marker_emitted = do_line_change (pfile, token, loc, false);
  cpp_output_token (token, print.outf);
+ line_marker_emitted = false;
}
 
+  print.prev_was_system_token = !!in_system_header_at(loc);
   /* CPP_COMMENT tokens and raw-string literal tokens can
 have embedded new-line characters.  Rather than enumerating
 all the possible token types just check if token uses

In there, the change is that I am now testing that line adjustments are
allowed and that we are not inside pragmas with the:

+ if (do_line_adjustments
+  !in_pragma

This make the change coherent with what is done elsewhere in
scan_translation_unit.

OK to commit this latest version to trunk?

gcc/c-family/ChangeLog:
* c-ppoutput.c (struct print::prev_was_system_token): New data
member.
(init_pp_output): Initialize it.
(maybe_print_line_1, maybe_print_line, print_line_1, print_line)
(do_line_change): Return a flag saying if a line marker was
emitted or not.
(scan_translation_unit): Detect if the system-ness of the token we
are about to emit is different from the one of the previously
emitted token.  If so, emit a line marker.  Avoid emitting
useless adjacent line markers.
(scan_translation_unit_directives_only): Adjust.

gcc/testsuite/ChangeLog:
* gcc.dg/cpp/syshdr{4,5}.{c,h}: New test files.

Signed-off-by: Dodji Seketeli do...@redhat.com

git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@212194 
138bc75d-0d04-0410-961f-82ee72b054a4
Signed-off-by: Dodji Seketeli do...@redhat.com
---
 gcc/c-family/ChangeLog | 15 
 gcc/c-family/c-ppoutput.c  | 78 ++
 gcc/testsuite/ChangeLog|  5 +++
 gcc/testsuite/gcc.dg/cpp/syshdr4.c | 24 
 gcc/testsuite/gcc.dg/cpp/syshdr4.h |  8 
 gcc/testsuite/gcc.dg/cpp/syshdr5.c | 14 +++
 gcc/testsuite/gcc.dg/cpp/syshdr5.h |  6 +++
 7 files changed, 126 insertions(+), 24 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/cpp/syshdr4.c
 create mode 100644 gcc/testsuite/gcc.dg/cpp/syshdr4.h
 create mode 100644 gcc/testsuite/gcc.dg/cpp/syshdr5.c
 create mode 100644 gcc/testsuite/gcc.dg/cpp/syshdr5.h

diff --git a/gcc/c-family/c-ppoutput.c b/gcc/c-family/c-ppoutput.c
index f3b5fa4..400d3a7 100644
--- a/gcc/c-family/c-ppoutput.c
+++ b/gcc/c-family/c-ppoutput.c
@@ -36,6 +36,8 @@ static struct
   unsigned char printed;   /* Nonzero if something output at line.  */
   bool first_time; /* pp_file_change hasn't been called yet.  */
   const char *src_file;/* Current source file.  */
+  bool prev_was_system_token;  /* True if the previous token was a
+  system token.*/
 } print;
 
 /* Defined and undefined macros being queued for output with -dU at
@@ -58,11 +60,11 @@ static void account_for_newlines (const unsigned char *, 
size_t);
 static int dump_macro (cpp_reader *, cpp_hashnode *, void *);
 static void dump_queued_macros (cpp_reader *);
 
-static void print_line_1 (source_location, const char*, FILE *);
-static void print_line (source_location, const char *);
-static void maybe_print_line_1 (source_location, FILE *);
-static void maybe_print_line (source_location);
-static void do_line_change (cpp_reader *, const cpp_token *,
+static bool print_line_1

[PATCH] PR preprocessor/60723 - missing system-ness marks for macro

[Dodji Seketeli]

Hello,

When a system macro is expanded in a non-system file during
out-of-line preprocessing, it can happen that the preprocessor forgets
to emit line markers to express the system-ness status of tokens that
come after the expansion of the macro.

That can lead to situations where the entire non-system file can be
considered as being a system file and thus have its warnings be
discarded during the compilation of the resulting preprocessed file.

My understanding is that this is due to the preprocessor not
systematically detecting (and reporting) the change in system-ness of
tokens.

And this is what this patch does.  Each time the system-ness of a
given token is different from the previous token that was emitted by
the preprocessor, it emits a line marker for the sole purpose of
marking the new system-ness of the subsequent tokens to come.

Bootstrapped and tested on x86_64-unknown-linux-gnu against trunk.

gcc/c-family/ChangeLog:
* c-ppoutput.c (struct print::prev_was_system_token): New data
member.
(init_pp_output): Initialize it.
(maybe_print_line_1, maybe_print_line, print_line_1, print_line)
(do_line_change): Return a flag saying if a line marker was
emitted or not.
(scan_translation_unit): Detect if the system-ness of the token we
are about to emit is different from the one of the previously
emitted token.  If so, emit a line marker.  Avoid emitting
useless adjacent line markers.
(scan_translation_unit_directives_only): Adjust.

gcc/testsuite/ChangeLog:
* gcc.dg/cpp/syshdr{4,5}.{c,h}: New test files.

Signed-off-by: Dodji Seketeli do...@redhat.com
---
 gcc/c-family/c-ppoutput.c  | 76 ++
 gcc/testsuite/gcc.dg/cpp/syshdr4.c | 24 
 gcc/testsuite/gcc.dg/cpp/syshdr4.h |  8 
 gcc/testsuite/gcc.dg/cpp/syshdr5.c | 14 +++
 gcc/testsuite/gcc.dg/cpp/syshdr5.h |  6 +++
 5 files changed, 104 insertions(+), 24 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/cpp/syshdr4.c
 create mode 100644 gcc/testsuite/gcc.dg/cpp/syshdr4.h
 create mode 100644 gcc/testsuite/gcc.dg/cpp/syshdr5.c
 create mode 100644 gcc/testsuite/gcc.dg/cpp/syshdr5.h

diff --git a/gcc/c-family/c-ppoutput.c b/gcc/c-family/c-ppoutput.c
index f3b5fa4..be4b674 100644
--- a/gcc/c-family/c-ppoutput.c
+++ b/gcc/c-family/c-ppoutput.c
@@ -36,6 +36,8 @@ static struct
   unsigned char printed;   /* Nonzero if something output at line.  */
   bool first_time; /* pp_file_change hasn't been called yet.  */
   const char *src_file;/* Current source file.  */
+  bool prev_was_system_token;  /* True if the previous token was a
+  system token.*/
 } print;
 
 /* Defined and undefined macros being queued for output with -dU at
@@ -58,11 +60,11 @@ static void account_for_newlines (const unsigned char *, 
size_t);
 static int dump_macro (cpp_reader *, cpp_hashnode *, void *);
 static void dump_queued_macros (cpp_reader *);
 
-static void print_line_1 (source_location, const char*, FILE *);
-static void print_line (source_location, const char *);
-static void maybe_print_line_1 (source_location, FILE *);
-static void maybe_print_line (source_location);
-static void do_line_change (cpp_reader *, const cpp_token *,
+static bool print_line_1 (source_location, const char*, FILE *);
+static bool print_line (source_location, const char *);
+static bool maybe_print_line_1 (source_location, FILE *);
+static bool maybe_print_line (source_location);
+static bool do_line_change (cpp_reader *, const cpp_token *,
source_location, int);
 
 /* Callback routines for the parser.   Most of these are active only
@@ -156,6 +158,7 @@ init_pp_output (FILE *out_stream)
   print.outf = out_stream;
   print.first_time = 1;
   print.src_file = ;
+  print.prev_was_system_token = false;
 }
 
 /* Writes out the preprocessed file, handling spacing and paste
@@ -168,6 +171,7 @@ scan_translation_unit (cpp_reader *pfile)
 = cpp_get_options (parse_in)-lang != CLK_ASM
!flag_no_line_commands;
   bool in_pragma = false;
+  bool line_marker_emitted = false;
 
   print.source = NULL;
   for (;;)
@@ -200,7 +204,7 @@ scan_translation_unit (cpp_reader *pfile)
   do_line_adjustments
   !in_pragma)
{
- do_line_change (pfile, token, loc, false);
+ line_marker_emitted = do_line_change (pfile, token, loc, false);
  putc (' ', print.outf);
}
  else if (print.source-flags  PREV_WHITE
@@ -216,7 +220,7 @@ scan_translation_unit (cpp_reader *pfile)
  if (src_line != print.src_line
   do_line_adjustments
   !in_pragma)
-   do_line_change (pfile, token, loc, false);
+   line_marker_emitted = do_line_change (pfile, token, loc, false);
  putc (' ', print.outf);
}
 
@@ -228,7 +232,7 @@ scan_translation_unit

Re: [PATCH, cpp] Fix line directive bug

[Dodji Seketeli]

Hello Nicholas,

Nicholas Ormrod nicholas.orm...@hotmail.com a écrit:


 We currently have two possible approaches. I see the trade-offs between these 
 two solutions as follows:

 Adding full line directives is not implemented, and will be more
 complicated than my patch. It will require someone else to implement
 it, but should fix the problem in its entirety.

Okay, let me look at this full-blown solution and I'll propose a patch
soon.

Cheers.

-- 
Dodji

Re: [PATCH, cpp] Fix line directive bug‏

[Dodji Seketeli]

Hello Nicholas,

First of all, thank you for taking the time to dive into this code and
provide such a detailed analysis along with a patch.  This is
appreciated.

Please find below my comments to some parts of your message.

Nicholas Ormrod nicholas.orm...@hotmail.com a écrit:

 PR preprocessor/60723
 
 Description:
 
 When line directives are inserted into the expansion of a macro, the line
 directive may erroneously specify the file as being a system file. This
 causes certain warnings to be suppressed in the rest of the file.

Agreed.

 The fact that line directives are ever inserted into a macro is itself a
 half-bug. Please see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60723 for
 full details.

We could discuss that, but I might slightly disagree.  I would rather
say that it's the way the directives are inserted that is a bug.  But
let's put this topic aside for now.

To ease the discussion at hand, let me paste here the test case that you
submitted to the bugzilla:

cat inc.h
#define FOO() static const char * F = __FILE__ ;
$ cat src.cpp 
#include inc.h
FOO(
)

int main() {
  int z = 1 / 0;
  return z;
}
$
$ g++ -E -isystem . src.cpp 
# 1 src.cpp
# 1 interne
# 1 command-line
# 1 /usr/include/stdc-predef.h 1 3 4
# 1 command-line 2
# 1 src.cpp
# 1 ./inc.h 1 3 4
# 2 src.cpp 2
static const char * F =
 src.cpp
# 2 src.cpp 3 4
 ;


int main() {
  int z = 1 / 0;
  return z;
}
$ 

So when compiling the resulting pre-processed file, the warning
concerning the division by zero is not emitted, and that is a bug, I
agree with you.

 Patch:
 
 Information for locations is, for similar pieces of data, read from a
 LOCATION_* macro. The sysp read which was causing the error was using an
 inconsistent method to read the data. Resolving this is a two-line
 fix.

Maybe I am missing something, but my understanding seems to differ
here, sorry.

I think that we really want to be able to say if the macro FOO that got
expanded in the file src.cpp was actually *defined* in a system header
or not.  That is, if the macro is a system macro[1] or not.  Because the
expansion of a system macro should (generally) not emit a warning, even
when that expansion occurs in a file (src.cpp in your example from
bugzilla) that is not a system file.

So in that case we want to suppress the potential warnings that arise
from the macro expansion.

But the issue here is, I think, that (in src.cpp) we consider the tokens
resulting from the expansion of the macro FOO as being system tokens[2]
(and rightly so) and *also* that all the subsequent tokens of the
src.cpp file are being system tokens; and this is wrong.

So, I would tend to think that a potential proper fix would emit a
subsequent line directive after the lines:

# 2 src.cpp 3 4
 ;

So that the pre-processed file looks like:

$ g++ -E -isystem . src.cpp 
# 1 src.cpp
# 1 interne
# 1 command-line
# 1 /usr/include/stdc-predef.h 1 3 4
# 1 command-line 2
# 1 src.cpp
# 1 ./inc.h 1 3 4
# 2 src.cpp 2
static const char * F =
 src.cpp
# 2 src.cpp 3 4
 ;
# 3 src.cpp 4


int main() {
  int z = 1 / 0;
  return z;
}
$ 

Note the additional line directive # 3 src.cpp 4 that doesn't
mention the '3' flags and thus says that the rest of the tokens are
*not* system tokens.

What do you think?

[1]: A system macro is a macro defined in a system header.
[2]: A system token is a token coming from the expansion of a system macro

Cheers,

-- 
Dodji

Re: libsanitizer merge from upstream r208536

[Dodji Seketeli]

Jakub Jelinek ja...@redhat.com writes:

[...]

 Ok, tried to merge also g++.dg/asan from the same revision as you've merged
 libsanitizer.

[...]


 2014-05-22  Jakub Jelinek  ja...@redhat.com

   * g++.dg/asan/asan_test.C: Add -std=c++11 and
   -DSANITIZER_USE_DEJAGNU_GTEST=1 to dg-options, remove
   -DASAN_USE_DEJAGNU_GTEST=1.
   * g++.dg/asan/asan_mem_test.cc: Updated from upstream
   r209283.
   * g++.dg/asan/asan_oob_test.cc: Likewise.
   * g++.dg/asan/sanitizer_test_utils.h: Likewise.
   * g++.dg/asan/asan_str_test.cc: Likewise.
   * g++.dg/asan/asan_test_utils.h: Likewise.
   * g++.dg/asan/sanitizer_test_config.h: Likewise.
   * g++.dg/asan/asan_test.cc: Likewise.
   * g++.dg/asan/sanitizer_pthread_wrappers.h: New file.
   Imported from upstream r209283.
   * g++.dg/asan/asan_test_config.h: Likewise.

This is OK.

Thanks.

-- 
Dodji

Re: libsanitizer merge from upstream r208536

[Dodji Seketeli]

Jakub Jelinek ja...@redhat.com writes:

 On Thu, May 22, 2014 at 04:07:38PM +0200, Jakub Jelinek wrote:
 2014-05-22  Jakub Jelinek  ja...@redhat.com
 
  * sanitizer.def (BUILT_IN_ASAN_REPORT_LOAD_N,
  BUILT_IN_ASAN_REPORT_STORE_N): New.
  * asan.c (struct asan_mem_ref): Change access_size type to
  HOST_WIDE_INT.
  (asan_mem_ref_init, asan_mem_ref_new, get_mem_refs_of_builtin_call,
  update_mem_ref_hash_table): Likewise.
  (asan_mem_ref_hasher::hash): Hash in a HWI.
  (report_error_func): Change size_in_bytes argument to HWI.
  Use *_N builtins if size_in_bytes is larger than 16 or not power of
  two.
  (build_shadow_mem_access): New function.
  (build_check_stmt): Use it.  Change size_in_bytes argument to HWI.
  Handle size_in_bytes not power of two or larger than 16.
  (instrument_derefs): Don't give up if size_in_bytes is not
  power of two or is larger than 16.

 Both patches bootstrapped/regtested on x86_64-linux and i686-linux
 successfully, ok for trunk for both?

Yes, this is OK.  Thanks.

-- 
Dodji

Re: libsanitizer merge from upstream r208536

[Dodji Seketeli]

Jakub Jelinek ja...@redhat.com writes:


[...]

 Here is the GCC side of the thing, 

[...]

 2014-05-23  Jakub Jelinek  ja...@redhat.com

   * asan.c (report_error_func): Add SLOW_P argument, use
   BUILT_IN_ASAN_*_N if set.
   (build_check_stmt): Likewise.
   (instrument_derefs): If T has insufficient alignment,
   force same handling as for odd sizes.

   * c-c++-common/asan/misalign-1.c: New test.
   * c-c++-common/asan/misalign-2.c: New test.


 ok for trunk if it bootstraps/regtests?

Yes, this is OK for me.

Thanks.

-- 
Dodji

Re: [PATCH] Fix -ftrack-macro-expansion preprocessing of A######A (PR preprocessor/58844)

[Dodji Seketeli]

Jakub Jelinek ja...@redhat.com writes:

 Hi!

 The following testcase build with -ftrack-macro-expansion=0,
 but don't build otherwise.  The problem seems to be that
 the libcpp for macro redefinition warning/error purposes if it sees
 more than one paste operator adds those extra CPP_PASTE tokens at the end,
 after normal tokens from the macro.  For -ftrack-macro-expansion=0 we were
 using macro_real_token_count (macro) to only use the real tokens for macro
 expansion purposes, but for track_macro_expansion it used macro-count,
 which includes also the extra tokens.

 Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux,
 ok for trunk (and after a while for 4.8)?

For what it's worth, this is OK for me, yes.  Though I am just a casual
contributor giving my point of view here.


 2014-02-18  Jakub Jelinek  ja...@redhat.com

   PR preprocessor/58844
   * macro.c (enter_macro_context): Only push
   macro_real_token_count (macro) tokens rather than
   macro-count tokens, regardless of
   CPP_OPTION (pfile, track-macro-expansion).

   * c-c++-common/cpp/pr58844-1.c: New test.
   * c-c++-common/cpp/pr58844-2.c: New test.

Thanks.

-- 
Dodji

Re: [PATCH] preprocessor/58580 - preprocessor goes OOM with warning for zero literals

[Dodji Seketeli]

H.J. Lu hjl.to...@gmail.com writes:

 The new tests failed on Linux/x86:

Woops.

I have committed the patch below under the obvious rule for this.  Sorry
for the inconvenience.


gcc/testsuite/ChangeLog:

* c-c++-common/cpp/warning-zero-location-2.c: Fix error message
specifier.

diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index a468447..2da 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2014-01-29  Dodji Seketeli  do...@redhat.com
+
+   * c-c++-common/cpp/warning-zero-location-2.c: Fix error message
+   selector.
+
 2014-01-29  Jakub Jelinek  ja...@redhat.com
 
PR middle-end/59917
diff --git a/gcc/testsuite/c-c++-common/cpp/warning-zero-location-2.c 
b/gcc/testsuite/c-c++-common/cpp/warning-zero-location-2.c
index c0e0bf7..e919bca 100644
--- a/gcc/testsuite/c-c++-common/cpp/warning-zero-location-2.c
+++ b/gcc/testsuite/c-c++-common/cpp/warning-zero-location-2.c
@@ -7,4 +7,4 @@
 #include .h
 int main() { return 0; }
 
-/* { dg-error No such file or directory { target *-*-* } 4636 } */
+/* { dg-message  #include {target *-*-* } 0 }
-- 
Dodji

Re: [PATCH] preprocessor/58580 - preprocessor goes OOM with warning for zero literals

[Dodji Seketeli]

Here is the patch I am committing right now.

gcc/ChangeLog

* input.c (location_get_source_line): Bail out on when line number
is zero, and test the return value of
lookup_or_add_file_to_cache_tab.

gcc/testsuite/ChangeLog

* c-c++-common/cpp/warning-zero-location.c: New test.
* c-c++-common/cpp/warning-zero-location-2.c: Likewise.

diff --git a/gcc/input.c b/gcc/input.c
index 547c177..63cd062 100644
--- a/gcc/input.c
+++ b/gcc/input.c
@@ -698,7 +698,13 @@ location_get_source_line (expanded_location xloc,
   static char *buffer;
   static ssize_t len;
 
-  fcache * c = lookup_or_add_file_to_cache_tab (xloc.file);
+  if (xloc.line == 0)
+return NULL;
+
+  fcache *c = lookup_or_add_file_to_cache_tab (xloc.file);
+  if (c == NULL)
+return NULL;
+
   bool read = read_line_num (c, xloc.line, buffer, len);
 
   if (read  line_len)
diff --git a/gcc/testsuite/c-c++-common/cpp/warning-zero-location-2.c 
b/gcc/testsuite/c-c++-common/cpp/warning-zero-location-2.c
new file mode 100644
index 000..c0e0bf7
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/cpp/warning-zero-location-2.c
@@ -0,0 +1,10 @@
+/*
+   { dg-options -D _GNU_SOURCE -fdiagnostics-show-caret }
+   { dg-do compile }
+ */
+
+#line 4636 configure
+#include .h
+int main() { return 0; }
+
+/* { dg-error No such file or directory { target *-*-* } 4636 } */
diff --git a/gcc/testsuite/c-c++-common/cpp/warning-zero-location.c 
b/gcc/testsuite/c-c++-common/cpp/warning-zero-location.c
new file mode 100644
index 000..ca2e102
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/cpp/warning-zero-location.c
@@ -0,0 +1,8 @@
+/*
+   { dg-options -D _GNU_SOURCE -fdiagnostics-show-caret }
+   { dg-do compile }
+ */
+
+#define _GNU_SOURCE/* { dg-warning redefined } */
+
+/* { dg-message  #define _GNU_SOURCE {target *-*-* } 0 }
-- 
Dodji

Re: [PATCH] preprocessor/58580 - preprocessor goes OOM with warning for zero literals

[Dodji Seketeli]

Dodji Seketeli do...@redhat.com writes:

 Here is the patch I am committing right now.

 gcc/ChangeLog

   * input.c (location_get_source_line): Bail out on when line number
   is zero, and test the return value of
   lookup_or_add_file_to_cache_tab.

 gcc/testsuite/ChangeLog

   * c-c++-common/cpp/warning-zero-location.c: New test.
   * c-c++-common/cpp/warning-zero-location-2.c: Likewise.

I forgot to say that it passed bootstrap  test on
x86_64-unknown-linux-gnu against trunk.

Thanks.

-- 
Dodji

Re: [PATCH] preprocessor/58580 - preprocessor goes OOM with warning for zero literals

[Dodji Seketeli]

Markus Trippelsdorf mar...@trippelsdorf.de writes:

 On 2014.01.22 at 09:16 +0100, Dodji Seketeli wrote:
 Bernd Edlinger bernd.edlin...@hotmail.de writes:
 
  Hi,
 
 Hello,
 
  since there was no progress in the last 2 months on that matter,
 
 Sorry, this is my bad.  I got sidetracked by something else and forgot
 that I had the patch working et al, and all its bits that need approval
 got approved.  It still can go in right now.  It improves performance
 and fixes the issue the way it was discussed.
 
 Here it is, regtested on x86_64-linux-gnu against trunk.
 
 If nobody objects in the next 24 hours, I'll commit it.

 The patch causes http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59935 .
 The follow-up patch (fp == NULL check) doesn't help.

I am looking into that, sorry for the inconvenience.

-- 
Dodji

Re: [PATCH] preprocessor/58580 - preprocessor goes OOM with warning for zero literals

[Dodji Seketeli]

Jakub Jelinek ja...@redhat.com writes:

 On Fri, Jan 24, 2014 at 04:40:52PM +0100, Dodji Seketeli wrote:
  The patch causes http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59935 .
  The follow-up patch (fp == NULL check) doesn't help.
 
 I am looking into that, sorry for the inconvenience.

 I'd say we want something like following.  Note that while the c == NULL
 bailout would be usually sufficient, if you'll do:
 echo foobar  'command-line'
 it would still crash.  Line 0 is used only for the special locations
 (command line, built-in macros) and there is no file associated with it
 anyway.

 --- gcc/input.c.jj2014-01-24 16:32:34.0 +0100
 +++ gcc/input.c   2014-01-24 16:41:42.012671452 +0100
 @@ -698,7 +698,13 @@ location_get_source_line (expanded_locat
static char *buffer;
static ssize_t len;
  
 -  fcache * c = lookup_or_add_file_to_cache_tab (xloc.file);
 +  if (xloc.line == 0)
 +return NULL;
 +
 +  fcache *c = lookup_or_add_file_to_cache_tab (xloc.file);
 +  if (c == NULL)
 +return NULL;
 +
bool read = read_line_num (c, xloc.line, buffer, len);
  
if (read  line_len)

Indeed.

Though, I am testing the patch below that makes read_line_num gracefully
handle empty caches or zero locations.  The rest of the code should just
work with that as is.

* input.c (read_line_num): Gracefully handle non-file locations or
empty caches.

diff --git a/gcc/input.c b/gcc/input.c
index 547c177..b05e1da 100644
--- a/gcc/input.c
+++ b/gcc/input.c
@@ -600,7 +600,8 @@ static bool
 read_line_num (fcache *c, size_t line_num,
   char ** line, ssize_t *line_len)
 {
-  gcc_assert (line_num  0);
+  if (!c || line_num  1)
+return false;
 
   if (line_num = c-line_num)
 {
diff --git a/gcc/testsuite/c-c++-common/cpp/warning-zero-location.c 
b/gcc/testsuite/c-c++-common/cpp/warning-zero-location.c
new file mode 100644
index 000..04a06b2
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/cpp/warning-zero-location.c
@@ -0,0 +1,6 @@
+/*
+   { dg-options -D _GNU_SOURCE }
+   { dg-do compile }
+ */
+
+#define _GNU_SOURCE/* { dg-warning redefined } */
-- 
Dodji

Re: [PATCH] preprocessor/58580 - preprocessor goes OOM with warning for zero literals

[Dodji Seketeli]

Jakub Jelinek ja...@redhat.com writes:

 On Wed, Jan 22, 2014 at 09:16:02AM +0100, Dodji Seketeli wrote:
 +static fcache*
 +add_file_to_cache_tab (const char *file_path)
 +{
 +
 +  FILE *fp = fopen (file_path, r);
 +  if (ferror (fp))
 +{
 +  fclose (fp);
 +  return NULL;
 +}

 I've seen various segfaults here when playing with preprocessed sources
 from PRs (obviously don't have the original source files).
 When fopen fails, it just returns NULL, so I don't see why you just don't
 do
   if (fp == NULL)
 return fp;

Right, I am testing the patch below.

* input.c (add_file_to_cache_tab): Handle the case where fopen
returns NULL.

diff --git a/gcc/input.c b/gcc/input.c
index 290680c..547c177 100644
--- a/gcc/input.c
+++ b/gcc/input.c
@@ -293,11 +293,8 @@ add_file_to_cache_tab (const char *file_path)
 {
 
   FILE *fp = fopen (file_path, r);
-  if (ferror (fp))
-{
-  fclose (fp);
-  return NULL;
-}
+  if (fp == NULL)
+return NULL;
 
   unsigned highest_use_count = 0;
   fcache *r = evicted_cache_tab_entry (highest_use_count);
-- 
Dodji

Re: [PATCH] preprocessor/58580 - preprocessor goes OOM with warning for zero literals

[Dodji Seketeli]

Bernd Edlinger bernd.edlin...@hotmail.de writes:

 Hi,

Hello,

 since there was no progress in the last 2 months on that matter,

Sorry, this is my bad.  I got sidetracked by something else and forgot
that I had the patch working et al, and all its bits that need approval
got approved.  It still can go in right now.  It improves performance
and fixes the issue the way it was discussed.

Here it is, regtested on x86_64-linux-gnu against trunk.

If nobody objects in the next 24 hours, I'll commit it.

libcpp/ChangeLog:

* include/line-map.h (linemap_get_file_highest_location): Declare
new function.
* line-map.c (linemap_get_file_highest_location): Define it.

gcc/ChangeLog:

* input.h (location_get_source_line): Take an additional line_size
parameter.
(void diagnostics_file_cache_fini): Declare new function.
* input.c (struct fcache): New type.
(fcache_tab_size, fcache_buffer_size, fcache_line_record_size):
New static constants.
(diagnostic_file_cache_init, total_lines_num)
(lookup_file_in_cache_tab, evicted_cache_tab_entry)
(add_file_to_cache_tab, lookup_or_add_file_to_cache_tab)
(needs_read, needs_grow, maybe_grow, read_data, maybe_read_data)
(get_next_line, read_next_line, goto_next_line, read_line_num):
New static function definitions.
(diagnostic_file_cache_fini): New function.
(location_get_source_line): Take an additional output line_len
parameter.  Re-write using lookup_or_add_file_to_cache_tab and
read_line_num.
* diagnostic.c (diagnostic_finish): Call
diagnostic_file_cache_fini.
(adjust_line): Take an additional input parameter for the length
of the line, rather than calculating it with strlen.
(diagnostic_show_locus): Adjust the use of
location_get_source_line and adjust_line with respect to their new
signature.  While displaying a line now, do not stop at the first
null byte.  Rather, display the zero byte as a space and keep
going until we reach the size of the line.
* Makefile.in: Add vec.o to OBJS-libcommon

gcc/testsuite/ChangeLog:

* c-c++-common/cpp/warning-zero-in-literals-1.c: New test file.

git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@204453 
138bc75d-0d04-0410-961f-82ee72b054a4

Signed-off-by: Dodji Seketeli do...@seketeli.org
---
 gcc/Makefile.in|   3 +-
 gcc/diagnostic.c   |  19 +-
 gcc/diagnostic.h   |   1 +
 gcc/input.c| 633 -
 gcc/input.h|   5 +-
 .../c-c++-common/cpp/warning-zero-in-literals-1.c  | Bin 0 - 240 bytes
 libcpp/include/line-map.h  |   8 +
 libcpp/line-map.c  |  40 ++
 8 files changed, 670 insertions(+), 39 deletions(-)
 create mode 100644 gcc/testsuite/c-c++-common/cpp/warning-zero-in-literals-1.c

diff --git a/gcc/Makefile.in b/gcc/Makefile.in
index 4d683a0..06c617a 100644
--- a/gcc/Makefile.in
+++ b/gcc/Makefile.in
@@ -1472,7 +1472,8 @@ OBJS = \
 
 # Objects in libcommon.a, potentially used by all host binaries and with
 # no target dependencies.
-OBJS-libcommon = diagnostic.o diagnostic-color.o pretty-print.o intl.o input.o 
version.o
+OBJS-libcommon = diagnostic.o diagnostic-color.o pretty-print.o intl.o \
+   vec.o  input.o version.o
 
 # Objects in libcommon-target.a, used by drivers and by the core
 # compiler and containing target-dependent code.
diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c
index 36094a1..6c83f03 100644
--- a/gcc/diagnostic.c
+++ b/gcc/diagnostic.c
@@ -176,6 +176,8 @@ diagnostic_finish (diagnostic_context *context)
 progname);
   pp_newline_and_flush (context-printer);
 }
+
+  diagnostic_file_cache_fini ();
 }
 
 /* Initialize DIAGNOSTIC, where the message MSG has already been
@@ -259,12 +261,13 @@ diagnostic_build_prefix (diagnostic_context *context,
MAX_WIDTH by some margin, then adjust the start of the line such
that the COLUMN is smaller than MAX_WIDTH minus the margin.  The
margin is either 10 characters or the difference between the column
-   and the length of the line, whatever is smaller.  */
+   and the length of the line, whatever is smaller.  The length of
+   LINE is given by LINE_WIDTH.  */
 static const char *
-adjust_line (const char *line, int max_width, int *column_p)
+adjust_line (const char *line, int line_width,
+int max_width, int *column_p)
 {
   int right_margin = 10;
-  int line_width = strlen (line);
   int column = *column_p;
 
   right_margin = MIN (line_width - column, right_margin);
@@ -284,6 +287,7 @@ diagnostic_show_locus (diagnostic_context * context,
   const diagnostic_info *diagnostic)
 {
   const char *line;
+  int line_width;
   char

Re: [PATCH] Allow building if libsanitizer on RHEL5 (i.e. with 2.6.18-ish kernel headers, take 2)

[Dodji Seketeli]

Jakub Jelinek ja...@redhat.com writes:

 Hi!

 Here is an updated version which doesn't warn about #include_next.
 Ok for trunk?

 2013-12-10  Jakub Jelinek  ja...@redhat.com

   * sanitizer_common/Makefile.am (AM_CPPFLAGS): Add
   -isystem $(top_srcdir)/include/system.
   * sanitizer_common/Makefile.in: Regenerated.
   * include/system/linux/aio_abi.h: New header.
   * include/system/linux/mroute.h: New header.
   * include/system/linux/mroute6.h: New header.
   * include/system/linux/perf_event.h: New header.
   * include/system/linux/types.h: New header.

This looks good to me.  A better approach would have been what you said
in another thread:

| much better would be just say a testcase that would include the
| sanitizer + kernel headers, guarded by recent enough LINUX_VERSION_CODE
| or configure or similar, so it wouldn't prevent library build on older
| kernel headers, the kernel ABI better be stable (only new things added,
| not size of structures/magic constants etc. changed from time to time).

But because of:

| But Kostya is apparently not willing to do that, so this patch
| provides a workaround in non-compiler-rt maintained files.

Let's get this in then :-)

Cheers.

-- 
Dodji

Re: [PATCH] Use libbacktrace for libsanitizer symbolization (take 2, PR sanitizer/59136)

[Dodji Seketeli]

Jakub Jelinek ja...@redhat.com writes:

 This is a second attempt at libsanitizer symbolization using
 libbacktrace.  The compiler-rt maintained bit have been
 already added by the recent merge from compiler-rt, so this
 patch is mostly configury/Makefile stuff.  Rather than using
 libbacktrace.la built in libbacktrace directory directly this
 patch builds libsanitizer's own copy of a subset of libbacktrace
 that it actually needs (only everything required for
 backtrace_{{sym,pc}info,create_state}),

OK.

 renames the symbols to __asan_backtrace_* so that when it is
 e.g. through -static-libasan etc. it doesn't clash with user symbols
 or other projects using libbacktrace (and, as libasan isn't yet symbol
 versioned, also doesn't export backtrace_* symbols from the DSO).

So we are carrying a light fork for libbacktrace then.  I am little
bit concerned about the maintenance cost of this over time.  I guess we
can figure out a way to factorize libbacktrace if the cost of its
maintenance really rises.


 Regtested on x86_64-linux (--target_board=unix\{-m32,-m64\}), ok for
 trunk (will do full bootstrap/regtest momentarily)?

Looks good to me.

Thank you.

-- 
Dodji

Re: [PATCH] libsanitizer demangling using cp-demangle.c

[Dodji Seketeli]

Jakub Jelinek ja...@redhat.com a écrit:



 2013-12-10  Jakub Jelinek  ja...@redhat.com

   * sanitizer_common/sanitizer_symbolizer_libbacktrace.h
   (LibbacktraceSymbolizer::Demangle): New declaration.
   * sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc
   (POSIXSymbolizer::Demangle): Use libbacktrace_symbolizer_'s Demangle
   method if possible.
   * sanitizer_common/sanitizer_symbolizer_libbacktrace.cc: Include
   demangle.h if SANITIZE_CP_DEMANGLE is defined.
   (struct CplusV3DemangleData): New type.
   (CplusV3DemangleCallback, CplusV3Demangle): New functions.
   (SymbolizeCodePCInfoCallback, SymbolizeCodeCallback,
   SymbolizeDataCallback): Use CplusV3Demangle.
   * sanitizer_common/Makefile.am (AM_CXXFLAGS): Add
   -DSANITIZE_CP_DEMANGLE and -I $(top_srcdir)/../include.
   * libbacktrace/backtrace-rename.h (cplus_demangle_builtin_types,
   cplus_demangle_fill_ctor, cplus_demangle_fill_dtor,
   cplus_demangle_fill_extended_operator, cplus_demangle_fill_name,
   cplus_demangle_init_info, cplus_demangle_mangled_name,
   cplus_demangle_operators, cplus_demangle_print,
   cplus_demangle_print_callback, cplus_demangle_type, cplus_demangle_v3,
   cplus_demangle_v3_callback, is_gnu_v3_mangled_ctor,
   is_gnu_v3_mangled_dtor, java_demangle_v3, java_demangle_v3_callback):
   Define.
   (__asan_internal_memcmp, __asan_internal_strncmp): New prototypes.
   (memcmp, strncmp): Redefine.
   * libbacktrace/Makefile.am (libsanitizer_libbacktrace_la_SOURCES): Add
   ../../libiberty/cp-demangle.c.
   * libbacktrace/bridge.cc (__asan_internal_memcmp,
   __asan_internal_strncmp): New functions.
   * sanitizer_common/Makefile.in: Regenerated.
   * libbacktrace/Makefile.in: Regenerated.
   * configure: Regenerated.
   * configure.ac: Regenerated.
   * config.h.in: Regenerated.

This looks good to me.

Thanks.

-- 
Dodji

Re: [PATCH] Put a breakpoint on __asan_report_error for ASAN

[Dodji Seketeli]

H.J. Lu hjl.to...@gmail.com a écrit:

 2012-11-24  H.J. Lu  hongjiu...@intel.com

 * configure.ac: Append gdbasan.in to .gdbinit if CFLAGS contains
 -fsanitize=address.
 * configure: Regenerated.

 * gdbasan.in: New file.

This is OK, if nobody objects in the next 48h.

Thanks.

-- 
Dodji

Re: [PATCH] Asan constructor init order checking

[Dodji Seketeli]

Hello,

Jakub Jelinek ja...@redhat.com writes:

 --- gcc/cgraph.h.jj   2013-11-13 18:32:52.0 +0100
 +++ gcc/cgraph.h  2013-11-15 12:05:25.950985500 +0100
 @@ -520,6 +520,11 @@ class GTY((tag (SYMTAB_VARIABLE))) var
  public:
/* Set when variable is scheduled to be assembled.  */
unsigned output : 1;
 +  /* Set if the variable is dynamically initialized.  Not set for
 + function local statics or variables that can be initialized in
 + multiple compilation units (such as template static data members
 + that need construction).  */
 +  unsigned asan_dynamically_initialized : 1;
  };

Maybe this could just be called dynamically_initialized?  It's just used
by asan today, but it looks like an information that could be used more
generally, independently from asan.

  
/* If we're using __cxa_atexit, register a function that calls the
destructor for the object.  */
 @@ -3498,6 +3507,9 @@ do_static_initialization_or_destruction
tf_warning_or_error);
finish_if_stmt_cond (cond, init_if_stmt);
  
 +  if (flag_sanitize  SANITIZE_ADDRESS)
 +finish_expr_stmt (asan_dynamic_init_call (/*after_p=*/false));
 +

I guess this spot could use some comment referring to the comment of
asan_globals.cc:__asan_before_dynamic_init from libsanitizer.  Basically
saying that we are emitting a call to __asan_before_dynamic_init to
poison all dynamically initialized global variables not defined in this
TU, so that a dynamic initializer for a global variable is only allowed
to touch the global variables from this current TU.  This comment could
be valuable when chasing a bug about this a couple of months from now
when we forget about how this works again.

And then, similarly ...

 @@ -3546,6 +3558,9 @@ do_static_initialization_or_destruction
  
} while (node);
  
 +  if (flag_sanitize  SANITIZE_ADDRESS)
 +finish_expr_stmt (asan_dynamic_init_call (/*after_p=*/true));
 +

... this spot could also use some comment referring to the comment of
asan_global.cc:__asan_after_dynamic_init, saying that because the
initializers of globals must have run by now (they are emitted by
one_static_initialization_or_destruction that has been invoked before
this point and after the point above) we are un-poisoning all
dynamically initialized global variables.

Also, do we have some tests for this?  I am not sure how I'd write
multi-tu dejagnu tests for this myself though ;-)

Other than that, LGTM.

Thanks.

-- 
Dodji

Re: [PATCH] Asan constructor init order checking

[Dodji Seketeli]

Dodji Seketeli do...@redhat.com writes:

 Also, do we have some tests for this?  I am not sure how I'd write
 multi-tu dejagnu tests for this myself though ;-)

Woops, I have just seen the sub-thread about the tests with Konstantin,
you and Alexey.  Sorry.

Cheers.

-- 
Dodji

Re: [PATCH] Asan constructor init order checking

[Dodji Seketeli]

Jakub Jelinek ja...@redhat.com writes:

 On Fri, Nov 22, 2013 at 04:38:58PM +0100, Dodji Seketeli wrote:
 Jakub Jelinek ja...@redhat.com writes:
 
  --- gcc/cgraph.h.jj2013-11-13 18:32:52.0 +0100
  +++ gcc/cgraph.h   2013-11-15 12:05:25.950985500 +0100
  @@ -520,6 +520,11 @@ class GTY((tag (SYMTAB_VARIABLE))) var
   public:
 /* Set when variable is scheduled to be assembled.  */
 unsigned output : 1;
  +  /* Set if the variable is dynamically initialized.  Not set for
  + function local statics or variables that can be initialized in
  + multiple compilation units (such as template static data members
  + that need construction).  */
  +  unsigned asan_dynamically_initialized : 1;
   };
 
 Maybe this could just be called dynamically_initialized?  It's just used
 by asan today, but it looks like an information that could be used more
 generally, independently from asan.

 I used that name initially, but then changed it, because it actually is
 quite asan specific.  E.g. template static data members are dynamically
 initialized, but we intentionally don't set asan_dynamically_initialized
 on those, because their initializer can be called from multiple CUs, there
 is no CU that owns the variable.

I see.  OK, I don't feel strongly about this anyway.  Thanks for the
clarification.


 /* If we're using __cxa_atexit, register a function that calls the
  destructor for the object.  */
  @@ -3498,6 +3507,9 @@ do_static_initialization_or_destruction
  tf_warning_or_error);
 finish_if_stmt_cond (cond, init_if_stmt);
   
  +  if (flag_sanitize  SANITIZE_ADDRESS)
  +finish_expr_stmt (asan_dynamic_init_call (/*after_p=*/false));
  +

 Will add the comments.

 Also, do we have some tests for this?  I am not sure how I'd write
 multi-tu dejagnu tests for this myself though ;-)

 I've postponed tests for stage3, when I find spare time I'll try to
 port libsanitizer tests that are applicable to gcc over to dejagnu,
 plus perhaps add new tests as needed.

OK, thanks.

The patch is OK to commit, as far as I am concerned.

Thanks.

-- 
Dodji

Re: [PATCH] Support -fsanitize=leak

[Dodji Seketeli]

Jakub Jelinek ja...@redhat.com writes:

 This patch adds support for -fsanitize=leak and -static-liblsan options.
 If combined with -fsanitize=address, it does nothing,

From this hunk:

@@ -8123,7 +8133,10 @@ sanitize_spec_function (int argc, const
 return (flag_sanitize  SANITIZE_THREAD) ?  : NULL;
   if (strcmp (argv[0], undefined) == 0)
 return (flag_sanitize  SANITIZE_UNDEFINED) ?  : NULL;
-
+  if (strcmp (argv[0], leak) == 0)
+return ((flag_sanitize
+ (SANITIZE_ADDRESS | SANITIZE_LEAK | SANITIZE_THREAD))
+   == SANITIZE_LEAK) ?  : NULL;
   return NULL;
 }

I'd say if combined with -fsanitize={address,thread} it does nothing,
right?

I'd say this needs some tests as well, but I guess they are coming a
bit later in the cycle?

[...]

 On Fri, Nov 15, 2013 at 12:34:14PM -0800, Ian Lance Taylor wrote:

 Documentation?

 Here it is, as incremental patch:

 2013-11-18  Jakub Jelinek  ja...@redhat.com

   * doc/invoke.texi (-static-liblsan, -fsanitize=leak): Document.

[...]


 Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Yes, this is OK with the incremental patch for the documentation.

Thanks.

-- 
Dodji

Re: [patch] Fix ICEs when DEBUG_MANGLE is enabled

[Dodji Seketeli]

I guess we should CC Jason for this ...

ccout...@google.com (Cary Coutant) a écrit:

This patch fixes a few ICEs I encountered when enabling DEBUG_MANGLE.
I've also changed dump_substitution_candidates to output the substitution
index in base 36, to match the actual mangled name.

OK for trunk?

-cary


2013-11-13  Cary Coutant  ccout...@google.com

gcc/
* cp/mangle.c (to_base36): New function.
(dump_substitution_candidates): Add checks for NULL.
Print substitution index in base 36.


commit 5ece725d55f104dd6499ac261380a9c9c4002613
Author: Cary Coutant ccout...@google.com
Date:   Wed Nov 13 09:28:58 2013 -0800

Fix ICEs when DEBUG_MANGLE is enabled.

diff --git a/gcc/cp/mangle.c b/gcc/cp/mangle.c
index 202fafc..56c1844 100644
--- a/gcc/cp/mangle.c
+++ b/gcc/cp/mangle.c
@@ -301,6 +301,19 @@ decl_is_template_id (const tree decl, tree* const 
template_info)
   return 0;
 }
 
+/* Convert VAL to base 36.  */
+
+static const char *
+to_base36 (int val)
+{
+  static char buffer[sizeof (HOST_WIDE_INT) * 8 + 1];
+  unsigned count;
+
+  count = hwint_to_ascii (number, 36, buffer + sizeof (buffer) - 1, 1);
+  buffer[sizeof (buffer) - 1] = '\0';
+  return buffer + sizeof (buffer) - 1 - count;
+}
+
 /* Produce debugging output of current substitution candidates.  */
 
 static void
@@ -317,12 +330,27 @@ dump_substitution_candidates (void)
   if (i  0)
fprintf (stderr, );
   if (DECL_P (el))
-   name = IDENTIFIER_POINTER (DECL_NAME (el));
+{
+  if (DECL_NAME (el))
+name = IDENTIFIER_POINTER (DECL_NAME (el));
+}
   else if (TREE_CODE (el) == TREE_LIST)
-   name = IDENTIFIER_POINTER (DECL_NAME (TREE_VALUE (el)));
+{
+  tree val = TREE_VALUE (el);
+  if (TREE_CODE (val) == IDENTIFIER_NODE)
+name = IDENTIFIER_POINTER (val);
+  else if (DECL_P (val))
+name = IDENTIFIER_POINTER (DECL_NAME (val));
+}
   else if (TYPE_NAME (el))
-   name = IDENTIFIER_POINTER (DECL_NAME (TYPE_NAME (el)));
-  fprintf (stderr,  S%d_ = , i - 1);
+{
+  if (DECL_NAME (TYPE_NAME (el)))
+name = IDENTIFIER_POINTER (DECL_NAME (TYPE_NAME (el)));
+}
+  if (i == 0)
+fprintf (stderr,  S_ = );
+  else
+fprintf (stderr,  S%s_ = , to_base36 (i - 1));
   if (TYPE_P (el) 
  (CP_TYPE_RESTRICT_P (el)
   || CP_TYPE_VOLATILE_P (el)

-- 
Dodji

Re: [PATCH] preprocessor/58580 - preprocessor goes OOM with warning for zero literals

[Dodji Seketeli]

Jakub Jelinek ja...@redhat.com writes:

 On Tue, Nov 12, 2013 at 04:33:41PM +0100, Dodji Seketeli wrote:
 +
 +  memmove (*line, l, len);
 +  (*line)[len - 1] = '\0';
 +  *line_len = --len;

 Shouldn't this be testing that len  0  (*line)[len - 1] == '\n'
 first before you decide to overwrite it and decrement len?

That code above is in a if (len  0) block.  So checking that condition
again is not necessary.  Also, I think we don't need to test there is a
terminal '\n' at the end because get_next_line always return the line
content followed either by a '\n' or by a junk byte that is right
after the last byte of the file -- in case we reach end of file w/o
seeing a '\n'.

 Though in that case there would be no '\0' termination of the string
 for files not ending in a new-line.  So, either get_next_line should
 append '\n' to the buffer, or you should have there space for that, or
 you can't rely on zero termination of the string and need to use just
 the length.

OK, I am settling for doing away with the '\0' altogether.

The patch below makes get_next_line always point to the last character
of the line before the '\n' when it is present.  So '\n' is never
counted int the string.  I guess that's less confusing to people.

Tested on x86_64-unknown-linux-gnu against trunk.

libcpp/ChangeLog:

* include/line-map.h (linemap_get_file_highest_location): Declare
new function.
* line-map.c (linemap_get_file_highest_location): Define it.

gcc/ChangeLog:

* input.h (location_get_source_line): Take an additional line_size
parameter.
(void diagnostics_file_cache_fini): Declare new function.
* input.c (struct fcache): New type.
(fcache_tab_size, fcache_buffer_size, fcache_line_record_size):
New static constants.
(diagnostic_file_cache_init, total_lines_num)
(lookup_file_in_cache_tab, evicted_cache_tab_entry)
(add_file_to_cache_tab, lookup_or_add_file_to_cache_tab)
(needs_read, needs_grow, maybe_grow, read_data, maybe_read_data)
(get_next_line, read_next_line, goto_next_line, read_line_num):
New static function definitions.
(diagnostic_file_cache_fini): New function.
(location_get_source_line): Take an additional output line_len
parameter.  Re-write using lookup_or_add_file_to_cache_tab and
read_line_num.
* diagnostic.c (diagnostic_finish): Call
diagnostic_file_cache_fini.
(adjust_line): Take an additional input parameter for the length
of the line, rather than calculating it with strlen.
(diagnostic_show_locus): Adjust the use of
location_get_source_line and adjust_line with respect to their new
signature.  While displaying a line now, do not stop at the first
null byte.  Rather, display the zero byte as a space and keep
going until we reach the size of the line.
* Makefile.in: Add vec.o to OBJS-libcommon

gcc/testsuite/ChangeLog:

* c-c++-common/cpp/warning-zero-in-literals-1.c: New test file.

git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@204453 
138bc75d-0d04-0410-961f-82ee72b054a4
---
 gcc/Makefile.in|   3 +-
 gcc/diagnostic.c   |  19 +-
 gcc/diagnostic.h   |   1 +
 gcc/input.c| 633 -
 gcc/input.h|   5 +-
 .../c-c++-common/cpp/warning-zero-in-literals-1.c  | Bin 0 - 240 bytes
 libcpp/include/line-map.h  |   8 +
 libcpp/line-map.c  |  40 ++
 8 files changed, 670 insertions(+), 39 deletions(-)
 create mode 100644 gcc/testsuite/c-c++-common/cpp/warning-zero-in-literals-1.c

diff --git a/gcc/Makefile.in b/gcc/Makefile.in
index 49285e5..9fe9060 100644
--- a/gcc/Makefile.in
+++ b/gcc/Makefile.in
@@ -1469,7 +1469,8 @@ OBJS = \
 
 # Objects in libcommon.a, potentially used by all host binaries and with
 # no target dependencies.
-OBJS-libcommon = diagnostic.o diagnostic-color.o pretty-print.o intl.o input.o 
version.o
+OBJS-libcommon = diagnostic.o diagnostic-color.o pretty-print.o intl.o \
+   vec.o  input.o version.o
 
 # Objects in libcommon-target.a, used by drivers and by the core
 # compiler and containing target-dependent code.
diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c
index 36094a1..6c83f03 100644
--- a/gcc/diagnostic.c
+++ b/gcc/diagnostic.c
@@ -176,6 +176,8 @@ diagnostic_finish (diagnostic_context *context)
 progname);
   pp_newline_and_flush (context-printer);
 }
+
+  diagnostic_file_cache_fini ();
 }
 
 /* Initialize DIAGNOSTIC, where the message MSG has already been
@@ -259,12 +261,13 @@ diagnostic_build_prefix (diagnostic_context *context,
MAX_WIDTH by some margin, then adjust the start of the line such
that the COLUMN is smaller than MAX_WIDTH minus the margin

Re: [PATCH] preprocessor/58580 - preprocessor goes OOM with warning for zero literals

[Dodji Seketeli]

Bernd Edlinger bernd.edlin...@hotmail.de writes:

 Using -- on a value that goes out of scope looks
 awkward IMHO.

 I don't understand this sentence. What do you mean by Using -- on a
 value that goes out of scope?


 I meant the operator --  in  *line_len = --len;

Sorry, I don't see how that is an issue.  This looks like a classical
way of passing an output parameter to me.

 Maybe, You could also avoid the copying completely, if you just hand out
 a pointer to the line buffer as const char*, and use the length instead of the
 nul-char as end delimiter ?

I thought about avoiding the copying of course.  But the issue with that
is that that ties the lifetime of the returned line to the time between
two invocations of read_next_line.  IOW, you'd have to use the line
quickly before calling read_next_line again.  Actually that
non-copying API that you are talking about exists in the patch; it's
get_next_line.  And you see that it's what we use when we want to avoid
the copying, e.g, in goto_next_line.  But when we want to give the
final user the string, I believe that copying is less surprising.  And
from what I could see from the tests I have done, the copying doesn't
make the thing slower than without the patch.  So I'd like to keep this
unless folks have very strong feeling about it.

-- 
Dodji

Re: [PATCH] preprocessor/58580 - preprocessor goes OOM with warning for zero literals

[Dodji Seketeli]

Sorry, I missed one question in the previous email.

Bernd Edlinger bernd.edlin...@hotmail.de writes:

 and what is it if there is no terminal '\n' ?

In that case it's that the entire file is made of one line.  For that
case get_next_line has allocated enough space for one
byte-passed-the-end of the file, so that there is no buffer overflow
here.

-- 
Dodji