Re: Rust frontend patches v1

[Manuel López-Ibáñez via Gcc-patches]

Dear Philip,

Another thing to pay attention to is the move to Sphinx for documentation:
https://gcc.gnu.org/pipermail/gcc/2022-August/239233.html

Best,

Manuel.

On Wed, 10 Aug 2022 at 20:57, Philip Herron 
wrote:

> Hi everyone
>
> For my v2 of the patches, I've been spending a lot of time ensuring
> each patch is buildable. It would end up being simpler if it was
> possible if each patch did not have to be like this so I could split
> up the front-end in more patches. Does this make sense? In theory,
> when everything goes well, does this still mean that we can merge in
> one commit, or should it follow a series of buildable patches? I've
> received feedback that it might be possible to ignore making each
> patch an independent chunk and just focus on splitting it up as small
> as possible even if they don't build.
>
> I hope this makes sense.
>
> Thanks
>
> --Phil
>
> On Thu, 28 Jul 2022 at 10:39, Philip Herron 
> wrote:
> >
> > Thanks, for confirming David. I think it was too big in the end. I was
> > trying to figure out how to actually split that up but it seems
> > reasonable that I can split up the front-end patches into patches for
> > each separate pass in the compiler seems like a reasonable approach
> > for now.
> >
> > --Phil
> >
> > On Wed, 27 Jul 2022 at 17:45, David Malcolm  wrote:
> > >
> > > On Wed, 2022-07-27 at 14:40 +0100, herron.philip--- via Gcc-patches
> > > wrote:
> > > > This is the initial version 1 patch set for the Rust front-end. There
> > > > are more changes that need to be extracted out for all the target
> > > > hooks we have implemented. The goal is to see if we are implementing
> > > > the target hooks information for x86 and arm. We have more patches
> > > > for the other targets I can add in here but they all follow the
> > > > pattern established here.
> > > >
> > > > Each patch is buildable on its own and rebased ontop of
> > > > 718cf8d0bd32689192200d2156722167fd21a647. As for ensuring we keep
> > > > attribution for all the patches we have received in the front-end
> > > > should we create a CONTRIBUTOR's file inside the front-end folder?
> > > >
> > > > Note thanks to Thomas Schwinge and Mark Wielaard, we are keeping a
> > > > branch up to date with our code on:
> > > >
> https://gcc.gnu.org/git/?p=gcc.git;a=shortlog;h=refs/heads/devel/rust/master
> > > >  but this is not rebased ontop of gcc head.
> > > >
> > > > Let me know if I have sent these patches correctly or not, this is a
> > > > learning experience with git send-email.
> > > >
> > > > [PATCH Rust front-end v1 1/4] Add skeleton Rust front-end folder
> > > > [PATCH Rust front-end v1 2/4] Add Rust lang TargetHooks for i386 and
> > > > [PATCH Rust front-end v1 3/4] Add Rust target hooks to ARM
> > > > [PATCH Rust front-end v1 4/4] Add Rust front-end and associated
> > >
> > > FWIW it looks like patch 4 of the kit didn't make it (I didn't get a
> > > copy and I don't see it in the archives).
> > >
> > > Maybe it exceeded a size limit?  If so, maybe try splitting it up into
> > > more patches.
> > >
> > > Dave
> > >
>

Re: [PATCH 2/2] Documentation hyperlinks for [-Wname-of-option] (PR 87488)

[Manuel López-Ibáñez]

Hi David,

While I agree that this is quite cool to have, the following:


+/* DOCUMENTATION_ROOT_URL should be supplied via -D by the Makefile
+   (see --with-documentation-root-url).
+
+   Expect an anchor of the form "index-Wfoo" e.g.
+   , and thus an id within
+   the URL of "#index-Wformat".  */
+return concat (DOCUMENTATION_ROOT_URL,
+  "Warning-Options.html",
+  "#index", cl_options[option_index].opt_text,
+  NULL);


will not work for many -W options:

https://gcc.gnu.org/onlinedocs/gcc/C_002b_002b-Dialect-Options.html#C_002b_002b-Dialect-Options
(scroll to the bottom)

https://gcc.gnu.org/onlinedocs/gcc/Objective-C-and-Objective-C_002b_002b-Dialect-Options.html#Objective-C-and-Objective-C_002b_002b-Dialect-Options
(scroll to the bottom)

https://gcc.gnu.org/onlinedocs/gcc-9.2.0/cpp/Invocation.html#index-Wcomment

https://gcc.gnu.org/onlinedocs/gcc-9.2.0/gfortran/Error-and-Warning-Options.html#Error-and-Warning-Options

I would argue that some options are documented in the wrong place (I think all 
C/C++ -W options should just go to Warning-Options.html), but I also believe 
the HTML page should be language dependent.


Cheers,

Manuel.

Re: [PATCH] ux.texi: move "Quoting" and "Fix-it hints" from DiagnosticsGuidelines wiki page

[Manuel López-Ibáñez]

On Wed, 24 Oct 2018, 17:39 David Malcolm,  wrote:

> Manu: are you wiki user "ManuelLopezIbanez", and are you happy to have
> any/all of your gcc wiki edits copied into gcc itself, covered under
> the usual FSF copyright assignment?
>

I'm wiki user "ManuelLopezIbanez". I believe nothing I have contributed to
the GCC wiki is an original work, since it is public domain information
about the workings of GCC collected from public discussions in the mailing
lists or derived from GCC itself. I contributed to the GCC wiki under the
assumption that all my contributions are in the public domain, except when
noted otherwise.

Best,

Manuel.

Re: [PATCH] handle attribute positional arguments consistently (PR 87541, 87542)

[Manuel López-Ibáñez]

On 07/10/18 23:38, Martin Sebor wrote:

+  pretty_printer posval;
+  if (pos != error_mark_node)
+{
+  /* Only format the position value when it's valid.  By convention
+do not quote constant integers.  */
+  pp_space ();
+  if (TREE_CODE (pos) != INTEGER_CST)
+   pp_begin_quote (, pp_show_color (global_dc->printer));
+  dump_generic_node (, pos, 0, TDF_NONE, false);
+  if (TREE_CODE (pos) != INTEGER_CST)
+   pp_end_quote (, pp_show_color (global_dc->printer));
+}


Sorry for the bike-shedding but is this really necessary?

First, you handle != INTEGER_CST separately, so you can simply use %qE for that 
case and %E for the rest. Nevertheless, I think the convention is 
(https://gcc.gnu.org/wiki/DiagnosticsGuidelines#Quoting): "elements such as 
numbers that do no refer to numeric constants that appear in the source code 
should not be quoted". Since this is a integer constant that appears in the 
source code, then it should be quoted.


Also, "value%s" where %s can be empty, will not translate correctly.

Perhaps you need a small function:

warn_attributes(const char * msg_no_value_no_arg,
const char * msg_no_value_arg,
const char * msg_value_no_arg,
const char * msg_value_arg,
tree atname, int argno, tree pos, ...)



+  if (TREE_CODE (pos) != INTEGER_CST)
+{
+  /* Only mention the argument number when it's non-zero.  */
+  if (argno < 1)
+   warning (OPT_Wattributes,
+"%qE attribute argument value%s is not an integer "
+"constant",
+atname, pp_formatted_text ());
+  else
+   warning (OPT_Wattributes,
+"%qE attribute argument %i value%s is not an integer "
+"constant",
+atname, argno, pp_formatted_text ());
+   
+  return NULL_TREE;
+}


So that in the code above you can say:

if (TREE_CODE (pos) != INTEGER_CST)
{
warn_attributes("%qE attribute argument value is not an integer",
"%qE attribute argument %i value is not an integer",
"%qE attribute argument value %qE is not an integer",
"%qE attribute argument %i value %qE is not an integer",
 atname, argno, pos);
return NULL_TREE;
}

Also, I wonder where input_location is pointing at for these warnings. There 
may be a better location. Clang is doing:


:5:1: error: 'alloc_align' attribute requires parameter 1 to be an 
integer constant

ALIGN ("1") void*
^  ~~~
:1:36: note: expanded from macro 'ALIGN'
#define ALIGN(N)   __attribute__ ((alloc_align (N)))
   ^~

while GCC does:

:6:16: warning: alloc_align parameter outside range [-Wattributes]
6 | fpvi_str_1 (int);
  |^

Apart from the above, this seems a major improvement, so I hope it goes in.

Cheers,

Manuel.

Re: [committed] diagnostics: add line numbers to source (PR other/84889)

[Manuel López-Ibáñez]

On 17/08/18 17:50, Andreas Schwab wrote:

On Aug 17 2018, Manuel López-Ibáñez  wrote:


However, I see that GCC trunk still counts tabs as 1-column, probably
because emacs counts tabs as one column when interpreting column numbers
in the output of GCC.


That is not true.  Emacs is using screen columns by default for almost
20 years now (see compilation-error-screen-columns).


Maybe I didn't properly explain what I mean. I mean that Emacs counts a tab as 
a "GCC" column, whatever number of visual columns the tab is configured to be 
in Emacs. The following code (with a tab before 'a'):


/* ñ/* */
/* a/* */
a

With gcc -c test.c -Wcomment, it gives:

test.c:1:10: warning: "/*" within comment [-Wcomment]
 /* ñ/* */
  ^
test.c:2:9: warning: "/*" within comment [-Wcomment]
 /* a/* */
 ^
test.c:3:2: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ at end of 
input
  a
  ^

When the above appears in a compilation-mode buffer, I can click (or press 
enter) on each diagnostic and Emacs will jump exactly to what is pointed by the 
"^". That is, for Emacs, 3:2 does not mean line 3, *visual* column 2. In my 
Emacs, it jumps to column 8 (where the "a" is).


Changing the output to be 3:9 (if GCC starts interpreting tabs as 8 spaces as 
recommended by the GCS or to the value given by -ftabstop) will make Emacs jump 
to the wrong place.


This is independent of multi-byte characters. GCC is pointing to the wrong 
place when the line contains "ñ".


I hope the above is clearer,

Manuel.

Re: [committed] diagnostics: add line numbers to source (PR other/84889)

[Manuel López-Ibáñez]

On 09/08/18 21:09, David Malcolm wrote:


It turns out that we convert tab characters to *single* space
characters when printing source code.

This behavior has been present since Manu first implemented
-fdiagnostics-show-caret in r186305 (aka
5a9830842f69ebb059061e26f8b0699cbd85121e, PR 24985), where it was this
logic (there in diagnostic.c's diagnostic_show_locus):
   char c = *line == '\t' ? ' ' : *line;
   pp_character (context->printer, c);

(that logic is now in diagnostic-show-locus.c in
layout::print_source_line)

Arguably this is a bug, but it's intimately linked to the way in which
we track "column numbers".  Our "column numbers" are currently simply a
1-based byte-count, I believe, so a tab character is treated by us as
simply an increment of 1 right now.  There are similar issues with
multibyte characters, which are being tracked in PR 49973.



Hi David,

At the time, this was done on purpose for two reasons:

1) The way we counted column numbers already counted tabs as 1-space and ...
2) It leads to wasting less horizontal space and more consistent output.

I believe that (1) was due to this bug: 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52899 which got fixed.


The GCS says that column numbers should count tab stops as 8 spaces 
[https://www.gnu.org/prep/standards/html_node/Errors.html].

I believe that -ftabstop=8 is the default after PR52899 was fixed.

However, I see that GCC trunk still counts tabs as 1-column, probably because 
emacs counts tabs as one column when interpreting column numbers in the output 
of GCC. As long as both of these things are true, I believe it doesn't make 
much sense to print 8 spaces (or a tab) instead of a 1-column space. It will 
make interpreting the column numbers much harder and break the parsing of GCC 
diagnostics done by emacs.


Note that if we print the tab directly, the width of the tab in the terminal 
may not be the same as in the editor the user is using. Moreover, if the user 
is using tabs consistently (instead of using tabs on some lines and spaces in 
others), replacing tabs with 1 space will only reduce the visual space per 
indentation level, but the indentation structure will remain consistent.


I wish I had added a summary of the above to the code as a comment.

Finally, PR49973 is about GCC counting multiple columns for characters that 
should be counted as one column. This should be fixed in our line-map 
implementation using wcwidth() when lexing. It is not the same issue at all. 
Once column numbers are correctly counted, the output should be fine as well 
(the caret line does not change multi-byte characters).


I hope the above helps,

Manuel.

Re: [PATCH] adjust warning_n() to take uhwi (PR 84207)

[Manuel López-Ibáñez]

On 14 Feb 2018 8:16 pm, "Pedro Alves"  wrote:

Instead of a class that has to have a constructor for every type
you want to pass as plural selector to the _n functions, which
increases coupling, I'd suggest using a conversion function, and
overload that.  I.e., something like, in the core diagnostics code:

static inline unsigned HOST_WIDE_INT
as_plural_form (unsigned HOST_WIDE_INT val)
{
  return val;
}

/* In some tree-diagnostics header.  */

static inline unsigned HOST_WIDE_INT
as_plural_form (tree t)
{
   // extract & return a HWI
}

/* In some whatever-other-type-diagnostics header.  */

static inline unsigned HOST_WIDE_INT
as_plural_form (whatever_other_type v)
{
   // like above
}

and then you call error_n and other similar functions like this:

error_n (loc, u, "%u thing", "%u things", u);
error_n (loc, as_plural_form (u), "%u thing", "%u things", u);
error_n (loc, as_plural_form (t), "%E thing", "%E things", t);
error_n (loc, as_plural_form (i), "%wu thing", "%wu things", i);

This is similar in spirit to std::to_string, etc.


If that's desired, why not simply have GCC::to_uhwi() ? It would likely be
useful in other contexts.

Cheers,

Manuel.

Re: [PATCH] adjust warning_n() to take uhwi (PR 84207)

[Manuel López-Ibáñez]

On 13 Feb 2018 5:58 pm, "Martin Sebor"  wrote:


I wanted to make the _n() functions like warning_n() more
robust by letting them accept a tree argument (as well as
offset_int and wide_int) in addition to HOST_WIDE_INT but
I can't do it if they can't work with these types.


There must be a tree-diagnostics.c where you can add those functions and
then call the general diagnostic functions. Same for RTL.

Note that pretty-printer.c works in the same way.

Cheers,

Manuel.

Re: [PATCH] adjust warning_n() to take uhwi (PR 84207)

[Manuel López-Ibáñez]

On 13/02/18 03:10, Martin Sebor wrote:

PS Is there any reason why diagnostic-core.h and diagnostic.c
does not/should not include tree.h and other GCC headers?


The short reason is that we want to keep the core diagnostics as independent of 
the rest of the compiler as possible so that using it doesn't bring undesired 
dependencies. It took some effort a few years ago to clean-up headers, in 
particular, the mess of toplev.h, inclusion of rtl in FE files, inclusion of 
tree.h in back-end files, etc. I'm not sure what is the status now but I'm 
afraid that without any definition of actual internal/external interfaces and 
without actual physical separation of files and compilation steps (read: 
libraries), the headers may have crept back in.


Long time ago there was a plan of making GCC more modular, perhaps moving to a 
library framework like LLVM/Clang. I even had a patch that moved all core 
diagnostics and line-map stuff to its own libdiagnostic that was used by libcpp 
and the rest of the compiler (instead of overriding call-backs in libcpp like 
we do now and forcing libcpp into every executable that wishes to use 
diagnostics or line-maps). Unfortunately, it never worked properly because of 
the build machinery.


More background (probably very much outdated and forgotten):

https://docs.google.com/document/pub?id=1ZfyfkB62EFaR4_g4JKm4--guz3vxm9pciOBziMHTnK4

https://gcc.gnu.org/wiki/rearch

https://gcc.gnu.org/wiki/ModularGCC

Cheers,

Manuel.

Re: [PATCH][mingw] Enable colorized diagnostics

[Manuel López-Ibáñez]

On 10 Oct 2017 2:34 am, "Liu Hao"  wrote:

Since on *nix it is not when `colorize_start()` is called that the terminal
color is changed (it is when those ANSI escape codes are delivered to the
other peer which will translate them), and the string passed to `fputs()`
is free to deliver multiple escape codes, it is not an option unless we
output integral diagnostic messages using multiple fputs()` calls.

For example,
```
test.c:3:9: warning: 'a' is used uninitialized in this function
[-Wuninitialized]
```
The words 'warning' and '-Wuninitialized' should be magenta, so there are
four ANSI escape codes (two to set the color and another two to restore the
color), and this line of text must be output using five individual calls to
the `fputs()` function (one for each segment with the consistent color),
which is not the case (this whole line of text is delivered using a single
call), so all five segments have to be all in magenta or no color at all.
This is not a solution.


Ops! You're obviously right. What was I thinking?

I still believe that pretty-printer.c is not the right place for all this
color-handling code (diagnostic-color.c or libiberty/ may be better
places). Also, your code handles a lot more ANSI codes than those needed
for color output. The code in grep seems much simpler. Could your fputs
replacement split the string as you suggest above, then if the chunk is an
ANSI code, use grep's conversion function to transform the codes, otherwise
use fputs to print text.

Cheers,

Manuel.

Re: [PATCH][mingw] Enable colorized diagnostics

[Manuel López-Ibáñez]

On 09/10/17 23:25, Manuel López-Ibáñez wrote:
Even if the host-specific part is not done, I honestly think it is a good idea 
to match grep's code as much as possible since we may want to merge bugfixes 
between the two and eventually this code may end up in gnulib. Moreover, if 
somebody else implemented color output for another OS in grep, it would be very 
easy to transplant it to GCC (or viceversa) if the API remains close.


Something like the attached should do the trick (I didn't even try to compile 
it and completely untested, so it may need some adjustments).


Cheers,

Manuel.
Index: diagnostic-color.c
===
--- diagnostic-color.c	(revision 253569)
+++ diagnostic-color.c	(working copy)
@@ -19,6 +19,12 @@
 #include "config.h"
 #include "system.h"
 #include "diagnostic-color.h"
+#ifdef __MINGW32__
+#  undef DATADIR /* conflicts with objidl.h, which is included by windows.h */
+#  include 
+static HANDLE hstderr = INVALID_HANDLE_VALUE;
+static SHORT norm_attr;
+#endif
 
 /* Select Graphic Rendition (SGR, "\33[...m") strings.  */
 /* Also Erase in Line (EL) to Right ("\33[K") by default.  */
@@ -104,7 +110,125 @@
 #define SGR_SEQ(str)		SGR_START str SGR_END
 #define SGR_RESET		SGR_SEQ("")
 
+#ifdef __MINGW32__
+/* Convert a color spec, a semi-colon separated list of the form
+   SGR_START"NN;MM;KK;..."SGR_END, where each number is a value of the SGR
+   parameter, into the corresponding Windows console text attribute.
 
+   This function supports a subset of the SGR rendition aspects that
+   the Windows console can display.  */
+static int
+w32_sgr2attr (const char *sgr_seq)
+{
+  const char *s, *p;
+  int code, fg = norm_attr & 15, bg = norm_attr & (15 << 4);
+  int bright = 0, inverse = 0;
+  static const int fg_color[] = {
+0,			/* black */
+FOREGROUND_RED,	/* red */
+FOREGROUND_GREEN,	/* green */
+FOREGROUND_GREEN | FOREGROUND_RED, /* yellow */
+FOREGROUND_BLUE,		   /* blue */
+FOREGROUND_BLUE | FOREGROUND_RED,  /* magenta */
+FOREGROUND_BLUE | FOREGROUND_GREEN, /* cyan */
+FOREGROUND_RED | FOREGROUND_GREEN | FOREGROUND_BLUE /* gray */
+  };
+  static const int bg_color[] = {
+0,			/* black */
+BACKGROUND_RED,	/* red */
+BACKGROUND_GREEN,	/* green */
+BACKGROUND_GREEN | BACKGROUND_RED, /* yellow */
+BACKGROUND_BLUE,		   /* blue */
+BACKGROUND_BLUE | BACKGROUND_RED,  /* magenta */
+BACKGROUND_BLUE | BACKGROUND_GREEN, /* cyan */
+BACKGROUND_RED | BACKGROUND_GREEN | BACKGROUND_BLUE /* gray */
+  };
+
+  sgr_seq = sqr_seq + strlen(SGR_START);
+  
+  for (s = p = sgr_seq; strcmp(s, SGR_END) != 0; p++)
+{
+  if (*p == ';' || strcmp(p, SGR_END) == 0)
+{
+  code = strtol (s, NULL, 10);
+  s = p + (strcmp(p, SGR_END) != 0);
+
+  switch (code)
+{
+case 0:	/* all attributes off */
+  fg = norm_attr & 15;
+  bg = norm_attr & (15 << 4);
+  bright = 0;
+  inverse = 0;
+  break;
+case 1:	/* intensity on */
+  bright = 1;
+  break;
+case 7:	/* inverse video */
+  inverse = 1;
+  break;
+case 22:	/* intensity off */
+  bright = 0;
+  break;
+case 27:	/* inverse off */
+  inverse = 0;
+  break;
+case 30: case 31: case 32: case 33: /* foreground color */
+case 34: case 35: case 36: case 37:
+  fg = fg_color[code - 30];
+  break;
+case 39:	/* default foreground */
+  fg = norm_attr & 15;
+  break;
+case 40: case 41: case 42: case 43: /* background color */
+case 44: case 45: case 46: case 47:
+  bg = bg_color[code - 40];
+  break;
+case 49:	/* default background */
+  bg = norm_attr & (15 << 4);
+  break;
+default:
+  break;
+}
+}
+}
+  if (inverse)
+{
+  int t = fg;
+  fg = (bg >> 4);
+  bg = (t << 4);
+}
+  if (bright)
+fg |= FOREGROUND_INTENSITY;
+
+  return (bg & (15 << 4)) | (fg & 15);
+}
+
+/* Clear to the end of the current line with the default attribute.
+   This is needed for reasons similar to those that require the "EL to
+   Right after SGR" operation on Posix platforms: if we don't do this,
+   setting the 'mt', 'ms', or 'mc' capabilities to use a non-default
+   background color spills that color to the empty space at the end of
+   the last screen line in a match whose line spans multiple screen
+   lines.  */
+static void
+w32_clreol (void)
+{
+  DWORD nchars;
+  COORD start_pos;
+  DWORD written;
+  CONSOLE

Re: [PATCH][mingw] Enable colorized diagnostics

[Manuel López-Ibáñez]

On 08/10/17 12:39, Liu Hao wrote:

On 2017/9/28 4:09, Joseph Myers wrote:

On Thu, 28 Sep 2017, Liu Hao wrote:


Colorized diagnostics used to be disabled for MinGW targets (on which
the macro `_WIN32` is defined), and this patch enables it.


I'd hope this is all to do with MinGW host, and nothing to do with the
target.


Ping? Are there any more opinions about this?


For what is worth, the color output of GCC comes originally from grep, and grep 
does have code for colorizing in Windows: 
http://git.savannah.gnu.org/cgit/grep.git/tree/lib


and there are significant differences with this patch. For once,

  /* $TERM is not normally defined on DOS/Windows, so don't require
 it for highlighting.  But some programs, like Emacs, do define
 it when running Grep as a subprocess, so make sure they don't
 set TERM=dumb.  */
  char const *t = getenv ("TERM");
  return ! (t && strcmp (t, "dumb") == 0);

and they don't need a custom fputs() because their strategy is slightly 
different: They only override colorize_start (print_start_colorize) and 
colorize_stop (print_end_colorize) and convert ANSI sequences to W32 sequences 
on the fly. Thus, we wouldn't need to touch pretty-printer.c and all changes 
will be restricted to diagnostic-color.c (which could be split into -posix.c 
and -w32.c like grep does and be moved into host-specific config/ subdir).


Even if the host-specific part is not done, I honestly think it is a good idea 
to match grep's code as much as possible since we may want to merge bugfixes 
between the two and eventually this code may end up in gnulib. Moreover, if 
somebody else implemented color output for another OS in grep, it would be very 
easy to transplant it to GCC (or viceversa) if the API remains close.


Cheers,

Manuel.

replace libiberty with gnulib (was: Re: [PATCH 0/2] add unique_ptr class)

[Manuel López-Ibáñez]

On 05/09/17 18:40, Pedro Alves wrote:

On 09/05/2017 05:52 PM, Manuel López-Ibáñez wrote:
Yeah, ISTR it was close, though there were a couple things
that needed addressing still.

The wiki seems to miss a pointer to following iterations/review
of that patch (mailing list archives don't cross month
boundaries...).  You can find it starting here:
  https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01208.html
I think this was the latest version posted:
  https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01554.html


Thanks, I have updated the hyperlinks in the wiki.

Unfortunately, Ayush left and there is no one else to finish the work. While 
converting individuals functions from libiberty to gnulib is more or less 
straightforward, the build system of GCC is far too complex for any new or 
less-experienced contributor to finish the job.


I have also updated https://gcc.gnu.org/wiki/SummerOfCode
I don't believe that this was the only project accepted in 2016, but I cannot 
remember the others. Didn't GCC apply this year?


Cheers,

Manuel.

Re: [PATCH 0/2] add unique_ptr class

[Manuel López-Ibáñez]

On 05/08/17 20:05, Pedro Alves wrote:

That'd be an "obvious" choice, and I'm not terribly against it,
though I wonder whether it'd be taking over a name that has a wider
scope than intended?  I.e., GNU is a larger set of projects than the
GNU toolchain.  For example, there's Gnulib, which already compiles
as libgnu.a / -lgnu, which might be confusing.  GCC doesn't currently
use Gnulib, but GDB does, and, there was work going on a while ago to
make GCC use gnulib as well.


Unfortunately, that work was never committed, although there are parts that are 
ready to be committed and the rest of the conversion could be done incrementally:


https://gcc.gnu.org/wiki/replacelibibertywithgnulib

Cheers,

Manuel.

Re: [PATCH 1/2] C++ template type diff printing

[Manuel López-Ibáñez]

On 08/05/17 17:51, David Malcolm wrote:

So I think it can work if we add a "needs quoting" flag to the
postprocessing phase, if we need to handle the case where %H and %I
ever appear without 'q' (and have the delayed handling stash that flag,
and do the quoting there).

I'll look at implementing that.


Perhaps this may also help to fix the quoting (and coloring) of typedefs?

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62170


Is '-felide-type' a good name?  Wouldn't something like
'-fdiagnostic-elide-template-args' be better?


Here I'm merely copying clang's option name.
  http://clang.llvm.org/docs/UsersManual.html#cmdoption-fno-elide-type



We don't need to copy things from Clang when they are worse and they are surely 
stuck now with some bad choices, like we are. Let's just copy the good bits :)


-fdiagnostics-show-template-tree is ok, but -felide-type sounds like an 
optimization. Moreover, with modern command-line completion, it is easier to 
partially complete -fdiagnostics- and see what options are there then press 
'e', than partially complete '-f' and try to find the name of that option that 
summarises the template arguments in diagnostics.


Cheers,

Manuel.

Re: [PATCH] Add -Wshadow-local and -Wshadow-compatible-local.

[Manuel López-Ibáñez]

On 11/09/16 14:02, Mark Wielaard wrote:

  -Wshadow-local which warns if a local variable shadows another local
  variable or parameter,

  -Wshadow-compatible-local which warns if a local variable shadows
  another local variable or parameter whose type is compatible with that
  of the shadowing variable.


I honestly don't see the need for the second flag. Why not make Wshadow, or at 
least Wshadow-local, work in this way by default? Warning for shadowed 
variables that will nevertheless trigger errors/warnings if used wrongly seems 
not very useful anyway.




+   /* If '-Wshadow-compatible-local' is specified without other
+  -Wshadow flags, we will warn only when the types of the
+  shadowing variable (i.e. new_decl) and the shadowed variable
+  (old_decl) are compatible.  */
+   if (comptypes (TREE_TYPE (old_decl), TREE_TYPE (new_decl)))
+ warning_code = OPT_Wshadow_compatible_local;
+   else
+ warning_code = OPT_Wshadow_local;
+   warned = warning (warning_code,
+ "declaration of %q+D shadows a parameter",
+ new_decl);


Please don't use +D. Use warning_at with DECL_SOURCE_LOCATION(new_decl).
See: https://gcc.gnu.org/wiki/DiagnosticsGuidelines#Locations



+ /* If '-Wshadow-compatible-local' is specified without other
+-Wshadow flags, we will warn only when the type of the
+shadowing variable (i.e. x) can be converted to that of
+the shadowed parameter (oldlocal). The reason why we only
+check if x's type can be converted to oldlocal's type
+(but not the other way around) is because when users
+accidentally shadow a parameter, more than often they
+would use the variable thinking (mistakenly) it's still
+the parameter. It would be rare that users would use the
+variable in the place that expects the parameter but
+thinking it's a new decl.  */


As said above, IMHO, this behavior should be the default of -Wshadow, or at 
least, of -Wshadow-local. The current behavior only leads to people not using 
-Wshadow (and us not including it in -Wall -Wextra). There is a Linus rant from 
some years ago that explains vehemently why Wshadow is useless in its current form.



+@item -Wshadow-compatible-local
+@opindex Wshadow-compatible-local
+@opindex Wno-shadow-compatible-local
+Warn when a local variable shadows another local variable or parameter
+whose type is compatible with that of the shadowing variable. In C++,
+type compatibility here means the type of the shadowing variable can be
+converted to that of the shadowed variable. The creation of this flag
+(in addition to @option{-Wshadow-local}) is based on the idea that when
+a local variable shadows another one of incompatible type, it is most
+likely intentional, not a bug or typo, as shown in the following example:


-Wshadow-compatible-local seems safe enough to be enabled by -Wall (or 
-Wextra). Options not enabled by either are rarely used (and, hence, rarely 
tested).


Cheers,

Manuel.

Re: [PATCH, i386] Spellcheck hints for the i386 backend option handling (PR middle-end/77475)

[Manuel López-Ibáñez]

On 05/09/16 20:42, Manuel López-Ibáñez wrote:

On 05/09/16 18:25, Jakub Jelinek wrote:

Hi!

While most of the i386.opt -m= options have enum args and thus
cmdline_handle_error handles those, -march=/-mtune=/-m*-strategy= (and also
-mrecip=) don't use that, with the CPU strings being maintained inside of a
function rather than in some *.def file that could be also sourced into the
*.opt or something (and similarly for the strategies).

This patch adds inform calls that handle those similarly to what
cmdline_handle_error does for the options with enum values.
In addition, it adds %qs instead of %s in a couple of spaces, and
stops reporting incorrect attribute option("march=...") when it is
target("march=...") etc.


Something like the following should avoid a lot of (future) duplication
(untested):


My proposal had an obvious regression if (e->unknown_error). This one might be 
better:


Index: opts-common.c
===
--- opts-common.c   (revision 239995)
+++ opts-common.c   (working copy)
@@ -1069,6 +1069,38 @@
   decoded->errors = 0;
 }

+static void
+candidates_to_string (char *s, const auto_vec  *candidates)
+{
+  int i;
+  const char *candidate;
+  char *p = s;
+  FOR_EACH_VEC_ELT (*candidates, i, candidate)
+{
+  gcc_assert (candidate);
+  size_t arglen = strlen (candidate);
+  memcpy (p, candidate, arglen);
+  p[arglen] = ' ';
+  p += arglen + 1;
+}
+  p[-1] = 0;
+}
+
+void
+unrecognized_argument_hint (location_t loc, const char *opt, const char *arg,
+const auto_vec  ,
+size_t total_len)
+{
+  char *s = XALLOCAVEC (char, total_len);
+  candidates_to_string (s, );
+  const char *hint = find_closest_string (arg, );
+  if (hint)
+inform (loc, "valid arguments to %qs are: %s; did you mean %qs?",
+   opt, s, hint);
+  else
+inform (loc, "valid arguments to %qs are: %s", opt, s);
+}
+
 /* Perform diagnostics for read_cmdline_option and control_warning_option
functions.  Returns true if an error has been diagnosed.
LOC and LANG_MASK arguments like in read_cmdline_option.
@@ -1107,40 +1139,22 @@
   if (errors & CL_ERR_ENUM_ARG)
 {
   const struct cl_enum *e = _enums[option->var_enum];
-  unsigned int i;
-  size_t len;
-  char *s, *p;
-
   if (e->unknown_error)
error_at (loc, e->unknown_error, arg);
   else
-   error_at (loc, "unrecognized argument in option %qs", opt);
+   error_at (loc, "argument %qs to %qs not recognized", arg, opt);

-  len = 0;
-  for (i = 0; e->values[i].arg != NULL; i++)
-   len += strlen (e->values[i].arg) + 1;
-
+  size_t len = 0;
+  unsigned int i;
   auto_vec  candidates;
-  s = XALLOCAVEC (char, len);
-  p = s;
   for (i = 0; e->values[i].arg != NULL; i++)
{
  if (!enum_arg_ok_for_language (>values[i], lang_mask))
-   continue;
- size_t arglen = strlen (e->values[i].arg);
- memcpy (p, e->values[i].arg, arglen);
- p[arglen] = ' ';
- p += arglen + 1;
+   continue;
+ len += strlen (e->values[i].arg) + 1;
  candidates.safe_push (e->values[i].arg);
}
-  p[-1] = 0;
-  const char *hint = find_closest_string (arg, );
-  if (hint)
-   inform (loc, "valid arguments to %qs are: %s; did you mean %qs?",
-   option->opt_text, s, hint);
-  else
-   inform (loc, "valid arguments to %qs are: %s", option->opt_text, s);
-
+  unrecognized_argument_hint (loc, opt, arg, candidates, len);
   return true;
 }

Re: [PATCH, i386] Spellcheck hints for the i386 backend option handling (PR middle-end/77475)

[Manuel López-Ibáñez]

On 05/09/16 18:25, Jakub Jelinek wrote:

Hi!

While most of the i386.opt -m= options have enum args and thus
cmdline_handle_error handles those, -march=/-mtune=/-m*-strategy= (and also
-mrecip=) don't use that, with the CPU strings being maintained inside of a
function rather than in some *.def file that could be also sourced into the
*.opt or something (and similarly for the strategies).

This patch adds inform calls that handle those similarly to what
cmdline_handle_error does for the options with enum values.
In addition, it adds %qs instead of %s in a couple of spaces, and
stops reporting incorrect attribute option("march=...") when it is
target("march=...") etc.


Something like the following should avoid a lot of (future) duplication 
(untested):

Index: opts-common.c
===
--- opts-common.c   (revision 239995)
+++ opts-common.c   (working copy)
@@ -1069,6 +1069,40 @@
   decoded->errors = 0;
 }

+static void
+candidates_to_string(char *s, const auto_vec  *candidates)
+{
+  int i;
+  const char *candidate;
+  char *p = s;
+  FOR_EACH_VEC_ELT (*candidates, i, candidate)
+{
+  gcc_assert (candidate);
+  size_t arglen = strlen (candidate);
+  memcpy (p, candidate, arglen);
+  p[arglen] = ' ';
+  p += arglen + 1;
+}
+  p[-1] = 0;
+}
+
+void
+unrecognized_argument_error (location_t loc, const char *opt, const char *arg,
+const auto_vec  ,
+size_t total_len)
+{
+  error_at (loc, "argument %qs to %qs not recognized", arg, opt);
+
+  char *s = XALLOCAVEC (char, total_len);
+  candidates_to_string (s, );
+  const char *hint = find_closest_string (arg, );
+  if (hint)
+inform (loc, "valid arguments to %qs are: %s; did you mean %qs?",
+   opt, s, hint);
+  else
+inform (loc, "valid arguments to %qs are: %s", opt, s);
+}
+
 /* Perform diagnostics for read_cmdline_option and control_warning_option
functions.  Returns true if an error has been diagnosed.
LOC and LANG_MASK arguments like in read_cmdline_option.
@@ -1107,40 +1141,22 @@
   if (errors & CL_ERR_ENUM_ARG)
 {
   const struct cl_enum *e = _enums[option->var_enum];
-  unsigned int i;
-  size_t len;
-  char *s, *p;
-
   if (e->unknown_error)
error_at (loc, e->unknown_error, arg);
   else
-   error_at (loc, "unrecognized argument in option %qs", opt);
-
-  len = 0;
-  for (i = 0; e->values[i].arg != NULL; i++)
-   len += strlen (e->values[i].arg) + 1;
-
-  auto_vec  candidates;
-  s = XALLOCAVEC (char, len);
-  p = s;
-  for (i = 0; e->values[i].arg != NULL; i++)
{
- if (!enum_arg_ok_for_language (>values[i], lang_mask))
-   continue;
- size_t arglen = strlen (e->values[i].arg);
- memcpy (p, e->values[i].arg, arglen);
- p[arglen] = ' ';
- p += arglen + 1;
- candidates.safe_push (e->values[i].arg);
+ size_t len = 0;
+ unsigned int i;
+ auto_vec  candidates;
+ for (i = 0; e->values[i].arg != NULL; i++)
+   {
+ if (!enum_arg_ok_for_language (>values[i], lang_mask))
+   continue;
+ len += strlen (e->values[i].arg) + 1;
+ candidates.safe_push (e->values[i].arg);
+   }
+ unrecognized_argument_error (loc, opt, arg, candidates, len);
}
-  p[-1] = 0;
-  const char *hint = find_closest_string (arg, );
-  if (hint)
-   inform (loc, "valid arguments to %qs are: %s; did you mean %qs?",
-   option->opt_text, s, hint);
-  else
-   inform (loc, "valid arguments to %qs are: %s", option->opt_text, s);
-
   return true;
 }

Re: [PATCH] Silence some uninitialized variable warnings that appear when bootstrapping

[Manuel López-Ibáñez]

On 3 September 2016 at 02:11, Eric Gallager <eg...@gwmail.gwu.edu> wrote:
> On 9/2/16, Manuel López-Ibáñez <lopeziba...@gmail.com> wrote:
>> On 3 Sep 2016 12:56 a.m., "Eric Gallager" <eg...@gwmail.gwu.edu> wrote:
>>> I tried that but it doesn't look like it produced any dumpfiles...
>>
>> I often use -fdump-tree-all-all-lineno
>>
>
>
> That produced a lot of files; I'm attaching a tarball of all of them
> because I don't know which is the correct one...

None of them. Are you sure you were using at least -O2 and -Wall ?

In any case, it is not worth it to waste your time on this further:

* Warnings in stage1 don't matter: https://gcc.gnu.org/wiki/FAQ#stage1warnings
* Wrong (missing or bogus) uninitialized warnings only present in old
compilers will not get fixed. They are rarely a regression nor easy to
fix.
* Large testcases for wrong uninitialized warnings are not worth
investigating except if they show a regression in trunk. Otherwise,
there are so many known issues with Wuninitialized that the analysis
will certainly be closed as a duplicate and not help at all towards a
possible fix.
* If you wish to learn about Wuninitialized, it is better to start
with one of the well-known and already analyzed bug reports than with
a large testcase. Major problems are described here:
https://gcc.gnu.org/wiki/Better_Uninitialized_Warnings#Current_Situation
However, those are likely among the hardest problems to solve in GCC,
so you may want to check for easyhacks.


Cheers,

Manuel.

Re: [PATCH] Move class substring_loc from c-family into gcc

[Manuel López-Ibáñez]

On 02/09/16 23:55, Martin Sebor wrote:

diff --git a/gcc/substring-locations.h b/gcc/substring-locations.h
index f839c74..bb0de4f 100644
--- a/gcc/substring-locations.h
+++ b/gcc/substring-locations.h
@@ -20,6 +20,73 @@ along with GCC; see the file COPYING3.  If not see
  #ifndef GCC_SUBSTRING_LOCATIONS_H
  #define GCC_SUBSTRING_LOCATIONS_H

+#include 
+


Is this header file going to be used in the middle-end? If so, then it is 
suspicious that it includes cpplib.h. Otherwise, perhaps it should live in 
c-family/


I'm not complaining about substring-locations.c because libcpp is already 
linked with everything else even for other non-C languages, like Ada, but the 
above is leaking all cpplib.h into the rest of the compiler, which defeats the 
purpose of this in coretypes.h


/* Provide forward struct declaration so that we don't have to include
   all of cpplib.h whenever a random prototype includes a pointer.
   Note that the cpp_reader and cpp_token typedefs remain part of
   cpplib.h.  */

struct cpp_reader;
struct cpp_token;


Cheers,
Manuel.

Re: [PATCH] Silence some uninitialized variable warnings that appear when bootstrapping

[Manuel López-Ibáñez]

On 02/09/16 20:27, Segher Boessenkool wrote:

On Fri, Sep 02, 2016 at 02:21:07PM -0400, Eric Gallager wrote:

../../gcc/combine.c: In function ‘int combine_instructions(rtx_insn*,
unsigned int)’:
../../gcc/combine.c:1310:8: warning: ‘prev’ may be used uninitialized
in this function [-Wmaybe-uninitialized]
if ((next = try_combine (insn, prev, NULL, NULL,
^~


That is:

  if (HAVE_cc0
  && JUMP_P (insn)
  && (prev = prev_nonnote_insn (insn)) != 0
  && NONJUMP_INSN_P (prev)
  && sets_cc0_p (PATTERN (prev)))
{
  if ((next = try_combine (insn, prev, NULL, NULL,
   _direct_jump_p,
   last_combined_insn)) != 0)

so prev is always initialised here.  Could you try to find out why GCC
warns anyway?  Or open a PR?

HAVE_cc0 probably expands to 0 (I'm not sure what your target is), that
might have something to do with it.


Unfortunately, the location reported by GCC is sometimes wrong, because 
location tracking in the middle-end is far from perfect. 
https://gcc.gnu.org/PR40635 https://gcc.gnu.org/PR53917


The only way to know for sure what GCC is warning about is to look at the 
uninit dump.


Moreover, if the warning is bogus and not a regression, it is very likely that 
it is reported already here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=24639


The effort dedicated to report and analyze the report would be better spent 
fixing any of the issues already known.


Nevertheless, assignments within 'if' are one of the things that make reading 
GCC code harder than it needs to be (and combine.c is scary).


Cheers,

Manuel.

Re: PR35503 - warn for restrict pointer

[Manuel López-Ibáñez]

On 02/09/16 18:44, David Malcolm wrote:

Much better would be to have the formatting be done inside the
diagnostics subsystem's call into pp_format, with something like this:

  warning_at_rich_loc_n (, OPT_Wrestrict,
 arg_positions
.length (),
 "passing argument %i to restrict
-qualified"
 " parameter aliases with argument
%FIXME",
 "passing argument %i to restrict
-qualified"
 " parameter aliases with arguments
%FIXME",
 param_pos + 1,

 _positions);


Yes, building up diagnostic messages from pieces is discouraged: 
https://gcc.gnu.org/codingconventions.html#Diagnostics



and have %FIXME (or somesuch) consume _positions in the va_arg,
printing the argument numbers there.  Doing it this way also avoids
building the string for the case where Wrestrict is disabled, since the
pp_format only happens after we know we're going to print the warning.


Is it possible to pass template arguments through ... ? And how does va_arg 
know the type of the particular template passed?



Assuming that there isn't yet a pre-canned way to print a set of
argument numbers that I've missed, the place to add the %FIXME-handling
would presumably be in default_tree_printer in tree-diagnostic.c -
though it's obviously nothing to do with trees. (Or if this is too
single-purpose, perhaps there's a need to temporarily inject one-time
callbacks for consuming custom args??).


I'm surprised we don't have a function pp_vec to print/debug a vec<>, but 
perhaps it is simpler to convert arg_pos to a 'char *' and use %s instead of 
%FIXME or call-backs.


Cheers,

Manuel.

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Manuel López-Ibáñez]

>> --Wno-format-contains-nul -Wno-format-extra-args -Wformat-nonliteral @gol
>> +-Wno-format-contains-nul -Wno-format-extra-args -Wformat-length=1 @gol
>>
>> Most options that take levels are documented as:
>>
>> -Wshift-overflow -Wshift-overflow=@var{n}
>> -Wstrict-overflow -Wstrict-overflow=@var{n}
>
>
> I haven't found any uses of the @var{} tag for numeric arguments
> to options (e.g., -Wformat=2), only for symbolic arguments (e.g.,
> -Warray-bounds=@var{n}), so I'm leaving this as is.

My suggestion was to document -Wformat-length=@var{n}. There is no
difference between -Wformat-length's levels and -Wstrict-overfllow,
-Wshift-overflow, -Wstrict-aliasing levels.
-Wformat=2 is actually the odd one. Note that its description does use:
@item -Wformat
@itemx -Wformat=@var{n}

Even if you want to be explicit and follow -Wformat, then it should be

+-Wno-format-contains-nul -Wno-format-extra-args -Wformat-length
-Wformat-length=2 @gol


> I agree.  The challenge is that not all the bits this depends on
> (the g_string_concat_db and parse_in globals defined in the front
> end) are available in the middle end.  I've been talking to David
> Malcolm about how best to factor things out of c-format.c and make
> it available in both parts of the compiler under a convenient API.

Perhaps diagnostics_context could have pointers to those, forward
defined in the .h file and include the relevant libcpp headers in
diagnostics.c (or input.c). FEs that make use of those features could
initialize them (via some API) to some existing object. Otherwise,
they will work like in your patch (but within diagnostic.c). Similar
to how we initialize the FE-specific pretty-printers.

We already depend on libcpp for line-map.c, so internally depending on
other libcpp features is not so bad. The important thing is to hide
this from the clients, so that the clients do not need to be aware of
what diagnostics.c requires. That is, the middle-end and Ada should
not include headers that include libcpp headers, but diagnostics.c can
include whatever it needs.

Otherwise, the future will be again a mess and we get further away
from ever separating the FEs from the ME.

BTW, it would be nice to explain in comments why each header needs to
be included, besides obvious ones such as tree.h and gimple.h (it
would be great if we had guidelines on how to order included headers,
why not group together all gimple*, tree*, backend-stuff, diagnostics
stuff?). On the other hand, it is unfair to nitpick your patch
regarding this when other commits do the same.

+#include "backend.h"
+#include "tree.h"
+#include "gimple.h"
+#include "tree-pass.h"
+#include "ssa.h"
+#include "gimple-fold.h"
+#include "gimple-pretty-print.h"
+#include "diagnostic-core.h"

Already included in diagnostic.h

+#include "fold-const.h"
+#include "gimple-iterator.h"
+#include "tree-ssa.h"
+#include "tree-object-size.h"
+#include "params.h"
+#include "tree-cfg.h"
+#include "calls.h"
+#include "cfgloop.h"
+#include "intl.h"
+
+#include "builtins.h"
+#include "stor-layout.h"
+
+#include "realmpfr.h"
+#include "target.h"
+#include "targhooks.h"
+
+#include "cpplib.h"

Not in the ME!

+#include "input.h"

already included in coretypes.h

+#include "toplev.h"

Very few files actually need to include toplev.h. Most of them just
include it because of copy-pasting headers lists. And even for bogus
reasons (final.c:#include "toplev.h" /* exact_log2, floor_log2 */)

+#include "substring-locations.h"
+#include "diagnostic.h"

Cheers,

Manuel.

Re: [PATCH 0/4] Applying fix-its on behalf of the user to their code

[Manuel López-Ibáñez]

On 24/08/16 14:56, Richard Biener wrote:

You never typoed

gcc t.c -o t.c

?  ;)  (I did ... :/)


With GCC >=5

$ gcc t.c -o t.c
gcc: fatal error: input file ‘t.c’ is the same as output file
compilation terminated.

You are welcome ;-)

Manuel.

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Manuel López-Ibáñez]

On 23 August 2016 at 22:56, Martin Sebor  wrote:
> Attached is the latest patch.

Some suggestions:

--Wno-format-contains-nul -Wno-format-extra-args -Wformat-nonliteral @gol
+-Wno-format-contains-nul -Wno-format-extra-args -Wformat-length=1 @gol

Most options that take levels are documented as:

-Wshift-overflow -Wshift-overflow=@var{n}
-Wstrict-overflow -Wstrict-overflow=@var{n}


+@item -Wformat-length
+@itemx -Wformat-length=@var{level}
+@opindex Wformat-length
+@opindex Wno-format-length
+@opindex ffreestanding
+@opindex fno-builtin
+@opindex Wformat-length=

This whole hunk is duplicated

Usually, at the end of the option description, it is mentioned which
options enabled this one (-Wformat=1 in this case, which is in turn
enabled by -Wall).

+The @option{-Wformat-length} option causes GCC to attempt to detect calls
+to formatted functions such as @code{sprintf} that might overflow the
+destination buffer, or bounded functions like @code{snprintf} that result
+in output truncation.

This text is a bit too verbose and quite different from other warning
options. A humble proposal:

Warn about calls to formatted functions, such as @code{sprintf}, that
may overflow the destination buffer, or bounded functions, such as
@code{snprintf} , that may? result in output truncation.

+GCC counts the number of bytes that each format
+string and directive within it writes into the provided buffer and, when
+it detects that more bytes that fit in the destination buffer may be output,
+it emits a warning.

This is saying basically the same as the first sentence. Remove it?

+Directives whose arguments have values that can be
+determined at compile-time account for the exact number of bytes they write.
+Directives with arguments whose values cannot be determined are processed
+based on heuristics that depend on the @var{level} argument to the option,
+and on optimization.

When the exact number of bytes written by a format string directive
cannot be determined at compile-time, it is estimated based on
heuristics that depend on optimization and the @var{level} argument.
(I swapped the two because level is the next thing explained).

+The default setting of @var{level} is 1.  Level
+@var{1} employs a conservative approach that warns only about calls that
+most likely overflow the buffer or result in output truncation.  At this
+level, numeric arguments to format directives whose values are unknown
+are assumed to have the value of one, and strings of unknown length are
+assumed to have a length of zero.  Numeric arguments that are known to
+be bounded to a subrange of their type, or string arguments whose output
+is bounded by their directive's precision, are assumed to take on the value
+within the range that results in the most bytes on output.  Level @var{2}
+warns also bout calls that may overflow the destination buffer or result
+in truncation given an argument of sufficient length or magnitude.  At
+this level, unknown numeric arguments are assumed to have the minimum
+representable value for signed types with a precision greater than 1,
+and the maximum representable value otherwise.  Unknown string arguments
+are assumed to be 1 character long.

A better format (and it enables searching for specific levels) would be:
@table @gcctabopt
@item -Wformat-length=1
This is the warning level enabled by @option{-Wformat-length} and is enabled
by @option{-Wformat=1}, which is in turn enabled by @option{-Wall}. It
employs a conservative heuristic that warns only about calls that
most likely overflow the buffer or result in output truncation.  At this
level, numeric arguments to format directives whose values are unknown
are assumed to have the value of one, and strings of unknown length are
assumed to have a length of zero.  Numeric arguments that are known to
be bounded to a subrange of their type, or string arguments whose output
is bounded by their directive's precision, are assumed to take on the value
within the range that results in the most bytes on output.

@item -Wformat-length=2
This level also warns about calls that may overflow the destination
buffer or result
in truncation given an argument of sufficient length or magnitude.
Unknown numeric arguments are assumed to have the minimum
representable value for signed types with a precision greater than one,
and the maximum representable value otherwise.  Unknown string arguments
are assumed to be one character long.
@end table

+Enabling optimization will in most
+cases improve the accuracy of the warning, although in some cases it may
+also result in false positives.

Since the example is not about optimization. Move this paragraph after
the examples.

+For example, at level @var{1}, the call to @code{sprintf} below is diagnosed

For example, @option{-Wformat-length=1} warns about the call to
@code{sprintf} below

+At level @var{2}, the call
+is again diagnosed, but this time

@option{-Wformat-length=2} also warns about the call to

Re: [PATCH/BUILD] [GSOC] [REPLACING LIBIBERTY WITH GNULIB]

[Manuel López-Ibáñez]

On 22 August 2016 at 17:34, Manuel López-Ibáñez <lopeziba...@gmail.com> wrote:
> On 22 August 2016 at 17:27, ayush goel <ayushgoel1...@gmail.com> wrote:
>> Links to the patches containing the work:
>>
>> https://gcc.gnu.org/ml/gcc-patches/2016-07/msg01302.html
>
> Actually, the above version is outdated and it should not be reviewed.
> The latest version of this patch, is this one:
>
> https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01477.html

And now it is https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01554.html

Apologies for the noise,

Manuel.

Re: [PATCH/BUILD] [GSOC] [REPLACING LIBIBERTY WITH GNULIB]

[Manuel López-Ibáñez]

On 22 August 2016 at 17:27, ayush goel  wrote:
> Links to the patches containing the work:
>
> https://gcc.gnu.org/ml/gcc-patches/2016-07/msg01302.html

Actually, the above version is outdated and it should not be reviewed.
The latest version of this patch, is this one:

https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01477.html

Cheers,

Manuel.

Re: C++11? (Re: protected alloca class for malloc fallback)

[Manuel López-Ibáñez]

On 22/08/16 13:02, Eric Gallager wrote:

As a rookie programmer considering possibly contributing to GCC in the
future once I'm more confident in my abilities, switching to C++11
would increase the barrier for me to contribute. I currently really
only know C, and I still have to learn C++ in general, much less all
the new features added in C++11. Also, I still occasionally bootstrap
with Apple's gcc 4.2.1, which doesn't have C++11 support. That being
said, I'd support taking steps to make it easier to optionally compile
gcc as C++11 for those that want to do so, such as replacing
-Wno-narrowing with -Wnarrowing in the warning flags that gcc uses.
Out of curiosity, I tried doing just that, and there were actually
fewer new unique warnings than I expected. I've attached a log of
them.


If you are able to bootstrap GCC and run the regression testsuite, you have 
done 50% of what you need to submit your first patch:


https://gcc.gnu.org/wiki/GettingStarted#Basics:_Contributing_to_GCC_in_10_easy_steps

The implementation language is often a detail hidden by many layers of 
abstraction. It is more important to get to know those abstractions, know how 
to use a debugger and understand how various things fit together in gcc (FEs, 
MEs, back-ends, gimple, trees, RTL) than the implementation of those things. 
And the best way to learn is to start with a small project in your area of 
interest 
(https://gcc.gnu.org/bugzilla/buglist.cgi?keywords=easyhack_id=158576=---) 
and persevere.


Cheers,

Manuel.

Re: [PATCH build/doc] Replacing libiberty with gnulib

[Manuel López-Ibáñez]

On 20 August 2016 at 11:22, ayush goel  wrote:
>>
>> We're talking about a one-line change, but this is absolutely
>> crucial and central to use of gnulib. Until this is correct,
>> any previous host-specific testing is invalid, unfortunately.
>>
>> In the previous revision, you had:
>>
>> INCGNU = -I../gnulib -I$(srcdir)/../gnulib/import
>>
>> and I was expecting to see in the new revision something
>> like:
>>
>> INCGNU = -I../gnulib/import -I$(srcdir)/../gnulib/import
>>
>> or perhaps even better:
>>
>> INCGNU = -I$(build_libobjdir)/gnulib/import -I$(srcdir)/../gnulib/import
>>
>> Try hacking one of the generated replacement headers, one that gcc
>> is sure to include, to cause a compilation error. E.g. add an #error call
>> to the generated gnulib/import/unistd.h. If a gcc build, from scratch since
>> there are no Makefile dependencies, still compiles with that, then we're
>> still not picking the right headers.
>
> So your concern seems valid, however the build process seems to pick
> the correct headers.
> I did try to raise an error from the gnulib generated header unitstd.h
> and it gives me a compile time error:

Perhaps adding #error is not the best way to test this, since gnulib
may still pick the correct headers, but gcc the wrong ones and the
error you showed happens when building gnulib. A better way, perhaps,
would be to add to unitsdt.h something like

#define unitstd_h_gnulib

then in some gcc/ file that includes this header, like gcov-tool.c or
system.h, you can do just after the #include

#ifndef unitstd_h_gnulib
#error "unitstd_h_gnulib not defined"
#endif

Test that it fails when not having the correct INCGNU and that it
works when having it. This should settle it, I hope.

Cheers,

Manuel.

Re: Implement -Wimplicit-fallthrough (take 3)

[Manuel López-Ibáñez]

On 18/08/16 18:07, David Malcolm wrote:


This isn't quite the way that fix-its are meant to be used, in my mind,
at least: the insertion text is supposed to be something that could be
literally inserted into the code (e.g. by an IDE) i.e. it should be a
code fragment, rather than a message to the user.


I added this info to

https://gcc.gnu.org/wiki/DiagnosticsGuidelines#Fix-it_hints




+static void
+maybe_warn_implicit_fallthrough (gimple_seq seq)
+{
+  if (!warn_implicit_fallthrough || lang_GNU_Fortran ())
+return;
+



Does this warning make sense if !(lang_GNU_C() || lang_GNU_CXX()) ?

Cheers,

Manuel.

Re: [PATCH build/doc] Replacing libiberty with gnulib

[Manuel López-Ibáñez]

On 15 August 2016 at 13:50, ayush goel  wrote:
> Included gnulib’s config.h header file inside gcc’s config.h itself as
> per the discussions.
>
> Built and tested the system.

You need to mention how you build it (languages, bootstrap, etc.) and
how you tested it (targets).

Do the other patches that you submitted still work with this new one?

Cheers,

Manuel.

Re: [PATCH v2] cpp/c: Add -Wexpansion-to-defined

[Manuel López-Ibáñez]

On 10 August 2016 at 15:44, Paolo Bonzini <bonz...@gnu.org> wrote:
>
>
> On 10/08/2016 16:42, Manuel López-Ibáñez wrote:
>> > > My only fear is that people not using -Wpedantic nor -pedantic-errors
>> > > expect that GNU extensions work. This is a GNU extension that defines
>> > > something that is undefined according to ISO. Enabling the warning
>> > > with -Wextra is just annoying those people who may not care about
>> > > other compilers.
>> >
>> > I think this warning falls in the same category as
>> > -Wshift-negative-value.  (In fact I dislike -Wshift-negative-value a
>> > lot, and would put that one under -Wpedantic only).
>>
>> It is not the same category. One is compile-time UB and the other is
>> runtime UB. See:
>> https://gcc.gnu.org/ml/gcc-patches/2015-04/msg01551.html
>> and https://gcc.gnu.org/ml/gcc-patches/2015-04/msg01529.html
>
> Right---what I meant is it's the same kind of "annoying for people who
> expect that GNU extensions work" warning.

Oh, I agree. I'm just mentioning what the current definition/behavior
is (and documenting it here:
https://gcc.gnu.org/wiki/DiagnosticsGuidelines FWIW), not what I think
should be.

Perhaps we need something like -Wextra-pedantic, for things that are
undefined by ISO C but defined by GNU. Thus, they would not trigger
pedwarns and no error with -pedantic-errors.

Or we need to split -Wpedantic into -Wpedantic-pedwarns and
-Wpedantic-nopedwarns (with better names). This way -pedantic-errors
would be equivalent to -Werror=pedantic-pedwarns +
-Werror=pedwarns-not-controlled-by-pedantic.

i find -pedantic-errors too out of place with the rest of -W* options.

Manuel.

Re: [PATCH v2] cpp/c: Add -Wexpansion-to-defined

[Manuel López-Ibáñez]

On 10 August 2016 at 13:06, Paolo Bonzini <bonz...@gnu.org> wrote:
>
>
> On 10/08/2016 13:31, Manuel López-Ibáñez wrote:
>> My only fear is that people not using -Wpedantic nor -pedantic-errors
>> expect that GNU extensions work. This is a GNU extension that defines
>> something that is undefined according to ISO. Enabling the warning
>> with -Wextra is just annoying those people who may not care about
>> other compilers.
>
> I think this warning falls in the same category as
> -Wshift-negative-value.  (In fact I dislike -Wshift-negative-value a
> lot, and would put that one under -Wpedantic only).

It is not the same category. One is compile-time UB and the other is
runtime UB. See:
https://gcc.gnu.org/ml/gcc-patches/2015-04/msg01551.html
and https://gcc.gnu.org/ml/gcc-patches/2015-04/msg01529.html

Cheers,

Manuel.

Re: [PATCH build/doc] Replacing libiberty with gnulib

[Manuel López-Ibáñez]

On 10 August 2016 at 14:40, ayush goel  wrote:
> On 9 August 2016 at 2:20:59 PM, Pedro Alves (pal...@redhat.com) wrote:
>> the scheme of configuring gnulib in a separate directory as borrowed from gdb
>> requires including two config.h headers -- the gnulib client's, and gnulib's.

Isn't this also true for libiberty's config.h ? I have no idea
when/how is that included.

>> Did you do something different that avoids needing that somehow?
>
> I wasn’t aware of this. Thanks for pointing this out.
> It’s strange however, I didn’t see anything failing while
> building/testing my system.
>>
>> In gdb, .c files don't include "config.h" directly. Instead all .c files
>> include a "defs.h" file first thing, and that in turn (after another 
>> indirection)
>> is what includes both gdb's "config.h" and gnulib's "config.h”:
>
> Can gcc also adopt a similar approach? Include gnulib’s config.h in a
> single header file instead of including it in every function that uses
> it.
> Which header file would be the most suitable for this purpose(probably
> which is generically included by almost all the gcc functions)?

Unfortunately, gcc/*.c include config.h directly. Sorry, I'm not
really sure how this is supposed to work and how it was working for
libiberty's config.h but I'd suggest to copy that if possible.

Cheers,

Manuel.

Re: [PATCH v2] cpp/c: Add -Wexpansion-to-defined

[Manuel López-Ibáñez]

On 10 August 2016 at 11:06, Paolo Bonzini  wrote:
> While I disagree with their inclusion of the warning to -Wall, I think
> it is a good addition overall.  First, it is a logical extension of the
> existing warning for breaking defined across a macro and its caller.
> Second, it is good to make these warnings for `defined' available with
> a command-line option other than -pedantic.  In fact this warning is
> not mandated by the standard and thus is a strange case of a non-pedwarn
> enabled by -pedantic.  As a side effect of using the command-line parsing
> machinery to attach the new warning to -pedantic, it would become an
> error for -pedantic-errors, which would be weird for a diagnostic that
> is not mandated by the standard.

Note that the definition of -pedantic-errors says: "in some cases
where there is undefined behavior at compile-time". Thus, this would
be allowed according to our current definitions. However, the
definition of -Wpedantic does not mention that, thus it could be a
pedwarn not controlled by -Wpedantic.

My only fear is that people not using -Wpedantic nor -pedantic-errors
expect that GNU extensions work. This is a GNU extension that defines
something that is undefined according to ISO. Enabling the warning
with -Wextra is just annoying those people who may not care about
other compilers.

Thus, my opinion is that the current definition of -Wpedantic is too
restrictive and it should contain the "in some cases where there is
undefined behavior at compile-time". And thus, this should be a
pedwarn enabled by -Wpedantic, not by -Wextra and an error with
-pedantic-errors. But you should wait for other opinions, specially
Joseph, before redoing it, even if you agree with me.

Cheers,

Manuel.

Re: [PATCH] cpp/c: Add -Wexpansion-to-defined

[Manuel López-Ibáñez]

On 9 August 2016 at 22:21, Paolo Bonzini <bonz...@gnu.org> wrote:
>
>
> On 09/08/2016 20:30, Manuel López-Ibáñez wrote:
>>>
>>>
>>> +  cpp_opts->warn_expansion_to_defined = cpp_warn_expansion_to_defined;
>>> +  if (cpp_warn_expansion_to_defined == -1)
>>> +cpp_warn_expansion_to_defined = pedantic || extra_warnings;
>>> +
>>
>> Instead of the above, plase use LangEnabledBy() or EnabledBy() in c.opt.
>> See Wendif-labels and other examples. Then, you do not need Init(-1).
>
> This causes this to produce an error if -pedantic-errors is provided,
> because -pedantic-errors does
>
>   control_warning_option (OPT_Wpedantic, DK_ERROR, NULL, value,
>   loc, lang_mask,
>   handlers, opts, opts_set,
>   dc);
>
> and this enables -Wexpansion-to-defined at DK_ERROR level.  Bug or fix?

TL;DR If the warning is enabled by -Wpedantic, it should be an error
with -Werror=pedantic and it should use cpp_pedwarning. Whether it
should be enabled by -Wpedantic is more difficult to say.

-pedantic is equivalent to -Wpedantic. If -Wx is enabled by -Wy, then
-Werror=y implies -Werror=x. Every warning enabled by -Wpedantic works
in the same way and I don't see why this one should be different.

Moreover, I think that this was a latent bug in libcpp: the code
should have used cpp_pedwarning instead of cpp_warning. This is
https://gcc.gnu.org/PR66505
Quoting https://gcc.gnu.org/wiki/DiagnosticsGuidelines#Types_of_diagnostics
: "The same rules apply to libcpp, which uses cpp_pedwarning (instead
of pedwarn), CPP_PEDANTIC (pfile) (instead of pedantic) and
CPP_W_PEDANTIC (instead of OPT_Wpedantic). In particular, you may use
cpp_pedwarning without CPP_W_PEDANTIC, but you may not use
CPP_W_PEDANTIC without cpp_pedwarning and you may not use CPP_PEDANTIC
(pfile) without CPP_W_PEDANTIC."

Cheers,

Manuel.

Re: [PATCH] cpp/c: Add -Wexpansion-to-defined

[Manuel López-Ibáñez]

On 09/08/16 16:59, Paolo Bonzini wrote:

Index: gcc/c-family/c-opts.c
===
--- gcc/c-family/c-opts.c   (revision 239276)
+++ gcc/c-family/c-opts.c   (working copy)
@@ -1256,6 +1256,10 @@ sanitize_cpp_opts (void)
   cpp_opts->unsigned_char = !flag_signed_char;
   cpp_opts->stdc_0_in_system_headers = STDC_0_IN_SYSTEM_HEADERS;

+  cpp_opts->warn_expansion_to_defined = cpp_warn_expansion_to_defined;
+  if (cpp_warn_expansion_to_defined == -1)
+cpp_warn_expansion_to_defined = pedantic || extra_warnings;
+


Instead of the above, plase use LangEnabledBy() or EnabledBy() in c.opt. See 
Wendif-labels and other examples. Then, you do not need Init(-1).



   /* Wlong-long is disabled by default. It is enabled by:
   [-Wpedantic | -Wtraditional] -std=[gnu|c]++98 ; or
   [-Wpedantic | -Wtraditional] -std=non-c99
Index: gcc/c-family/c.opt
===
--- gcc/c-family/c.opt  (revision 239276)
+++ gcc/c-family/c.opt  (working copy)
@@ -506,6 +506,10 @@ Wdouble-promotion
 C ObjC C++ ObjC++ Var(warn_double_promotion) Warning
 Warn about implicit conversions from \"float\" to \"double\".

+Wexpansion-to-defined
+C ObjC C++ ObjC++ CppReason(CPP_W_EXPANSION_TO_DEFINED) 
Var(cpp_warn_expansion_to_defined) Init(-1) Warning
+Warn if an undefined macro is used in an #if directive.
+


You are also missing CPP(warn_expansion_to_defined) so that the cpp and gcc 
sides are in sync even when using pragmas.


Cheers,

Manuel.

Re: [PATCH build/doc] Replacing libiberty with gnulib

[Manuel López-Ibáñez]

On 29 July 2016 at 17:55, Manuel López-Ibáñez <lopeziba...@gmail.com> wrote:
> On 29 July 2016 at 17:51, Joseph Myers <jos...@codesourcery.com> wrote:
>> On Wed, 20 Jul 2016, Manuel López-Ibáñez wrote:
>>
>>> On 20 July 2016 at 19:21, ayush goel <ayushgoel1...@gmail.com> wrote:
>>> > Hey,
>>> > As a first step of my GSOC project
>>> > (https://gcc.gnu.org/wiki/replacelibibertywithgnulib) I have imported
>>> > the gnulib library inside the gcc tree. I have created gnulib as a top
>>> > level directory which contains the necessary scripts to import the
>>> > modules. It also contains the necessary Makefile.in and configure.ac
>>> > files.
>>>
>>> Looks good to me, but I cannot approve it. Joseph, what do you think?
>>
>> That this would best be reviewed by a build-system maintainer.
>
> Sure, who could that be?

Jeff, as a global reviewer, how can we move this forward? You have
said in the past: "I suspect we'll probably want to go with direct use
of gnulib obstack at some point." Well, here it is.

If there is something wrong or missing, ideally we would like to know
so that Ayush can work on fixing it before the Summer of Code is over
in less than two weeks.

Cheers,

Manuel.

Re: [PATCH] Replacing gcc's dependence on libiberty's fnmatch to gnulib's fnmatch

[Manuel López-Ibáñez]

On 31 July 2016 at 23:39, Joseph Myers <jos...@codesourcery.com> wrote:
> On Sat, 30 Jul 2016, Manuel López-Ibáñez wrote:
>
>> > Building for different targets is fairly irrelevant here; the issue is
>> > building for different hosts, which is harder.
>>
>> What about my suggestion of forcing GCC to use the gnulib functions by
>> temporarily removing the system-wide functions? Would that be
>> equivalent testing to building on a host that requires the libiberty
>> version of a function?
>
> That depends on the details of how the configure tests in question work;
> if they test whether calls to a function can be linked, the function will
> be detected as present even if the header has been removed.

True. Oh well, Ayush does not have access to different hosts, thus I
guess it is better that he focuses his limited time on functions that
he can test. There are plenty of functions that are both in gnulib and
libiberty but not in glibc.

Cheers,

Manuel.

Re: [PATCH 1/3] (v2) On-demand locations within string-literals

[Manuel López-Ibáñez]

On 29 July 2016 at 18:27, David Malcolm <dmalc...@redhat.com> wrote:
> On Fri, 2016-07-29 at 17:53 +0100, Manuel López-Ibáñez wrote:
>> On 29 July 2016 at 16:25, David Malcolm <dmalc...@redhat.com> wrote:
>> >
>> > FWIW, it appears that clang uses the on-demand approach; the
>> > relevant
>> > code appears to be StringLiteral::getLocationOfByte:
>> > http://clang.llvm.org/doxygen/Expr_8cpp_source.html#l01008
>>
>> As far as I know, llvm doesn't do language diagnostics from the
>> middle-end/LTO. Thus, they do not have those problems.
>
> If you really want to have middle-end diagnostics from LTO, I can make
> the on-demand approach work.

Personally, I'm happy with having this work only on the FEs. I haven't
had time to look at what Martin is doing, so he may prefer otherwise.

In any case, making it work from LTO could be done as a follow-up, no?

> I can also do the stored-location approach, but it would mean rewriting
> all the patches again, I think, would be less efficient.

Agreed, FWIW.

> I would prefer the on-demand approach.
>
> Who is empowered to make a decision here?

I thought you were the diagnostics maintainer ;-)

Cheers,

Manuel.

Re: [PATCH] Replacing gcc's dependence on libiberty's fnmatch to gnulib's fnmatch

[Manuel López-Ibáñez]

On 29 July 2016 at 23:10, Joseph Myers  wrote:
> On Tue, 26 Jul 2016, Prathamesh Kulkarni wrote:
>
>> >> GCC can run on other systems besides OSX and GNU/Linux, how can you
>> >> test that your change does not break anything on those systems?
>> >>
>> > Well I have access to these two systems only. How would you suggest I
>> > test my patches on all possible systems?
>> Maybe building contrib/config-list.mk could help ? AFAIK, it will cross build
>> the gcc tree for different targets, but not run the testsuite, however
>> I could be wrong.
>
> Building for different targets is fairly irrelevant here; the issue is
> building for different hosts, which is harder.

What about my suggestion of forcing GCC to use the gnulib functions by
temporarily removing the system-wide functions? Would that be
equivalent testing to building on a host that requires the libiberty
version of a function?

Cheers,

Manuel.

Re: [PATCH build/doc] Replacing libiberty with gnulib

[Manuel López-Ibáñez]

On 29 July 2016 at 17:51, Joseph Myers <jos...@codesourcery.com> wrote:
> On Wed, 20 Jul 2016, Manuel López-Ibáñez wrote:
>
>> On 20 July 2016 at 19:21, ayush goel <ayushgoel1...@gmail.com> wrote:
>> > Hey,
>> > As a first step of my GSOC project
>> > (https://gcc.gnu.org/wiki/replacelibibertywithgnulib) I have imported
>> > the gnulib library inside the gcc tree. I have created gnulib as a top
>> > level directory which contains the necessary scripts to import the
>> > modules. It also contains the necessary Makefile.in and configure.ac
>> > files.
>>
>> Looks good to me, but I cannot approve it. Joseph, what do you think?
>
> That this would best be reviewed by a build-system maintainer.

Sure, who could that be?

Re: [PATCH 1/3] (v2) On-demand locations within string-literals

[Manuel López-Ibáñez]

On 29 July 2016 at 16:25, David Malcolm  wrote:
>
> FWIW, it appears that clang uses the on-demand approach; the relevant
> code appears to be StringLiteral::getLocationOfByte:
> http://clang.llvm.org/doxygen/Expr_8cpp_source.html#l01008

As far as I know, llvm doesn't do language diagnostics from the
middle-end/LTO. Thus, they do not have those problems.

Cheers,

Manuel.

Re: [PATCH 1/3] (v2) On-demand locations within string-literals

[Manuel López-Ibáñez]

On 27 July 2016 at 15:30, David Malcolm  wrote:
>> Perhaps it could live for now in c-format.c, since it is the only
>> place using it?
>
> Martin Sebor [CC-ed] wants to use it from the middle-end:
>   https://gcc.gnu.org/ml/gcc-patches/2016-07/msg01088.html
> so it's unclear to me that c-format.c would be a better location.

Fine. He will have to figure out how to get a cpp_reader from the
middle-end, though.

> There are various places it could live; but getting it working took a
> lot of effort to achieve - the currently proposed mixture of libcpp,
> input.c and c-format.c for the locations of the various pieces works
> (for example, auto_vec isn't available in libcpp).

I don't doubt it. I tried to do something similar in the past and I
failed, this is why I ended up with the poor approximation that was in
place until now. This is a significant step forward.

Is libcpp still C? When would be the time to move it to C++ already
and start using common utilities?

Also, moving vec.h, sbitmap, etc to their own directory/library so
that they can be used by other parts of the compiler (hey! maybe even
by other parts of the toolchain?) is desirable. Richard has said in
the past that he supports such moves. Did I understand correctly
Richard?

> Given that both Martin and I have candidate patches that are touching
> the same area, I'd prefer to focus on getting this code in to trunk,
> rather than rewrite it out-of-tree, so that we can at least have the
> improvement to location-handling for Wformat.  Once the code is in the
> tree, it should be easier to figure out how to access it from the
> middle-end.

Sure, I think this version is fine. I'm a big proponent of
step-by-step, even if the steps are only approximations to the optimal
solution :)
It may be enough to motivate someone else more capable to improve over
my poor approximations ;-)


>> [*] In an ideal world, we would have a language-agnostic diagnostics
>> library
>> that would include line-map and that would be used by libcpp and the
>> rest of
>> GCC, so that we can remove all the error-routines in libcpp and the
>> awkward
>> glue code that ties it into diagnostics.c.,
>
> Agreed, though that may have to wait until gcc 8 at this point.
> (Given that the proposed diagnostics library would use line maps, and
> would be used by libcpp, would it make sense to move the diagnostics
> into libcpp itself?  Diagnostics would seem to be intimately related to
> location-tracking)

I don't think so. There is nothing in diagnostic.* pretty-print.*
input.* line-map.* that requires libcpp (and only two mentions of tree
that could be easily abstracted out). This was a deliberate design
goal of Gabriel and followed by most of us later working on
diagnostics. Of course, cpp may make use of the new library, but also
other parts of the toolchain (GAS?). The main obstacle I faced when
trying to do this move was the build machinery to make both libcpp and
gcc build and statically link with this new library.

Once that move is done, the main abstraction challenge to remove the
glue is that libcpp has its own flags for options and diagnostics that
are independent from those of gcc (see c_cpp_error in c-common.c). It
would be great if libcpp used the common flags, but then one would
have to figure out a way to reorder things so that the diagnostic
library, libcpp and gcc can use (or avoid being dependent on) the same
flags.

Cheers,

Manuel.

Re: [PATCH 1/3] (v2) On-demand locations within string-literals

[Manuel López-Ibáñez]

On 26/07/16 18:11, David Malcolm wrote:


gcc/ChangeLog:
* gcc.c (cpp_options): Rename string to...
(cpp_options_): ...this, to avoid clashing with struct in
cpplib.h.


It seems to me that you need this because  now gcc.c includes cpplib.h via 
input.h, which seems wrong.


input.h was FE-independent (it depends on line-map.h but it is an accident of 
history that line-map.h is in libcpp since it doesn't depend on anything from 
libcpp [*]). Note that input.h is included in coretypes.h, so this means that 
now cpplib.h is included almost everywhere! [**]


There is the following in coretypes.h:

/* Provide forward struct declaration so that we don't have to include
   all of cpplib.h whenever a random prototype includes a pointer.
   Note that the cpp_reader and cpp_token typedefs remain part of
   cpplib.h.  */

struct cpp_reader;
struct cpp_token;

precisely to avoid including cpplib.h.


If I understand correctly, cpplib.h is needed in input.h because of this 
declaration:


+extern const char *get_source_range_for_substring (cpp_reader *pfile,
+  string_concat_db *concats,
+  location_t strloc,
+  enum cpp_ttype type,
+  int start_idx, int end_idx,
+  source_range *out_range);


Does this really need to be in input.h ?  It seems something that only C-family 
languages will be able to use. Note that you need a reader to use this 
function, and for that, you need to already include cpplib.h.


Perhaps it could live for now in c-format.c, since it is the only place using 
it?

Cheers,

Manuel.

[*] In an ideal world, we would have a language-agnostic diagnostics library 
that would include line-map and that would be used by libcpp and the rest of 
GCC, so that we can remove all the error-routines in libcpp and the awkward 
glue code that ties it into diagnostics.c.


[**] And it seems that we are slowly undoing all the work that was done by 
Andrew MacLeod to clean up the .h web and remove dependencies 
(https://gcc.gnu.org/wiki/rearch).

Re: [PATCH] Replacing gcc's dependence on libiberty's fnmatch to gnulib's fnmatch

[Manuel López-Ibáñez]

On 26 July 2016 at 14:51, ayush goel <ayushgoel1...@gmail.com> wrote:
> On 26 July 2016 at 3:38:59 AM, Manuel López-Ibáñez
> (lopeziba...@gmail.com) wrote:
>> Why the change from "fnmatch.h" to ?
>
> Gnulib doesn’t contain a header for fnmatch. It itself relies on
> glib’c fnmatch.h

I see two modules here:

https://www.gnu.org/software/gnulib/MODULES.html#module=fnmatch
https://www.gnu.org/software/gnulib/MODULES.html#module=fnmatch-gnu

Both of them generate a header file if the one from glibc is not
present or it is broken:

http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob_plain;f=modules/fnmatch

Does GCC use the GNU extensions of fnmatch? If not, you probably only
need fnmatch; otherwise you may need to also import fnmatch-gnu.

>> Also, are the files in gnulib and libiberty semantically identical?
>> The wiki page does not say anything about this. How did you check
>> this?
>
> Well the online docs for libiberty and gnulib claim the same
> definition for fnmatch. Apart from this I’ve manually gone through the
> source code and they seem to be semantically similar.

Ah, good! You should mention this in future submissions (and in the wiki page).

> Also the fact that the system builds fine and the tests also execute
> fine could serve as a manifestation for the fact that they are
> semantically same.

This is a necessary condition but not a sufficient condition.
Unfortunately, comparing the two functions in detail is still
necessary even if the tests succeed.

>> GCC can run on other systems besides OSX and GNU/Linux, how can you
>> test that your change does not break anything on those systems?
>>
> Well I have access to these two systems only. How would you suggest I
> test my patches on all possible systems?

Well, you could find fnmatch.h in your system and remove/rename it
temporarily (also remove the libiberty versions under include/), then
try to bootstrap and see what happens. If the system fnmatch.h is not
used, the build system of gnulib should create a funmatch.h (check if
it is created, otherwise GCC found a different fnmatch.h).

Cheers,

Manuel.

Re: [PATCH] Replacing gcc's dependence on libiberty's fnmatch to gnulib's fnmatch

[Manuel López-Ibáñez]

On 25 July 2016 at 18:18, ayush goel  wrote:
> On top of the previously filed patch for importing gnulib (the link
> isn’t available on the archive yet, however this contains some of the
> information: 
> http://gcc.1065356.n5.nabble.com/Importing-gnulib-into-the-gcc-tree-td1275807.html#a1279573)
> now I have replaced another function from libiberty with the
> corresponding version from gnulib.
> Even though in both OSX and GNU/Linux, fnmatch is provided by the GNU
> libc already, so the copy in libiberty is not used in your systems.
> However since the objective is to replace whatever functions can be
> leveraged by gnulib, these changes have been made.

Why the change from "fnmatch.h" to ?

Also, are the files in gnulib and libiberty semantically identical?
The wiki page does not say anything about this. How did you check
this?

GCC can run on other systems besides OSX and GNU/Linux, how can you
test that your change does not break anything on those systems?

Cheers,

Manuel.

Re: [PATCH build/doc] Replacing libiberty with gnulib

[Manuel López-Ibáñez]

On 20 July 2016 at 19:21, ayush goel  wrote:
> Hey,
> As a first step of my GSOC project
> (https://gcc.gnu.org/wiki/replacelibibertywithgnulib) I have imported
> the gnulib library inside the gcc tree. I have created gnulib as a top
> level directory which contains the necessary scripts to import the
> modules. It also contains the necessary Makefile.in and configure.ac
> files.

Looks good to me, but I cannot approve it. Joseph, what do you think?

Minor nit: It should be as follows (you can also use the script contrib/mklog )

2016-07-20 Ayush Goel 

* Makefile.def: Add gnulib as build & host library and dependency of
all-gcc on gnulib.
...

Re: RFA: new pass to warn on questionable uses of alloca() and VLAs

[Manuel López-Ibáñez]

On 19 July 2016 at 18:47, Jeff Law <l...@redhat.com> wrote:
> On 07/17/2016 09:52 AM, Manuel López-Ibáñez wrote:
>>
>> +  if (is_vla)
>> +gcc_assert (warn_vla_limit > 0);
>> +  if (!is_vla)
>> +gcc_assert (warn_alloca_limit > 0);
>>
>> if-else ? Or perhaps:
>
> Shouldn't really matter, except perhaps in a -O0 compilation.  Though I
> think else-if makes it slightly clearer.

Of course, I mentioned it because of clarity. It was difficult to
distinguish !i versus (i in my screen and I had to stop to read it
again.

Cheers,

Manuel.

Re: Importing gnulib into the gcc tree

[Manuel López-Ibáñez]

On 16 July 2016 at 10:54, ayush goel  wrote:
> Hi,
> Thanks for the feedbacks.
>
> —> I’m already configuring gcc with multiple languages and multilib enabled
>
> —> The changes have been bootstrapped and regression tested (complete check, 
> make -k -j20 check).
>
> —> As mentioned, I have locally removed obstack.[ch] from libiberty and built 
> and tested the entire thing.
>
> PFA the patch

This sounds great to me, but I cannot approve it. I hope some of the
people who can will comment on it.

One thing that I miss is documenting gnulib in doc/sourcebuild.texi.
It would be good to document in particular how to add a new module.
GDB has: 
https://sourceware.org/gdb/wiki/DeveloperTips#Updating_GDB.27s_import_of_gnulib
but I think this info should be in sourcebuild.texi (or somewhere else
under doc/).

 I see several other mentions of libiberty in doc/, but some of them
may be just using libiberty as an example, thus not relevant.

Cheers,

Manuel.

Re: RFA: new pass to warn on questionable uses of alloca() and VLAs

[Manuel López-Ibáñez]

On 15/07/16 18:05, Aldy Hernandez wrote:

+case OPT_Walloca_larger_than_:
+  if (!value)
+   inform (loc, "-Walloca-larger-than=0 is meaningless");
+  break;
+
+case OPT_Wvla_larger_than_:
+  if (!value)
+   inform (loc, "-Wvla-larger-than=0 is meaningless");
+  break;
+

We don't give similar notes for any of the other Wx-larger-than= options. If 
-Wvla-larger-than=0 suppresses a previous -Wvla-larger-than=, then it doesn't 
seem meaningless, but a useful thing to have.


+  if (is_vla)
+gcc_assert (warn_vla_limit > 0);
+  if (!is_vla)
+gcc_assert (warn_alloca_limit > 0);

if-else ? Or perhaps:

gcc_assert (!is_vla || warn_vla_limit > 0);
gcc_assert (is_vla || warn_alloca_limit > 0);

--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -275,6 +275,15 @@ Wall
 C ObjC C++ ObjC++ Warning
 Enable most warning messages.

+Walloca
+C ObjC C++ ObjC++ Var(warn_alloca) Warning
+
+Walloca-larger-than=
+C ObjC C++ ObjC++ Var(warn_alloca_limit) Warning Joined RejectNegative UInteger
+-Walloca-larger-than= Warn on unbounded uses of
+alloca, and on bounded uses of alloca whose bound can be larger than
+ bytes.

No description for Walloca.

+ if (warn_alloca)
+   warning_at (loc, OPT_Walloca, "use of alloca");
+ continue;

Since alloca is a source code entity, it would be good to quote it using %< %> 
(this hints translators to not translate it).



+ const char *alloca_str
+   = is_vla ? "variable-length array" : "alloca";
+ char buff[WIDE_INT_MAX_PRECISION / 4 + 4];
+ switch (w)
+   {
+   case ALLOCA_OK:
+ break;
+   case ALLOCA_BOUND_MAYBE_LARGE:
+ gcc_assert (assumed_limit != 0);
+ if (warning_at (loc, wcode,
+ "argument to %s may be too large", alloca_str))
+   {
+ print_decu (assumed_limit, buff);
+ inform (loc, "limit is '%u' bytes, but argument may be '%s'",
+ is_vla ? warn_vla_limit : warn_alloca_limit, buff);
+   }
+ break;
+   case ALLOCA_BOUND_DEFINITELY_LARGE:
+ gcc_assert (assumed_limit != 0);
+ if (warning_at (loc, wcode,
+ "argument to %s is too large", alloca_str))
+   {
+ print_decu (assumed_limit, buff);
+ inform (loc, "limit is %u' bytes, but argument is '%s'",
+ is_vla ? warn_vla_limit : warn_alloca_limit, buff);
+   }
+ break;

https://gcc.gnu.org/codingconventions.html#Diagnostics :

All diagnostics should be full sentences without English fragments substituted 
in them, to facilitate translation.


Example:

if (warning_at (loc, wcode,
is_vla ? "argument to variable-length array may be too large"
   : "argument to % may be too large"))

+ print_decu (assumed_limit, buff);
+ inform (loc, "limit is '%u' bytes, but argument may be '%s'",
+ is_vla ? warn_vla_limit : warn_alloca_limit, buff);
+ }

https://gcc.gnu.org/wiki/DiagnosticsGuidelines#Quoting :
Other elements such as numbers that do no refer to numeric constants that 
appear in the source code should not be quoted.


+ warning_at (loc, wcode, "argument to %s may be too large due to "
+ "conversion from '%T' to '%T'",
+ alloca_str, invalid_casted_type, size_type_node);

From the same link:

Text should be quoted by either using the q modifier in a directive such as 
%qE, or by enclosing the quoted text in a pair of %< and %> directives, and 
never by using explicit quote characters. The directives handle the appropriate 
quote characters for each language and apply the correct color or highlighting.


I don't think the above are critical problems, they could be fixed by a follow 
up patch.


Cheers,
Manuel.

Re: [PATCH] Avoid invoking ranlib on libbackend.a

[Manuel López-Ibáñez]

On 17/07/16 15:43, Patrick Palka wrote:

On Sun, Jul 17, 2016 at 9:15 AM, David Edelsohn  wrote:

You repeatedly are making bad assumptions and assertions without
having studied much about GCC. You assume that GNU ar is the only
archiver in use. You propose removing libbackend.a without having
investigated when it was introduced and why.

Your patches would be a lot more compelling if you invested the time
to learn some context.


The only patch I officially proposed is the one elides the invocation
of ranlib if the current ar is GNU ar (thanks to those who kindly
mentioned that other ar's are supported).  Do you have any comments
about this patch?

And you're right.  I am sorry for suggesting ways to improve rebuild
times without first uncovering the undocumented intricacies of the
build system.  Shame on me!


"Look at the bright side. Technical discussions sometimes appear harsh and dry 
to newcomers. Moreover, negative opinions are more vocal than positive ones. 
Thus, something that most people think is a good idea or they are indifferent 
may only get negative feedback from a few. Take this into account when judging 
how other people evaluate your ideas."

Point 3 https://gcc.gnu.org/wiki/Community

(I will extend the above with a few sentences about how tone is lost in email 
and one should always interpret criticism in the most friendly manner. Perhaps 
also extract anything useful from: 
http://producingoss.com/en/communications.html#writing-tone)


For what is worth, I commend your attempts at improving build times and I don't 
think you need to be sorry. On the other hand, it is to be expected that people 
may not welcome with open arms suggestions that may break GCC for them.


You are certainly right that there is a lot of undocumented/cargo-cult stuff in 
GCC. Unfortunately, what usually happens is that once the "newbie" becomes an 
"expert", the desire to document evaporates (I'm guilty also of this).


Why not get your "elide ranlib if possible" patch in first? Then, start a 
different discussion about whether libbackend.a is really necessary. I would 
actually prefer if GCC built some parts as libraries that could be re-used by 
other free-software projects rather than as a work-around:


https://gcc.gnu.org/wiki/rearch
https://gcc.gnu.org/wiki/ModularGCC#Middle-end_modules

Happy hacking,

Manuel.

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Manuel López-Ibáñez]

On 01/07/16 19:15, Martin Sebor wrote:
+ /* Differentiate between an exact and inexact buffer overflow
+or truncation.  */
+ const char *fmtstr;
+ if (res->number_chars < 0)
+   fmtstr = info->bounded
+ ? "output may be truncated at or before format character "
+   "%qc at offset %qlu past the end of a region of size %qlu"
+ : "writing format character %qc at offset %qlu "
+   "in a region of size %qlu";
+ else
+   fmtstr = info->bounded
+ ? "output truncated at format character %qc at offset %qlu "
+   "just past the end of a region of size %qlu"
+ : "writing format character %qc at offset %qlu "
+   "just past the end of a region of size %qlu";
+ warning_at (loc, OPT_Wformat_length_, fmtstr,
+ format_chars [-1], off - 1,
+ (unsigned long)info->objsize);
+   }


I'm not sure gettext can parse the text of format strings given like this. It 
may be smarter enough if the conditional expression is directly the argument to 
warning_at. GCC's -Wformat has the same limitations. Of course, the fool-proof 
way is to use multiple calls to warning_at.


Cheers,
Manuel.

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Manuel López-Ibáñez]

On 12/07/16 16:59, Martin Sebor wrote:

You're probably right.  I suspect I have a tendency to overuse
the quotes (e.g, the -Wplacement-new warning also quotes the
sizes).  If there aren't yet (I vague recall coming across
something on the GCC Wiki but can't find it now), it would be
helpful to put in place some diagnostic style conventions like
there are for formatting code to guide us in cases like this.
I'm willing to help put the document together or add this to
it if one already exists.


https://gcc.gnu.org/wiki/DiagnosticsGuidelines

Re: RFA: new pass to warn on questionable uses of alloca() and VLAs

[Manuel López-Ibáñez]

On 11 July 2016 at 11:10, Aldy Hernandez <al...@redhat.com> wrote:
> On 07/10/2016 07:41 PM, Manuel López-Ibáñez wrote:
>>>
>>> +Walloca
>>> +LangEnabledBy(C ObjC C++ ObjC++)
>>> +; in common.opt
>>> +
>>> +Walloca=
>>> +LangEnabledBy(C ObjC C++ ObjC++)
>>> +; in common.opt
>>> +
>>
>>
>> I'm not sure what you think the above means, but this is an invalid use
>> of LangEnabledBy(). (It would be nice if the .awk scripts were able to
>> catch it and give an error.)
>
>
> I was following the practice for -Warray-bounds in c-family/c.opt:
>
> Warray-bounds
> LangEnabledBy(C ObjC C++ ObjC++,Wall)
> ; in common.opt
>
> Warray-bounds=
> LangEnabledBy(C ObjC C++ ObjC++,Wall,1,0)
> ; in common.opt

The ones you quoted mean: For that list of languages, -Warray-bounds
is enabled by -Wall.

https://gcc.gnu.org/onlinedocs/gccint/Option-properties.html#Option-properties

But the entries that you added do not specify an option. It should
give an error by the *.awk scripts that parse .opt files. I'm actually
not sure what the scripts are generating in your case.

> I *thought* you defined the option in common.opt, and narrowed the language
> variants in c-family/c.opt.  ??

No, options that are language specific just need to be defined for the
respective FEs using the respective language flags: Wvla is an example
of that. It doesn't appear in common.opt.

Adding language flags to a common option is just redundant (again,
this is not what LangEnabledBy is doing).

Cheers,

Manuel.

Re: RFA: new pass to warn on questionable uses of alloca() and VLAs

[Manuel López-Ibáñez]

+Walloca
+LangEnabledBy(C ObjC C++ ObjC++)
+; in common.opt
+
+Walloca=
+LangEnabledBy(C ObjC C++ ObjC++)
+; in common.opt
+


I'm not sure what you think the above means, but this is an invalid use of 
LangEnabledBy(). (It would be nice if the .awk scripts were able to catch it 
and give an error.)




+;; warn_vla == 0 for -Wno-vla
+;; warn_vla == -1 for nothing passed.
+;; warn_vla == -2 for -Wvla passed
+;; warn_vla == NUM for -Wvla=NUM
 Wvla
 C ObjC C++ ObjC++ Var(warn_vla) Init(-1) Warning
 Warn if a variable length array is used.

+Wvla=
+C ObjC C++ ObjC++ Var(warn_vla) Warning Joined RejectNegative UInteger
+-Wvla= Warn on unbounded uses of variable-length arrays, and
+warn on bounded uses of variable-length arrays whose bound can be
+larger than  bytes.
+


This overloading of warn_vla seems confusing (as shown by all the places that 
require updating). Why not call it Wvla-larger-than= and use a different 
warn_vla_larger_than variable? We already have -Wlarger-than= and 
-Wframe-larger-than=.


Using warn_vla_larger_than (even if you wish to keep -Wvla= as the option name) 
as a variable distinct from warn_vla would make things simpler:


-Wvla => warn_vla == 1
-Wno-vla => warn_vla == 0
-Wvla=N => warn_vla_larger_than == N (where N == 0 means disable).

If you wish that -Wno-vla implies -Wvla=0 then you'll need to handle that 
manually. I don't think we have support for that in the .opt files. But that 
seems far less complex than having a single shared Var().


Cheers,
Manuel.

Re: Importing gnulib into the gcc tree

[Manuel López-Ibáñez]

On 10 July 2016 at 17:04, Manuel López-Ibáñez <lopeziba...@gmail.com> wrote:
> Hi Ayush,
>
> Some suggestions:

Also, it may be a good idea to configure with:

--enable-languages=c,ada,c++,java,go,fortran,objc,obj-c++ --enable-multilib

so that you get as much coverage as possible.

Cheers,

Manuel.

Re: Importing gnulib into the gcc tree

[Manuel López-Ibáñez]

Hi Ayush,

Some suggestions:

* When resubmitting a patch, also add again the Changelog.
* Use '.diff' or '.patch' as an extension, so that mail readers can
open the file as text.
* Mention that you have a copyright assignment in place already (I'm
assuming you have one already, no?).
* Mention how you regression tested your changes (on which targets?)
* In the case of these changes, and as further testing, it would be
good if you tried locally to remove obstack.[ch] from libiberty to
make sure nothing in GCC is using it. I don't think we can actually
remove it because it may be used by other projects using libiberty,
however, as your first submission showed, it is not always evident
that everything in GCC is using the gnulib version.

Cheers,

Manuel.


On 8 July 2016 at 20:30, ayush goel <ayushgoel1...@gmail.com> wrote:
> Yes, that’s correct. It has been moved before the libiberty library in the 
> list now. Bootstrapped the system with the changes as well.
>
> PFA the updated patch
>
> --
> Thanks,
> Ayush Goel
>
> On 8 July 2016 at 2:29:04 AM, Manuel López-Ibáñez (lopeziba...@gmail.com) 
> wrote:
>> On 7 July 2016 at 13:48, ayush goel wrote:
>> > In order to show the setup works, I’ve replaced libiberty’s version by 
>> > obstack by gnulib’s.
>> This was made possible by replacing the corresponding header file and then 
>> including
>> gnulib headers and gnulib static library in the build path required to 
>> compile gcc files.
>>
>> Hi Ayush,
>>
>> I'm not an expert on the build machinery, so this question might be
>> misguided: How do you know it is using the version in gnulib rather
>> than the one in libiberty? I see it uses gnulib's header file but:
>>
>> # Dependencies on the intl and portability libraries.
>> LIBDEPS= libcommon.a $(CPPLIB) $(LIBIBERTY) $(LIBINTL_DEP) $(LIBICONV_DEP) \
>> - $(LIBDECNUMBER) $(LIBBACKTRACE)
>> + $(LIBDECNUMBER) $(LIBBACKTRACE) $(LIBGNU)
>>
>> makes me think that the code in libiberty is found before the one in libgnu.
>>
>> Cheers,
>>
>> Manuel.
>>

Re: Importing gnulib into the gcc tree

[Manuel López-Ibáñez]

On 7 July 2016 at 13:48, ayush goel  wrote:
> In order to show the setup works, I’ve replaced libiberty’s version by 
> obstack by gnulib’s. This was made possible by replacing the corresponding 
> header file and then including gnulib headers and gnulib static library in 
> the build path required to compile gcc files.

Hi Ayush,

I'm not an expert on the build machinery, so this question might be
misguided: How do you know it is using the version in gnulib rather
than the one in libiberty? I see it uses gnulib's header file but:

# Dependencies on the intl and portability libraries.
 LIBDEPS= libcommon.a $(CPPLIB) $(LIBIBERTY) $(LIBINTL_DEP) $(LIBICONV_DEP) \
- $(LIBDECNUMBER) $(LIBBACKTRACE)
+ $(LIBDECNUMBER) $(LIBBACKTRACE) $(LIBGNU)

makes me think that the code in libiberty is found before the one in libgnu.

Cheers,

Manuel.

Re: [PATCH 2/2] C++ FE: handle misspelled identifiers and typenames

[Manuel López-Ibáñez]

On 30/06/16 19:53, David Malcolm wrote:

This is a port of the C frontend's r237714 [1] to the C++ frontend:
   https://gcc.gnu.org/ml/gcc-patches/2016-06/msg01052.html
offering spelling suggestions for misspelled identifiers, macro names,
and some keywords (e.g. "singed" vs "signed" aka PR c/70339).


Cool!


-  error_at (location, "%qE does not name a type", id);
+  const char *suggestion = NULL;
+  if (TREE_CODE (id) == IDENTIFIER_NODE)
+suggestion = lookup_name_fuzzy (id, FUZZY_LOOKUP_TYPENAME);
+  if (suggestion)
+   {
+ gcc_rich_location richloc (location);
+ richloc.add_fixit_misspelled_id (location, suggestion);
+ error_at_rich_loc (,
+"%qE does not name a type; did you mean %qs?",
+id, suggestion);
+   }
+  else
+   error_at (location, "%qE does not name a type", id);


It should be possible to encapsulate all this suggestion logic very deep in 
diagnostics.c and replace all the above with something similar to:


const char *suggestion = NULL;
if (TREE_CODE (id) == IDENTIFIER_NODE)
suggestion = lookup_name_fuzzy (id, FUZZY_LOOKUP_TYPENAME);

error_with_suggestion(location, suggestion, "%qE does not name a type",
  "%qE does not name a type; did you mean %qs?", id);

Perhaps with a bit more effort, even make the language-specific printers handle 
the task of looking for the suggestion, replacing the above with something like:


error_with_suggestion(location, FUZZY_LOOKUP_TYPENAME,
  "%qE does not name a type",
  "%qE does not name a type; did you mean %qs?", id);



diff --git a/gcc/diagnostic-show-locus.c b/gcc/diagnostic-show-locus.c
index 7aab658..49f7f11 100644
--- a/gcc/diagnostic-show-locus.c
+++ b/gcc/diagnostic-show-locus.c
@@ -1280,9 +1280,18 @@ diagnostic_show_locus (diagnostic_context * context,
  {
pp_newline (context->printer);

-  if (!context->show_caret
-  || diagnostic_location (diagnostic, 0) <= BUILTINS_LOCATION
-  || diagnostic_location (diagnostic, 0) == context->last_location)
+  /* Do nothing if source-printing has been disabled.  */
+  if (!context->show_caret)
+return;
+
+  /* Don't attempt to print source for UNKNOWN_LOCATION and for builtins.  */
+  if (diagnostic_location (diagnostic, 0) <= BUILTINS_LOCATION)
+return;
+
+  /* Don't print the same source location twice in a row, unless we have
+ fix-it hints.  */
+  if (diagnostic_location (diagnostic, 0) == context->last_location
+  && diagnostic->richloc->get_num_fixit_hints () == 0)
  return;

context->last_location = diagnostic_location (diagnostic, 0);


This seems independent of the suggestion stuff and could be committed 
separately.


--- a/gcc/spellcheck-tree.c
+++ b/gcc/spellcheck-tree.c
@@ -22,6 +22,7 @@ along with GCC; see the file COPYING3.  If not see
  #include "coretypes.h"
  #include "tm.h"
  #include "tree.h"
+#include "cpplib.h"


You make a language-independent file depend on cpplib.h. Wouldn't it be better 
to move c-specific stuff to a new c-family/c-spellcheck.c ?


Cheers,

Manuel.

Re: [PATCH] PR driver/69265: add hint for options with misspelled arguments

[Manuel López-Ibáñez]

On 10/05/16 22:03, David Malcolm wrote:

On Tue, 2016-05-10@22:09 +0200, Bernhard Reutner-Fischer wrote:

On Mon, May 09, 2016@08:14:47PM -0400, David Malcolm wrote:


-  inform (loc, "valid arguments to %qs are: %s", option
->opt_text, s);
+  const char *hint = find_closest_string (arg, );
+  if (hint)
+   inform (loc, "valid arguments to %qs are: %s; did you mean
%qs?",
+   option->opt_text, s, hint);
+  else
+   inform (loc, "valid arguments to %qs are: %s", option
->opt_text, s);
+


btw.. did you consider a format specifier for this ";did you mean
%qs"
common stanza?

inform (loc, "valid arguments to %qs are: %s%qhs", x, y, hint);
(let's say) where the specifier would end up empty if hint was NULL.


It would save a conditional, but it seems a bit too much like "magic"
to me; I prefer the explicitness of the existing approach.


Also, I'm pretty sure that %qX will print `; did you mean hint?', that is, with 
the quotes all messed up. GCC has the same issue when printing typedefs (aka). 
GCC prints:


error: member reference base type 'pid_t {aka int}' is not a structure or union

instead of the more correct quoting used by Clang:

error: member reference base type 'pid_t' (aka 'int') is not a structure or 
union

But perhaps a wrapper or a dedicated overload can hide some of the details 
within diagnostic.c without requiring a new format specifier:


inform_with_hint (const char *target, const auto_vec *candidates,
  location_t, const char *gmsgid, ...)

Eventually, we should move to an API like the rich_location one, where 
diagnostics can be explicitly constructed, properties can be dynamically added 
and then the diagnostic is explicitly emitted.


Perhaps even an API that uses the Named Parameter Idiom?

diagnostic.at(rich_loc)
  .with_hint(target, candidates)
  .inform("something").print()

Cheers,
Manuel.

Re: [PATCH, cpp] Fix pr61817 and 69391

[Manuel López-Ibáñez]

On 05/04/16 17:22, Richard Henderson wrote:

These two related PRs are all about remembering where a macro is expanded.
Worse, we've got two competing goals -- the real location of the expansion, for
__LINE__, and the virtual location of the expansion, for diagnostics.

There seems to be no way to unify the two competing goals.  If we simply "fix"
the first, we break the second.  Therefore, I resort to passing down both
locations.



+++ b/gcc/testsuite/gcc.dg/pr61817.c
@@ -0,0 +1,19 @@
+/* { dg-do compile } */
+/* { dg-options "-std=c11 -ftrack-macro-expansion=0" } */
+


Why use -ftrack-macro-expansion=0? This should work with =1, which is also the 
default, no?


Cheers,

Manuel.

Re: C++ PATCH for c++/70449 (ICE when printing a filename of unknown location)

[Manuel López-Ibáñez]

On 30 March 2016 at 23:42, Manuel López-Ibáñez <lopeziba...@gmail.com> wrote:
> On 30/03/16 17:14, Marek Polacek wrote:
>>
>> This test ICEs since the addition of the assert in pp_string which ensures
>> that
>> we aren't trying to print an empty string.  But that's what happens here,
>> the
>> location is actually UNKNOWN_LOCATION, so LOCATION_FILE on that yields
>> null.
>> Fixed byt not trying to print the filename of UNKNOWN_LOCATION.

> Even if we accept the broken location for now (adding some FIXME to the code
> would help the next person to realise this is not normal), if
> LOCATION_FILE() is NULL, we should print "progname" like
> diagnostic_build_prefix() does. Moreover, the filename string should be
> built with file_name_as_prefix() to get correct coloring.

Even better: Use "f ? f : progname" in file_name_as_prefix() and
simplify the code to:

  /* FIXME: Somehow we may get UNKNOWN_LOCATION here: See
g++.dg/cpp0x/constexpr-70449.C */
  const char * prefix = file_name_as_prefix (context,
LOCATION_FILE (location));
pp_verbatim (context->printer,
 TREE_CODE (p->decl) == TREE_LIST
 ? _("%s: In substitution of %qS:\n")
 : _("%s: In instantiation of %q#D:\n"),
 prefix, p->decl);
  free (prefix);

Fixes the ICE, adds colors, mentions the broken location and does not
add extra strings.

Cheers,

Manuel.

Re: C++ PATCH for c++/70449 (ICE when printing a filename of unknown location)

[Manuel López-Ibáñez]

On 30/03/16 17:14, Marek Polacek wrote:

This test ICEs since the addition of the assert in pp_string which ensures that
we aren't trying to print an empty string.  But that's what happens here, the
location is actually UNKNOWN_LOCATION, so LOCATION_FILE on that yields null.
Fixed byt not trying to print the filename of UNKNOWN_LOCATION.


How it can be UNKNOWN_LOCATION? It has to be somewhere in the input file!

This is what Clang prints:

prog.cc:5:3: warning: type definition in a constexpr function is a C++14 
extension [-Wc++14-extensions]

  enum E { a = f<0> () };
  ^

This is what we print:

In instantiation of 'constexpr int f() [with int  = 0]':
prog.cc:5:22:   required from here
cc1plus: error: body of constexpr function 'constexpr int f() [with int 
 = 0]' not a return-statement


This is very broken: The fact that input_location is UNKNOWN_LOCATION makes us 
print "cc1plus" in the error.


Even if we accept the broken location for now (adding some FIXME to the code 
would help the next person to realise this is not normal), if LOCATION_FILE() 
is NULL, we should print "progname" like diagnostic_build_prefix() does. 
Moreover, the filename string should be built with file_name_as_prefix() to get 
correct coloring.


Something like:

  const char *filename = LOCATION_FILE (location);
  /* FIXME: Somehow we may get UNKNOWN_LOCATION here: See 
g++.dg/cpp0x/constexpr-70449.C */

  if (filename == NULL) filename = progname;
  const char * prefix = file_name_as_prefix (context, filename);

pp_verbatim (context->printer,
 TREE_CODE (p->decl) == TREE_LIST
 ? _("%s: In substitution of %qS:\n")
 : _("%s: In instantiation of %q#D:\n"),
 prefix, p->decl);
  free (prefix);

The above also avoids adding yet another slightly different new string for 
translation.


Cheers,

Manuel.

Re: [PATCH] Reuse the saved_scope structures allocated by push_to_top_level

[Manuel López-Ibáñez]

On 3 March 2016 at 15:39, Patrick Palka <patr...@parcs.ath.cx> wrote:
> On Thu, Mar 3, 2016 at 10:22 AM, Manuel López-Ibáñez
> <lopeziba...@gmail.com> wrote:
>> It would be an overall improvement if it was neither a TREE_LIST, nor a
>> TREE_VECTOR: https://gcc.gnu.org/wiki/Speedup_areas#Trees
>>
>> Those kinds of cleanups are always welcome even if they do not improve
>> performance noticeably at first glance. The speed-up will show up once
>> TREE_LIST is removed completely.
>
> Ah yeah, I meant if cp_binding_level::names were a vec<tree, va_gc>
> since it would have to be resizable.

Sure, what I meant is that such a change is an improvement even if you
cannot measure any speed-up at all right now. Go for it!

Cheers,

Manuel.

Re: Proposed Patch for Bug 69687

[Manuel López-Ibáñez]

On 03/03/16 14:21, Bernd Schmidt wrote:

On 03/02/2016 06:22 PM, Mike Stump wrote:


So, check for overflow, or better use unsigned values that are large
enough to never overflow.  With no possibility for overflow, you can
then retest the bug and see if there are any other failure modes and
fix those.


What C standard can we assume for libiberty? I was looking@patching this and
discovered that SIZE_MAX is defined only for C99, so I'm leaning towards
retaining the ints and using INT_MAX.


Retaining INT_MAX should be ok in this case, since that should allow pretty 
large mangled strings. As far as I know, the only users of libiberty are GDB 
and GCC, and GDB only because they have not completely moved to gnulib yet. GCC 
is C++, GDB assumes C90 but it is moving to C++ anyway, so it could be bumped 
to SIZE_MAX later.


However, it would be much better to add to libiberty something like gnulib's 
x2realloc and x2nrealloc and use that because:


* It is more concise.
* Avoid duplication.
* libiberty should be replaced by gnulib eventually
* error-handling is shared with xrealloc, which gives both more consistency and 
more flexibility.


Of course, there is an even better fix: Add to the GCC repository enough gnulib 
modules to use directly the x2realloc from gnulib, make the demangler use that. 
GDB is already using some gnulib modules, so it should not be a problem for 
them. It is a bit more work in the short term, but re-implementing function by 
function a lower quality implementation of the whole gnulib seems much worse in 
the long run.


Cheers,

Manuel.

Re: [PATCH] Reuse the saved_scope structures allocated by push_to_top_level

[Manuel López-Ibáñez]

On 03/03/16 14:49, Patrick Palka wrote:

I think the slowness of this function is mostly due to the pointer
chasing performed in the function store_bindings, where we iterate
over all the names in each non-global scope to figure out whether to
preserve them.  It would probably improve performance if
cp_binding_level::names were a vector of trees instead of a linked
list of trees.


It would be an overall improvement if it was neither a TREE_LIST, nor a 
TREE_VECTOR: https://gcc.gnu.org/wiki/Speedup_areas#Trees


Those kinds of cleanups are always welcome even if they do not improve 
performance noticeably at first glance. The speed-up will show up once 
TREE_LIST is removed completely.


Cheers,

Manuel.

Re: [PATCH] Specify that new ports should use LRA

[Manuel López-Ibáñez]

On 2 March 2016 at 21:47, H.J. Lu <hjl.to...@gmail.com> wrote:
> On Wed, Mar 2, 2016 at 12:18 PM, Manuel López-Ibáñez
> <lopeziba...@gmail.com> wrote:
>> Pre-approved by Jeff here: 
>> https://gcc.gnu.org/ml/gcc-help/2016-03/msg6.html
>>
>> Committed as revision 233914.
>
> I checked in this missing patch.

Thanks H.J.!

Re: [PATCH] PR c/69993: improvements to wording of -Wmisleading-indentation

[Manuel López-Ibáñez]

On 02/03/16 04:20, Patrick Palka wrote:

Using this wording order makes it seem that the problem is with the if
statement, because we emit a warning about it and then emit "only" a
note for the misleadingly-indented goto statement.


... on second thought, I may be overthinking the semantic difference
between a warning and a note.  Feel free to disregard my nitpicking.


This is because the semantics of "note" are confusing. Currently it serves as a 
stand-alone note and as extra info for a warning/error. There is no way to tell 
the difference between the two except by making sense of the text of the 
diagnostic. This is probably not an issue for humans if the text is clear 
enough, but for automatic tools it is impossible to distinguish the two cases. 
There is also the issue of distinguishing two independent single-line notes 
from a multi-line note.


Ideally, we would have a different type of diagnostic

info: some informative text
note: continued here


Cheers,

Manuel.

[PATCH] Specify that new ports should use LRA

[Manuel López-Ibáñez]

Pre-approved by Jeff here: https://gcc.gnu.org/ml/gcc-help/2016-03/msg6.html

Committed as revision 233914.
Index: ChangeLog
===
--- ChangeLog   (revision 233913)
+++ ChangeLog   (working copy)
@@ -1,3 +1,7 @@
+2016-03-02  Manuel Lopez-Ibanez 
+
+   * target.def(lra_p): Specify that new ports should use LRA.
+
 2016-03-02  Jakub Jelinek  
 
PR libgomp/69555
Index: target.def
===
--- target.def  (revision 233910)
+++ target.def  (working copy)
@@ -4884,9 +4884,9 @@
 DEFHOOK
 (lra_p,
  "A target hook which returns true if we use LRA instead of reload pass.\
-  It means that LRA was ported to the target.\
   \
-  The default version of this target hook returns always false.",
+  The default version of this target hook returns always false, but new\
+  ports should use LRA.",
  bool, (void),
  default_lra_p)
 

Re: [PATCH] PR28901 Add two levels for -Wunused-const-variable.

[Manuel López-Ibáñez]

On 23/02/16 08:56, Jakub Jelinek wrote:

On Tue, Feb 23, 2016@09:53:57AM +0100, Mark Wielaard wrote:

--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,10 @@
+2016-02-23  Mark Wielaard  
+   Jakub Jelinek  
+
+   PR c/69911
+   * cgraphunit.c (check_global_declaration): Check main_input_filename
+   and DECL_SOURCE_FILE are not NULL.
+
  2016-02-20  Mark Wielaard  


This is ok for trunk if it passes testing.  Thanks.


diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index 27a073a..8b3fddc 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -917,6 +917,7 @@ walk_polymorphic_call_targets (hash_set 
*reachable_call_targets,
  static void
  check_global_declaration (symtab_node *snode)
  {
+  const char *decl_file;
tree decl = snode->decl;

/* Warn about any function declared static but not defined.  We don't
@@ -944,8 +945,10 @@ check_global_declaration (symtab_node *snode)
 || (((warn_unused_variable && ! TREE_READONLY (decl))
|| (warn_unused_const_variable > 0 && TREE_READONLY (decl)
&& (warn_unused_const_variable == 2
-   || filename_cmp (main_input_filename,
-DECL_SOURCE_FILE (decl)) == 0)))
+   || (main_input_filename != NULL
+   && (decl_file = DECL_SOURCE_FILE (decl)) != NULL
+   && filename_cmp (main_input_filename,
+decl_file) == 0



Can we please please please hide this ugliness behind an (inline?) function 
such as bool in_main_file_at (location_t) in input.[ch]? The condition here is 
quickly becoming unreadable.


Also because in the future somebody would want to re-implement this using 
MAIN_FILE_P() from line-map.h, which is faster.


Cheers,

Manuel.

Re: [C++ patch] report better diagnostic for static following '[' in parameter declaration

[Manuel López-Ibáñez]

On 29/01/16 17:01, Prathamesh Kulkarni wrote:

Thanks for the review. AFAIK the type-qualifiers would be const,
restrict, volatile and _Atomic (n1570 p 6.7.3) ?
I added a check for those and for variable length array.
I am having issues with writing the test-case,
some cases pass with -std=c++11 but fail with -std=c++98.
Could you please have a look ?


Is there _Atomic in C++?

Also, why not simply reuse cp_parser_cv_qualifier_seq_opt (cp_parser* parser), 
perhaps adding a complain parameter that defaults to tf_error and calling it 
here with tf_none.


I think you will get nicer errors if you don't set bounds to error-mark, just 
give the error, consume the tokens and continue as usual.


Ideally, smart error-recovery should only be done when things already go wrong, 
thus after


 bounds = cp_parser_constant_expression (parser,
 /*allow_non_constant=*/true,
 _constant_p);
 if (!non_constant_p)
/* OK */;

fails, however our C++ parser tends to give errors quite deep in the stack 
instead of letting the caller decide what to do, which makes this too noisy in 
this case. Nonetheless, moving this error-recovery within:

if (token->type != CPP_CLOSE_SQUARE){ }
but before the above can only make the parser (marginally) faster for correct 
code.

Cheers,

Manuel.

Re: [wwwdocs] gcc-6/changes.html: diagnostics, Levenshtein, -Wmisleading-indentation, jit (v2)

[Manuel López-Ibáñez]

On 20 January 2016 at 17:38, Gerald Pfeifer  wrote:
> On Wed, 20 Jan 2016, Jakub Jelinek wrote:
>>>   Content-Security-Policy: default-src 'self' http: https:
>>>
>>> So either we get the configuration of the web server changed, or
>>> indeed we need to touch all those existing pages.
>> At least the warning/error/note styles are something that multiple pages
>> are using and going to use in the future, so if that could be defined in
>> the main gcc.css, it would be enough.
>
> Done thusly.  With this change, at least gcc-6/changes.html should
> be fine again.
>
> And I can commit working my way backwards through all the other
> changes.html pages over the coming couple of days.

wwwdocs/htdocs$ find . -name '*.html' | xargs grep --color -e " style *="

shows a bit more inline CSS than changes.html, unfortunately.

Re: [wwwdocs] gcc-6/changes.html: diagnostics, Levenshtein, -Wmisleading-indentation, jit (v2)

[Manuel López-Ibáñez]

On 19/01/16 17:08, Gerald Pfeifer wrote:

On Fri, 15 Jan 2016, David Malcolm wrote:

Here's an updated version of the above, which the W3C validator
reports as being clean (fixing various "&" and "<" and a missing
end-tag).


Nice - and a lot of nice changes you implemented since GCC 5!



Am I the only one who doesn't see the colors at 
https://gcc.gnu.org/gcc-6/changes.html#c-family nor 
https://gcc.gnu.org/gcc-5/changes.html#fortran ?


Firefox 43.0.4 says "Content Security Policy: The page's settings blocked the 
loading of a resource at self ("default-src https://gcc.gnu.org http: https:")."


Cheers,

Manuel.

Re: [wwwdocs] gcc-6/changes.html: diagnostics, Levenshtein, -Wmisleading-indentation, jit (v2)

[Manuel López-Ibáñez]

On 19 January 2016 at 19:31, Mike Stump <mikest...@comcast.net> wrote:
> On Jan 19, 2016, at 11:05 AM, Manuel López-Ibáñez <lopeziba...@gmail.com> 
> wrote:
>>
>> Am I the only one who doesn't see the colors at 
>> https://gcc.gnu.org/gcc-6/changes.html#c-family nor 
>> https://gcc.gnu.org/gcc-5/changes.html#fortran ?
>
> Yes.  The darkslategrey of the headings is very close to black, but the links 
> should be blue.

Those colors are fine but the example diagnostics should also have
colors. They had when I added them some months ago.

Re: genattrab.c generate switch

[Manuel López-Ibáñez]

On 18/01/16 14:39, Jesper Broge Jørgensen wrote:

No i have not gone through copyright assignment.
This is my first time trying to contribute to a GNU project so i have tried
following the "Contributing to GCC"@
https://gcc.gnu.org/contribute.html
There i followed the advice to run the patch through contrib/check_GNU_style.sh
and it came out clean. Maybe contrib/check_GNU_style.sh does not check for
indention rules and/or my editor is set up wrongly so it looked to me like i
was following the coding standard.


Hi Jesper,

Unfortunately, https://gcc.gnu.org/contribute.html is quite hard to follow and 
outdated. I would suggest to start here: 
https://gcc.gnu.org/wiki/GettingStarted#Basics:_Contributing_to_GCC_in_10_easy_steps


From there, you'll get to https://gcc.gnu.org/wiki/FormattingCodeForGCC

If you know how to improve those pages, for example extending them to other 
editors, I can give you write access.


Cheers,

Manuel.

Re: [PATCH] doc: discourage use of __attribute__((optimize())) in production code

[Manuel López-Ibáñez]

On 14/12/15 16:40, Markus Trippelsdorf wrote:

On 2015.12.14@11:20 -0500, Trevor Saunders wrote:

On Mon, Dec 14, 2015@10:01:27AM +0100, Richard Biener wrote:

On Sun, Dec 13, 2015@9:03 PM, Andi Kleen  wrote:

Markus Trippelsdorf  writes:


Many developers are still using __attribute__((optimize())) in
production code, although it quite broken.


Wo reads documentation? @) If you want to discourage it better warn once
@runtime.


We're also quite heavily using it in LTO internally now.


besides that does this really make sense?  I suspect very few people are
using this for the fun of it.  I'd guess most usage is to disable
optimizations to work around bugs, or maybe trying to get a very hot
function optimized more.  Either way I suspect its only used by people
with good reason and this would just really iritate them.


Well, if you look@bugzilla you'll find several wrong code bugs caused
by this attribute, e.g.: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59262

Also Richi stated in the past (I quote):
»I consider the optimize attribute code seriously broken and
unmaintained (but sometimes useful for debugging - and only that).«

https://gcc.gnu.org/ml/gcc/2012-07/msg00201.html


It is even a FAQ: https://gcc.gnu.org/wiki/FAQ#optimize_attribute_broken

Cheers,

Manuel.

Re: [Patch, Fortran] PR68815 - replace '%s' quotes by %< ... %>

[Manuel López-Ibáñez]

On 12/09/2015 03:53 PM, Tobias Burnus wrote:

In principle, %<%c%> and %<%d%> should be convertable to %qc and
%qd (as the code is more readable), but the current function
annotation prevent this, telling that the q flag is not valid for
%c and %d. As %< is fine, I didn't dig into it.


You need to edit the gcc_gfc_* variables in c-family/c-format.c. The correct 
way to do that does not seem to be documented anywhere. I only got the current 
support working after painful trial and error.


Joseph, Jakub, could you advise the Fortran devs on how to edit c-format.c to 
handle %qc and %qd as the C/C++ FEs do?


Thanks,

Manuel

Re: PATCH to shorten_compare -Wtype-limits handling

[Manuel López-Ibáñez]

On 20 November 2015 at 22:28, Jeff Law  wrote:
>>> That was the overall plan and he posted a patch for that.  But that patch
>>> didn't do the due diligence to verify that once the shortening code was
>>> made
>>> "pure" that we didn't regress on the quality of the code we generated.
>>
>>
>> I thought that the original plan was to make the warning code also use
>> match.pd. That is, that all folding, including FE folding, will be
>> match.pd-based. Has this changed?
>
> I don't think that's changed.
>
> Detangling the two is the first step. Once detangled we can then look to
> move the warning to a more suitable location -- right now it's in the C/C++
> front-ends, firing way too early.  Instead it ought to be checked in gimple
> form, after match.pd canonicalization and simplifications.

Perhaps worth noting is that the warnings in shorten_compare are
basically trying to figure out if X CMP_OP Y can be folded to
true/false. They do not care about shortening/optimizing the types and
conversions. Perhaps an intermediate step would be to duplicate the
function into a warning version and an optimization version, making
the warning version pure as you say, call the warning version first,
then the optimizing one. Note that this is not exactly what you want
to have at the end and it adds a bit of duplication, but it gives a
clear separation between what is needed for warning and what is needed
for optimizing.This is step one.

Step two makes the warning version fold using match.pd, fix any
warning regressions. Note that the warning version does not really
need to be pure. If the comparison can be folded to true/false, there
is no much more optimization to be done. But I'm not sure how the FEs
are handling results from folding for warnings. Otherwise, one is
probably duplicating a bit of analysis between the warning and
optimizing functions. Not sure if the overhead will be measurable at
all once the warning version uses match.pd.

Step three moves the optimizing version to match.pd. Identifying
regressions here may be harder, but no harder than any other move of
optimizations to match.pd.

The attached patch implements step 1. Feel free to do as you please with it.

Cheers,

Manuel.
Index: c-common.c
===
--- c-common.c  (revision 230753)
+++ c-common.c  (working copy)
@@ -4419,10 +4419,251 @@ expr_original_type (tree expr)
 {
   STRIP_SIGN_NOPS (expr);
   return TREE_TYPE (expr);
 }
 
+static void
+warn_shorten_compare (location_t loc, tree op0, tree op1,
+ tree restype, enum tree_code rescode)
+{
+  if (!warn_type_limits)
+return;
+
+  switch (rescode)
+{
+case NE_EXPR:
+case EQ_EXPR:
+case LT_EXPR:
+case GT_EXPR:
+case LE_EXPR:
+case GE_EXPR:
+  break;
+
+default:
+  return;
+}
+
+  /* Throw away any conversions to wider types
+ already present in the operands.  */
+  int unsignedp0, unsignedp1;
+  tree primop0 = c_common_get_narrower (op0, );
+  tree primop1 = c_common_get_narrower (op1, );
+
+  /* If primopN is first sign-extended from primopN's precision to opN's
+ precision, then zero-extended from opN's precision to
+ restype precision, shortenings might be invalid.  */
+  if (TYPE_PRECISION (TREE_TYPE (primop0)) < TYPE_PRECISION (TREE_TYPE (op0))
+  && TYPE_PRECISION (TREE_TYPE (op0)) < TYPE_PRECISION (restype)
+  && !unsignedp0
+  && TYPE_UNSIGNED (TREE_TYPE (op0)))
+primop0 = op0;
+  if (TYPE_PRECISION (TREE_TYPE (primop1)) < TYPE_PRECISION (TREE_TYPE (op1))
+  && TYPE_PRECISION (TREE_TYPE (op1)) < TYPE_PRECISION (restype)
+  && !unsignedp1
+  && TYPE_UNSIGNED (TREE_TYPE (op1)))
+primop1 = op1;
+
+  /* Handle the case that OP0 does not *contain* a conversion
+ but it *requires* conversion to FINAL_TYPE.  */
+  if (op0 == primop0 && TREE_TYPE (op0) != restype)
+unsignedp0 = TYPE_UNSIGNED (TREE_TYPE (op0));
+  if (op1 == primop1 && TREE_TYPE (op1) != restype)
+unsignedp1 = TYPE_UNSIGNED (TREE_TYPE (op1));
+
+  /* If one of the operands must be float, we do not warn.  */
+  if (TREE_CODE (TREE_TYPE (primop0)) == REAL_TYPE
+  || TREE_CODE (TREE_TYPE (primop1)) == REAL_TYPE)
+return;
+
+  /* If first arg is constant, swap the args (changing operation
+ so value is preserved), for canonicalization.  Don't do this if
+ the second arg is 0.  */
+
+  if (TREE_CONSTANT (primop0)
+  && !integer_zerop (primop1) && !real_zerop (primop1)
+  && !fixed_zerop (primop1))
+{
+  std::swap (primop0, primop1);
+  std::swap (op0, op1);
+  std::swap (unsignedp0, unsignedp1);
+
+  switch (rescode)
+   {
+   case LT_EXPR:
+ rescode = GT_EXPR;
+ break;
+   case GT_EXPR:
+ rescode = LT_EXPR;
+ break;
+   case LE_EXPR:
+ rescode = GE_EXPR;
+ break;
+   case GE_EXPR:
+ rescode = LE_EXPR;
+ break;
+  

Re: [PATCH] Fix -Werror= handling for Joined warnings, add a few missing Warning keywords (PRs c/48088, c/68657)

[Manuel López-Ibáñez]

On 4 December 2015 at 17:53, Jakub Jelinek  wrote:
> +
> + if (e->unknown_error)
> +   error_at (loc, e->unknown_error, option->opt_text);
> + else
> +   error_at (loc, "unrecognized argument in option %qs",
> + option->opt_text);

The same code that handles command-line options has:

  if (e->unknown_error)
error_at (loc, e->unknown_error, decoded->arg);
  else
error_at (loc, "unrecognized argument in option %qs", opt);

My guess is that the first error_at should use arg instead of
option->opt_text to be equivalent. Of course, ideally, this code would
not be duplicated, but rather merged "somehow".

Cheers,

Manuel.

Re: PATCH to shorten_compare -Wtype-limits handling

[Manuel López-Ibáñez]

On 20 November 2015 at 17:42, Jeff Law  wrote:
> So we have to detangle the operand shortening from warning detection. Kai's
> idea was to first make the shortening code "pure" in the sense that it would
> have no side effects other than to generate the warnings.  Canonicalization
> and other transformations would still occur internally, but not be reflected
> in the IL.
>
> That was the overall plan and he posted a patch for that.  But that patch
> didn't do the due diligence to verify that once the shortening code was made
> "pure" that we didn't regress on the quality of the code we generated.

I thought that the original plan was to make the warning code also use
match.pd. That is, that all folding, including FE folding, will be
match.pd-based. Has this changed?

Cheers,

Manuel.

Re: PATCH to shorten_compare -Wtype-limits handling

[Manuel López-Ibáñez]

On 20 November 2015 at 16:10, Martin Sebor  wrote:
>>> Hmm, it looks like using expansion_point_if_in_system_header might avoid
>>> the first issue you mention.
>>
>>
>> Thus.
>
>
> Great, thanks! (I'll have to remember the trick for my own use!)

I added this to  https://gcc.gnu.org/wiki/DiagnosticsGuidelines under
"Locations" for future reference. I hope others would do the same in
the future, so the info is kept up-to-date.

Cheers,

Manuel.

Re: PATCH to shorten_compare -Wtype-limits handling

[Manuel López-Ibáñez]

On 19 November 2015 at 17:54, Jeff Law  wrote:
>> But there were a couple of patches from you some time ago, for
>> example: http://permalink.gmane.org/gmane.comp.gcc.patches/343476
>>
>> What happened with those?
>
> On hold pending fixing the type-limits warning placement.  Essentially that
> has to be untangled first.

Could you elaborate on this? Or point me to some previous email
thread? (I don't have enough free time to follow the mailing list
anymore, sorry).

Cheers,

Manuel.

Re: PATCH to shorten_compare -Wtype-limits handling

[Manuel López-Ibáñez]

On 19 November 2015 at 17:09, Jeff Law  wrote:
> The even longer term direction for this code is to separate out the
> type-limits warning from the canonicalization and shortening.  I've got a
> blob of code form Kai that goes in that direction, but it needs more
> engineering around it.
>
> Ideally the canonicalization/shortening moves into match.pd.  The warning,
> in theory, moves out of the front-ends as well.

The last attempt by Kai was here:
https://gcc.gnu.org/ml/gcc-patches/2015-09/msg01265.html

But there were a couple of patches from you some time ago, for
example: http://permalink.gmane.org/gmane.comp.gcc.patches/343476

What happened with those?

There is also https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51712 It
would be great if that was eventually fixed in the new delay-folding
world.

Cheers,

Manuel.

Re: [PATCH] Add clang-format config to contrib folder

[Manuel López-Ibáñez]

On 18/11/15 17:05, Jeff Law wrote:

As we've been continuously converting our source base to C++, the
clang-format should
provide better results than a collection of regular expressions
(check_GNU_style.sh).

As a reference file I attach gcc/tree-ssa-uninit.c file.
Feel free to comment the suggested configuration file.

This is fine.  Given that gnu-indent seems to muck up C++ badly in my
experience, clang-format may be a better long term solution.  I'd really like
to get to a point one day where formatting is a commit hook so that things are
always kept properly formatted.


Which is a sad demonstration of how the refusal of GCC's FEs being re-used for 
other purposes by GNU tools (and others) was and is a mistake, and it is 
leading to GNU tools being replaced by LLVM-based ones (ultimately affecting 
GCC and GDB themselves).


Cheers,

Manuel.

Re: [C++ PATCH] Issue hard error even with -fpermissive for certain goto violations (PR c++/67409)

[Manuel López-Ibáñez]

On 18/11/15 22:55, Jakub Jelinek wrote:


  static bool
-identify_goto (tree decl, const location_t *locus)
+identify_goto (tree decl, const location_t *locus, bool harderr)
  {
-  bool complained = (decl
-? permerror (input_location, "jump to label %qD", decl)
-: permerror (input_location, "jump to case label"));
+  bool complained;
+  if (!harderr)
+{
+  if (decl)
+   complained = permerror (input_location, "jump to label %qD", decl);
+  else
+   complained = permerror (input_location, "jump to case label");
+}
+  else
+{
+  if (decl)
+   error ("jump to label %qD", decl);
+  else
+   error ("jump to case label");
+  complained = true;
+}
if (complained && locus)
  inform (*locus, "  from here");
return complained;


The above is a bit repetitive. Why not simply?

static bool
error_jumpto (diagnostic_t kind, location_t loc, tree decl)
{
  bool complained = (decl
 ? emit_diagnostic (kind, input_location, 0,
"jump to label %qD", decl)
 : emit_diagnostic (kind, input_location, 0,
"jump to case label"));
  if (complained && loc)
inform (loc, " from here");
  return complained;
}

That is, call a function that gives errors about X, error_X; no point in 
passing a pointer to location_t; most diagnostic functions take loc as the 
first argument; no obscure bool parameter. Then call:



@@ -2991,15 +3004,16 @@ check_previous_goto_1 (tree decl, cp_bin
   bool exited_omp, const location_t *locus)
  {
cp_binding_level *b;
-  bool identified = false, complained = false;
+  bool complained = false;
+  int identified = 0;
bool saw_eh = false, saw_omp = false, saw_tm = false;

if (exited_omp)
  {
-  complained = identify_goto (decl, locus);
+  complained = identify_goto (decl, locus, true);


complained = error_jumpto (DK_ERROR, loc, decl);


+ complained = identify_goto (decl, locus, false);


complained = error_jumpto (DK_PERMERROR, loc, decl);



+  if (ent->in_try_scope || ent->in_catch_scope
+ || ent->in_transaction_scope || ent->in_omp_scope)
+   {
+ error_at (DECL_SOURCE_LOCATION (decl), "jump to label %qD", decl);
+ complained = true;
+ identified = 2;
+   }
+  else
+   {
+ complained = permerror (DECL_SOURCE_LOCATION (decl),
+ "jump to label %qD", decl);
+ identified = 1;
+   }
   if (complained)
inform (input_location, "  from here");


Note that if the function above takes another location_t argument, you can also 
simplify this hunk to:


  diagnostic_t kind;
  if (ent->in_try_scope || ent->in_catch_scope
  || ent->in_transaction_scope || ent->in_omp_scope)
{
  kind = DK_ERROR;
  identified = 2;
}
   else
{
  kind = DK_PERMERROR;
  identified = 1;
}
complained = error_jumpto (kind, loc, DECL_SOURCE_LOCATION (decl), 
decl);

You can even use kind (maybe 'error_kind') directly instead of identified for 
what you are trying to achieve, with error_kind in {DK_UNSPECIFIED, DK_ERROR, 
DK_PERMERROR}.





FOR_EACH_VEC_SAFE_ELT (ent->bad_decls, ix, bad)
@@ -3155,6 +3180,14 @@ check_goto (tree decl)
if (u > 1 && DECL_ARTIFICIAL (bad))
{
  /* Can't skip init of __exception_info.  */
+ if (identified == 1)
+   {
+ error_at (DECL_SOURCE_LOCATION (decl),
+   "jump to label %qD", decl);
+ inform (input_location, "  from here");
+ complained = true;
+ identified = 2;
+   }


and here:

 kind = DK_ERROR;
 complained = error_jumpto (kind, input_location,
DECL_SOURCE_LOCATION (decl), decl);


  if (complained)
inform (DECL_SOURCE_LOCATION (bad), "  enters catch block");
  saw_catch = true;
@@ -3195,13 +3228,13 @@ check_goto (tree decl)
break;
  if (b->kind == sk_omp)
{
- if (!identified)
+ if (identified < 2)
{
- complained = permerror (DECL_SOURCE_LOCATION (decl),
- "jump to label %qD", decl);
- if (complained)
-   inform (input_location, "  from here");
- identified = true;
+ error_at (DECL_SOURCE_LOCATION (decl),
+   "jump to label %qD", decl);
+ inform (input_location, "  from here");
+ complained = true;
+ identified = 2;
}


and the same here.

Cheers,

Manuel.

Re: [PATCH c/c++] use explicit locations for some warnings in c-pragma.c

[Manuel López-Ibáñez]

On 4 November 2015 at 09:45, Mike Stump  wrote:
> in the top of the tree.  This is bad as the same line appears in a PASS: and 
> an XFAIL:.  Each test case should be unique.  Should it be updated to 64?

I think it is sufficient to change it to:

/* { dg-warning "24:missing" "wrong column" { xfail *-*-* }  2 } */

This dg-warning is there to show that the column number is wrong and
tell whoever fixes this that there is already a test that only needs
updating. Changing 24 to 64 defeats the purpose of having it in the
first place.

Cheers,

Manuel.

Re: [PATCH] v4 of diagnostic_show_locus and rich_location

[Manuel López-Ibáñez]

On 12 October 2015 at 16:44, David Malcolm  wrote:
> v4 of the patch does the conversion of Fortran, and eliminates the
> adaptation layer.  No partial transitions here!
>
> Manu: I hope this addresses your concerns.

Yes, it looks great. I don't understand how this

-   and for two locations that do not fit in the same locus line:
-
-   [name]:[locus]: Error: (1)
-   [name]:[locus2]: Error: Some error at (1) and (2)
+   [locus of primary range]: Error: Some error at (1) and (2)


passes the Fortran regression testsuite since the testcases normally
try to match the two locus separately, but I guess you figured out a
way to make it work and I must admit I did not have the time to read
the patch in deep detail. But it is a bit strange that you also
deleted this part:

-   With -fdiagnostic-show-caret (the default) and for valid locations,
-   it prints for one location:
+   With -fdiagnostic-show-caret (the default) it prints:

-   [locus]:
+   [locus of primary range]:

   some code
  1
Error: Some error at (1)

-   for two locations that fit in the same locus line:
+  With -fno-diagnostic-show-caret or if the primary range is not
+  valid, it prints:

-   [locus]:
-
- some code and some more code
-1   2
-   Error: Some error at (1) and (2)
-
-   and for two locations that do not fit in the same locus line:
-
-   [locus]:
-
- some code
-1
-   [locus2]:
-
- some other code
-   2
-   Error: Some error at (1) and (2)
-

which should work the same before and after your patch. Independently
of whether the actual logic moved into some new mechanism in the new
rich locations world, this seems like useful info to keep in
fortran/error.c.

Cheers,

Manuel.

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

[Manuel López-Ibáñez]

On 25 September 2015 at 17:14, Dodji Seketeli  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.

I could not make it work by touching directive_result.type. However,
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. But if we register the pragma in the FE to
ignore it, then we get

cc1plus: internal compiler error: #pragma GCC diagnostic is already registered

Cheers,

Manuel.

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

[Manuel López-Ibáñez]

On 25 September 2015 at 22:11, David Malcolm  wrote:
>>
>> +  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.

Functions that are useful only for debugging GCC usually start with
debug_* and have special attribute annotation (grep ^debug_) which
prevents those kinds of warnings (or the optimizers being too smart
and removing them).

Cheers,

Manuel.

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

[Manuel López-Ibáñez]

On 25 September 2015 at 22:18, Manuel López-Ibáñez
<lopeziba...@gmail.com> wrote:
> On 25 September 2015 at 22:11, David Malcolm <dmalc...@redhat.com> wrote:


   context->last_location = diagnostic_location (diagnostic, 0);
-  expanded_location s0 = diagnostic_expand_location (diagnostic, 0);
-  expanded_location s1 = { };
-  /* Zero-initialized. This is checked later by
diagnostic_print_caret_line.  */

-  if (diagnostic_location (diagnostic, 1) > BUILTINS_LOCATION)
-s1 = diagnostic_expand_location (diagnostic, 1);
+  if (context->frontend_calls_diagnostic_print_caret_line_p)
+{
+  /* The GCC < 6 routine. */
+  expanded_location s0 = diagnostic_expand_location (diagnostic, 0);
+  expanded_location s1 = { };
+  /* Zero-initialized. This is checked later by
+ diagnostic_print_caret_line.  */
+
+  if (diagnostic_num_locations (diagnostic) >= 2)
+s1 = diagnostic->message.m_richloc->get_range (1)->m_start;

-  diagnostic_print_caret_line (context, s0, s1,
-   context->caret_chars[0],
-   context->caret_chars[1]);
+  diagnostic_print_caret_line (context, s0, s1,
+   context->caret_chars[0],
+   context->caret_chars[1]);
+}
+  else
+/* The GCC >= 6 routine.  */
+diagnostic_print_ranges (context, diagnostic);
 }


I haven't had time to look at the patch in detail, so please excuse me
if this is answered elsewhere.

Why do you need this hack? The whole point of moving Fortran to the
common machinery is to not have this duplication.

Can't the new code print one caret without ranges ever? Something like:

error: expected ';'
  }
   ^

If it can, then the function responsible for doing that can be called
by Fortran and it should replace diagnostic_print_caret_line.

Or is it that the new diagnostic_print_ranges cannot print multiple
carets in the same line? Like this

error: error at (1) and (2)
  adfadfafd asdfdaffa
   12

If this is the case, this is a missing functionality that
diagnostic_print_caret_line already has and that was ready to be used
in C/C++. See example at  (O) here:
https://gcc.gnu.org/wiki/Better_Diagnostics

In my mind, it should be possible for Fortran to pass to the
diagnostics machinery two locations with range width 1 (or 0,
depending how you want to represent a range that covers exactly one
char) and get a caret line like the example above. Why is this not
possible?

Cheers,

Manuel.

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

[Manuel López-Ibáñez]

+   If SHOW_CARET_P is true, then the range should be rendered with
+   a caret at its starting location.  This
+   is for use by the Fortran frontend, for implementing the
+   "%C" and "%L" format codes.  */
+
+void
+rich_location::set_range (unsigned int idx, source_range src_range,
+  bool show_caret_p, bool overwrite_loc_p)

I do not understand when is this show_caret_p used by Fortran given
the diagnostic_show_locus code mentioned earlier.

Related to this:

inline void set_location (unsigned int idx, location_t loc, bool caret_p)

is always called with the last parameter 'true' (boolean parameters
are always almost bad API). Do you really need this parameter?

+/* Overwrite the range within this text_info's rich_location.
+   For use e.g. when implementing "+" in client format decoders.  */

If we got rid of '+' we would not need this extra work. Also '+'
breaks #pragma diagnostics. Not the fault of your patch, but it just
shows that technical debt keeps accumulating.
https://gcc.gnu.org/wiki/Partial_Transitions

Cheers,

Manuel.

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

[Manuel López-Ibáñez]

On 25 September 2015 at 23:24, David Malcolm <dmalc...@redhat.com> wrote:
> On Fri, 2015-09-25 at 23:13 +0200, Manuel López-Ibáñez wrote:
>> +   If SHOW_CARET_P is true, then the range should be rendered with
>> +   a caret at its starting location.  This
>> +   is for use by the Fortran frontend, for implementing the
>> +   "%C" and "%L" format codes.  */
>> +
>> +void
>> +rich_location::set_range (unsigned int idx, source_range src_range,
>> +  bool show_caret_p, bool overwrite_loc_p)
>>
>> I do not understand when is this show_caret_p used by Fortran given
>> the diagnostic_show_locus code mentioned earlier.

[...]
> rich_location::set_range exists to ensure that the %C and %L codes used
> by Fortran (and "+" in the C family of FEs) can write back into the
> rich_location instance, faithfully emulating the old code that wrote
> back to
> struct text_info's:
>   location_t locations[MAX_LOCATIONS_PER_MESSAGE];

Why Fortran cannot use text->set_location like the other FEs? This way
you do not need set_range at all. In fact, you do:

+source_range range
+  = source_range::from_location (
+  linemap_position_for_loc_and_offset (line_table,
+   loc->lb->location,
+   offset));
+text->set_range (loc_num, range, true);

But I guess this doesn't actually create a range like ^~~~ but as single ^.

The other issue that confuses me is that show_caret_p is always true
when reaching this function via the pretty-printer. Thus, show_caret_p
is also used by C/C++. In fact, I'm not sure when it can be false.

Cheers,

Manuel.

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

[Manuel López-Ibáñez]

On 25 September 2015 at 23:15, David Malcolm  wrote:
> My recollection is that I saw that the Fortran frontend has logic for
> calling into diagnostic_print_caret_line, noticed that the fortran
> testsuite has dg- assertions about finding specific messages, and I got
> worried that they embed assumptions about how the old printer worked.
> Hence I wanted to avoid touching that for the first version, and so in
> this patch it's a hybrid of the old Fortran printing code with the new
> representation for multiple locations.

It is quite simple, one you understand the logic. Fortran has three
types of output:

(a) # [name]:[locus]:
#
#some code
#  1
# Error: Some error at (1)

which can call the same function used by other FEs to print the caret
line (I call the caret line, the line that contains the caret
character/ranges, 1 in this case).

(b) # [name]:[locus]:
#
#   some code and some more code
#  1   2
# Error: Some error at (1) and (2)


which according to what you explained should also be possible by
calling diagnostic_show_locus with the appropriate location info and

(c) # [name]:[locus]:
#
#   some code
#  1
# [name]:[locus2]:
#
#   some other code
# 2
# Error: Some error at (1) and (2)
# or

which was implemented by calling diagnostic_show_locus with just the
location of 1, then calling diagnostic_print_caret_line with just the
expanded_location of 2. I could have just called diagnostic_show_locus
also to print 2 by overriding diagnostic->location[0] =
diagnostic->location[1] and caret_char[0] = caret_char[1], but that
seemed a bit hackish and more expensive (but perhaps less confusing?).

If you have a function that you can call with one or more
location_t/expanded_location  (or something that can be converted from
a location_t) and pass explicitly the caret_char, then you just need
to call that function with the right parameters to get the second part
of (c). Otherwise, you may simply temporarily do caret_char[0] =
caret_char[1], before calling the same function that prints the
caret-line for (a).

> Maybe that's a cop-out.  Would you prefer that the patch goes all the
> way, and that I attempt to eliminate all calls to
> diagnostic_print_caret_line from the Fortran FE, and eliminate the old
> implementation?  (either now, or as a followup patch?)  I may need
> assistance with that; I suspect that some of the dg- assertions in the
> Fortran test suite may need updating.

There is only one call! I just think this hack is really not necessary
(in fact, it seems more complicated than the alternatives outlined
above). And I'm afraid that once it goes in, it will stay there
forever. You are in a far better position than the Fortran devs to
understand how to call your new interfaces to get the output you
desire.

Cheers,

Manuel.

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

[Manuel López-Ibáñez]

On 25 September 2015 at 10:51, Dodji Seketeli  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! :)

Cheers,

Manuel.

Re: fdiagnostics-color=never does not disable color for some diagnostics

[Manuel López-Ibáñez]

On 24 September 2015 at 15:06, Jason Merrill <ja...@redhat.com> wrote:
> On 09/22/2015 04:23 PM, Manuel López-Ibáñez wrote:
>>
>> +error_at (loc, "-Werror=%s: no option -%s", arg, new_option);
>> +  else if (!(cl_options[option_index].flags & CL_WARNING))
>> +error_at (loc, "-Werror=%s: -%s is not an option that controls
>> warnings",
>
>
> Won't these incorrectly start with "-Werror=Wsomething:" rather than the
> "-Werror=something" that the user wrote?

They follow the pattern of the code they replace:

-{
-  error_at (loc, "-Werror=%s: no option -%s", arg, new_option);
-}

where 'arg' is what the user wrote after '=', and new_option is:

   new_option[0] = 'W';
   strcpy (new_option + 1, arg);

Or am I misunderstanding you?

Cheers,

Manuel.

[PATCH c-family/49654/49655] reject invalid options in pragma diagnostic

[Manuel López-Ibáñez]

Use find_opt instead of linear search through options in
handle_pragma_diagnostic (PR 49654) and reject non-warning options and
options not valid for the current language (PR 49655).

Boot on x86_64-linux-gnu.

OK?

gcc/testsuite/ChangeLog:

2015-09-22  Manuel López-Ibáñez  <m...@gcc.gnu.org>

PR c/49655
* gcc.dg/pragma-diag-6.c: New test.

gcc/ChangeLog:

2015-09-22  Manuel López-Ibáñez  <m...@gcc.gnu.org>

PR c/49655
* opts.h (write_langs): Declare.
* opts-global.c (write_langs): Make it extern.

gcc/c-family/ChangeLog:

2015-09-22  Manuel López-Ibáñez  <m...@gcc.gnu.org>

PR c/49654
PR c/49655
* c-pragma.c (handle_pragma_diagnostic): Detect non-warning
options and options not valid for the current language.
Index: gcc/c-family/c-pragma.c
===
--- gcc/c-family/c-pragma.c (revision 227965)
+++ gcc/c-family/c-pragma.c (working copy)
@@ -747,26 +747,44 @@ handle_pragma_diagnostic(cpp_reader *ARG
   warning_at (loc, OPT_Wpragmas,
  "missing option after %<#pragma GCC diagnostic%> kind");
   return;
 }
 
+  const char *option_string = TREE_STRING_POINTER (x);
+  /* option_string + 1 to skip the initial '-' */
+  unsigned int lang_mask = c_common_option_lang_mask () | CL_COMMON;
+  unsigned int option_index = find_opt (option_string + 1, lang_mask);
+  if (option_index == OPT_SPECIAL_unknown)
+{
+  warning_at (loc, OPT_Wpragmas,
+ "unknown option after %<#pragma GCC diagnostic%> kind");
+  return;
+}
+  else if (!(cl_options[option_index].flags & CL_WARNING))
+{
+  warning_at (loc, OPT_Wpragmas,
+ "%qs is not an option that controls warnings", option_string);
+  return;
+}
+  else if (!(cl_options[option_index].flags & lang_mask))
+{
+  char * ok_langs = write_langs (cl_options[option_index].flags);
+  char * bad_lang = write_langs (c_common_option_lang_mask ());
+  warning_at (loc, OPT_Wpragmas,
+ "option %qs is valid for %s but not for %s",
+ option_string, ok_langs, bad_lang);
+  free (ok_langs);
+  free (bad_lang);
+  return;
+}
+
   struct cl_option_handlers handlers;
   set_default_handlers ();
-
-  unsigned int option_index;
-  const char *option_string = TREE_STRING_POINTER (x);
-  for (option_index = 0; option_index < cl_options_count; option_index++)
-if (strcmp (cl_options[option_index].opt_text, option_string) == 0)
-  {
-   control_warning_option (option_index, (int) kind, kind != DK_IGNORED,
-   input_location, c_family_lang_mask, ,
-   _options, _options_set,
-   global_dc);
-   return;
-  }
-  warning_at (loc, OPT_Wpragmas,
- "unknown option after %<#pragma GCC diagnostic%> kind");
+  control_warning_option (option_index, (int) kind, kind != DK_IGNORED,
+ loc, lang_mask, ,
+ _options, _options_set,
+ global_dc);
 }
 
 /*  Parse #pragma GCC target (xxx) to set target specific options.  */
 static void
 handle_pragma_target(cpp_reader *ARG_UNUSED(dummy))
Index: gcc/testsuite/gcc.dg/pragma-diag-6.c
===
--- gcc/testsuite/gcc.dg/pragma-diag-6.c(revision 0)
+++ gcc/testsuite/gcc.dg/pragma-diag-6.c(revision 0)
@@ -0,0 +1,5 @@
+/* { dg-do compile } */
+#pragma GCC diagnostic error "-Wnoexcept" /* { dg-warning "is valid for 
C../ObjC.. but not for C" } */
+#pragma GCC diagnostic error "-fstrict-aliasing" /* { dg-warning "not an 
option that controls warnings" } */
+#pragma GCC diagnostic error "-Werror" /* { dg-warning "not an option that 
controls warnings" } */
+int i;
Index: gcc/opts.h
===
--- gcc/opts.h  (revision 227965)
+++ gcc/opts.h  (working copy)
@@ -366,10 +366,11 @@ extern void control_warning_option (unsi
unsigned int lang_mask,
const struct cl_option_handlers *handlers,
struct gcc_options *opts,
struct gcc_options *opts_set,
diagnostic_context *dc);
+extern char *write_langs (unsigned int mask);
 extern void print_ignored_options (void);
 extern void handle_common_deferred_options (void);
 extern bool common_handle_option (struct gcc_options *opts,
  struct gcc_options *opts_set,
  const struct cl_decoded_option *decoded,
Index: gcc/opts-global.c
==

fdiagnostics-color=never does not disable color for some diagnostics

[Manuel López-Ibáñez]

Actually, I was trying to reject non-warning options as argument to
-Werror=. However, the new test fails because -fdiagnostics-color=never is
always placed by the driver after the warning options when calling the compiler
proper. This patch prunes all -fdiagnostics-color from the command-line but the
last one, which is moved to the first position.

Boot on x86_64-linux-gnu

OK?

gcc/ChangeLog:

2015-09-22  Manuel López-Ibáñez  <m...@gcc.gnu.org>

PR driver/67640
* opts-common.c (prune_options): Discard all -fdiagnostics-color
but the last one, which is moved to the front to be processed
first.
* opts.c (enable_warning_as_error): Reject options that do not
control warnings.


gcc/testsuite/ChangeLog:

2015-09-22  Manuel López-Ibáñez  <m...@gcc.gnu.org>

PR driver/67640
* gcc.dg/Werror-13.c: New test.
Index: gcc/opts-common.c
===
--- gcc/opts-common.c   (revision 228011)
+++ gcc/opts-common.c   (working copy)
@@ -823,10 +823,11 @@ prune_options (struct cl_decoded_option 
   unsigned int new_decoded_options_count;
   struct cl_decoded_option *new_decoded_options
 = XNEWVEC (struct cl_decoded_option, old_decoded_options_count);
   unsigned int i;
   const struct cl_option *option;
+  unsigned int fdiagnostics_color_idx = 0;
 
   /* Remove arguments which are negated by others after them.  */
   new_decoded_options_count = 0;
   for (i = 0; i < old_decoded_options_count; i++)
 {
@@ -842,10 +843,15 @@ prune_options (struct cl_decoded_option 
case OPT_SPECIAL_ignore:
case OPT_SPECIAL_program_name:
case OPT_SPECIAL_input_file:
  goto keep;
 
+   /* Do not save OPT_fdiagnostics_color_, just remember the last one.  */
+   case OPT_fdiagnostics_color_:
+ fdiagnostics_color_idx = i;
+ continue;
+
default:
  gcc_assert (opt_idx < cl_options_count);
  option = _options[opt_idx];
  if (option->neg_index < 0)
goto keep;
@@ -877,10 +883,21 @@ keep:
}
  break;
}
 }
 
+  if (fdiagnostics_color_idx > 1)
+{
+  /* We put the last -fdiagnostics-color= at the first position
+after argv[0] so it can take effect immediately.  */
+  memmove (new_decoded_options + 2, new_decoded_options + 1,
+  sizeof (struct cl_decoded_option) 
+  * (new_decoded_options_count - 1));
+  new_decoded_options[1] = old_decoded_options[fdiagnostics_color_idx];
+  new_decoded_options_count++;
+}
+
   free (old_decoded_options);
   new_decoded_options = XRESIZEVEC (struct cl_decoded_option,
new_decoded_options,
new_decoded_options_count);
   *decoded_options = new_decoded_options;
Index: gcc/testsuite/gcc.dg/Werror-13.c
===
--- gcc/testsuite/gcc.dg/Werror-13.c(revision 0)
+++ gcc/testsuite/gcc.dg/Werror-13.c(revision 0)
@@ -0,0 +1,8 @@
+/* { dg-do compile } */
+/* { dg-options "-Werror=error -Werror=p, -Werror=l, -Werror=fatal-errors" } */
+/* { dg-error "-Wp, is not an option that controls warnings" "" { target *-*-* 
} 0 } */
+/* { dg-error "-Wl, is not an option that controls warnings" "" { target *-*-* 
} 0 } */
+/* { dg-error "-Werror is not an option that controls warnings" "" { target 
*-*-* } 0 } */
+/* { dg-error "-Wfatal-errors is not an option that controls warnings" "" { 
target *-*-* } 0 } */
+
+int i;
Index: gcc/opts.c
===
--- gcc/opts.c  (revision 228011)
+++ gcc/opts.c  (working copy)
@@ -2357,13 +2357,14 @@ enable_warning_as_error (const char *arg
   new_option = XNEWVEC (char, strlen (arg) + 2);
   new_option[0] = 'W';
   strcpy (new_option + 1, arg);
   option_index = find_opt (new_option, lang_mask);
   if (option_index == OPT_SPECIAL_unknown)
-{
-  error_at (loc, "-Werror=%s: no option -%s", arg, new_option);
-}
+error_at (loc, "-Werror=%s: no option -%s", arg, new_option);
+  else if (!(cl_options[option_index].flags & CL_WARNING))
+error_at (loc, "-Werror=%s: -%s is not an option that controls warnings",
+ arg, new_option);
   else
 {
   const diagnostic_t kind = value ? DK_ERROR : DK_WARNING;
 
   control_warning_option (option_index, (int) kind, value,

Re: [PATCH tree-inline] do not say "called from here" with UNKNOWN_LOCATION

[Manuel López-Ibáñez]

On 21 September 2015 at 12:29, Richard Biener
<richard.guent...@gmail.com> wrote:
> On Mon, Sep 21, 2015 at 11:59 AM, Manuel López-Ibáñez
> <lopeziba...@gmail.com> wrote:
>> On 21 September 2015 at 10:18, Richard Biener
>> <richard.guent...@gmail.com> wrote:
>>> input_location is set from the call stmt:
>>>
>>>   /* FIXME: instantiate_decl isn't called by inlinable_function_p.  */
>>>   saved_location = input_location;
>>>   input_location = gimple_location (stmt);
>>>
>>> it would be nice to get rid of that.
>>
>> I could replace all uses of input_location in this function by
>> gimple_location(stmt) as I noted in the comments. Would that be ok if
>> it works? I'm not sure I can prove that input_location is not used
>> behind the scenes for some other purpose (all the more reason to kill
>> input_location once and for all). Friends, don't let friends use
>> input_location in new code!
>
> Yeah...  not sure how to check but to look for any changes in
> generated cc1/cc1plus
> debug info.  You could also try making it invalid (-1?) and hope
> libcpp would eventually
> blow up if that is used.
>

It does blow up with:

/home/manuel/test2/227965M/build/gcc/gnat1 -gnatwa -quiet -nostdinc
-dumpbase s-mudido.adb -auxbase-strip s-mudido.o -O2 -Wextra -Wall
-fpic -g -gnatpg -mtune=generic -march=x86-64 -gnatO s-mudido.o
s-mudido.adb -o /tmp/ccNQpzNF.s

at:

B =>SET_EXPR_LOCATION (mod, EXPR_LOC_OR_LOC (val, input_location));

#0  internal_get_tmp_var (val=0x75dfdc40, pre_p=0x7fffdaf0,
post_p=, is_formal=) at
/home/manuel/test2/src/gcc/gimplify.c:540
#1  0x00c00efd in gimplify_expr
(expr_p=expr_p@entry=0x7fffdaf8, pre_p=pre_p@entry=0x7fffdaf0,
post_p=0x7fffd9a0, post_p@entry=0x0, gimple_test_f=, fallback=fallback@entry=1) at
/home/manuel/test2/src/gcc/gimplify.c:9040
#2  0x00c19b67 in gimple_regimplify_operands
(stmt=0x76074be0, gsi_p=gsi_p@entry=0x7fffdbb0) at
/home/manuel/test2/src/gcc/gimplify-me.c:252
#3  0x00e8cbe3 in copy_bb (id=id@entry=0x7fffde40,
bb=bb@entry=0x760eb548,
frequency_scale=frequency_scale@entry=1,
count_scale=count_scale@entry=1) at
/home/manuel/test2/src/gcc/tree-inline.c:1798
#4  0x00e8e039 in copy_cfg_body (new_entry=0x0,
exit_block_map=0x75dff340, entry_block_map=0x760d4c30,
frequency_scale=1, count=, id=0x7fffde40) at
/home/manuel/test2/src/gcc/tree-inline.c:2716
#5  copy_body (id=0x7fffde40, count=,
frequency_scale=1, entry_block_map=0x760d4c30,
exit_block_map=0x75dff340, new_entry=0x0) at
/home/manuel/test2/src/gcc/tree-inline.c:2955
#6  0x00e94f71 in expand_call_inline (id=0x7fffde40,
stmt=, bb=) at
/home/manuel/test2/src/gcc/tree-inline.c:4693
#7  gimple_expand_calls_inline (id=0x7fffde40, bb=)
at /home/manuel/test2/src/gcc/tree-inline.c:4833
#8  optimize_inline_calls (fn=) at
/home/manuel/test2/src/gcc/tree-inline.c:4973
#9  0x014c503c in inline_transform (node=0x7644ccf0) at
/home/manuel/test2/src/gcc/ipa-inline-transform.c:545
#10 0x00d54bac in execute_one_ipa_transform_pass
(ipa_pass=0x2656340, node=0x7644ccf0) at
/home/manuel/test2/src/gcc/passes.c:2197
#11 execute_all_ipa_transforms () at /home/manuel/test2/src/gcc/passes.c:2238
#12 0x00a99bc8 in cgraph_node::expand
(this=this@entry=0x7644ccf0) at
/home/manuel/test2/src/gcc/cgraphunit.c:1976
#13 0x00a9b44e in expand_all_functions () at
/home/manuel/test2/src/gcc/cgraphunit.c:2119
#14 symbol_table::compile (this=this@entry=0x7642b0a8) at
/home/manuel/test2/src/gcc/cgraphunit.c:2472
#15 0x00a9da63 in symbol_table::compile (this=0x7642b0a8)
at /home/manuel/test2/src/gcc/cgraphunit.c:2536
#16 symbol_table::finalize_compilation_unit (this=0x7642b0a8) at
/home/manuel/test2/src/gcc/cgraphunit.c:2562
#17 0x00e17d90 in compile_file () at
/home/manuel/test2/src/gcc/toplev.c:508
#18 0x0069e8a4 in do_compile () at
/home/manuel/test2/src/gcc/toplev.c:1973
#19 toplev::main (this=this@entry=0x7fffe0a0, argc=argc@entry=21,
argv=argv@entry=0x7fffe198) at
/home/manuel/test2/src/gcc/toplev.c:2080
#20 0x006a0bd7 in main (argc=21, argv=0x7fffe198) at
/home/manuel/test2/src/gcc/main.c:39

For some extra reason val does not have a location:

(gdb) p debug_tree(val)
 
sizes-gimplified asm_written nonaliased-component BLK size
 unit size 
align 8 symtab -166627696 alias set 32 canonical type
0x75fca2a0 domain  context

pointer_to_this 
reference_to_this  chain >
asm_written public unsigned DI
size 
unit size 
align 64 symtab -166626736 alias set 41 canonical type 0x75fca540>

arg 0 
nothrow
arg 0 
visited var def_stmt
R.94_159 = .builtin_alloca_with_align

Re: [PATCH tree-inline] do not say "called from here" with UNKNOWN_LOCATION

[Manuel López-Ibáñez]

On 21 September 2015 at 12:29, Richard Biener
 wrote:
>>> least note the function we are failing to inline to (thus, use
>>> DECL_SOURCE_LOCATION
>>> of cfun->decl).  So better add a diag_location and compute that upfront to 
>>> avoid
>>> repeating the check.
>>
>>error ("inlining failed in call to always_inline %q+F: %s", fn,
>>   cgraph_inline_failed_string (reason));
>>
>> The call is using '+F', thus the location is set to some location
>> related to F, depending on which *_printer function is active at that
>> moment. cp_printer uses location_of, and default_tree_printer uses
>> DECL_SOURCE_LOCATION. Is the default_tree_printer always used at this
>> point? If yes, I completely agree we should use an explicit
>> DECL_SOURCE_LOCATION. The meaning of '+' is not only opaque but it
>> breaks #pragma GCC diagnostic.
>
> But it prints the location of the function we failed to inline.  I
> want to retain
> at least an approximation to the location of the call, which is the location
> of the function we inline _to_.

I think I misunderstood you. Do you mean something like?

if (gimple_location (stmt) != UNKNOWN_LOCATION)
inform (gimple_location (stmt), "called from here");
else
inform (DECL_SOURCE_LOCATION (cfun->decl), "called from this function");


Cheers,

Manuel.