Re: [PATCH] panic: suppress gnu_printf warning

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

On 07/01/2024 19:21, Andrew Morton wrote:

On Sun,  7 Jan 2024 17:16:41 +0800 Baoquan He  wrote:


with GCC 13.2.1 and W=1, there's compiling warning like this:

kernel/panic.c: In function ?__warn?:
kernel/panic.c:676:17: warning: function ?__warn? might be a candidate for 
?gnu_printf? format attribute [-Wsuggest-attribute=format]
   676 | vprintk(args->fmt, args->args);
   | ^~~

The normal __printf(x,y) adding can't fix it. So add workaround which
disables -Wsuggest-attribute=format to mute it.

...

--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -666,8 +666,13 @@ void __warn(const char *file, int line, void *caller, 
unsigned taint,
pr_warn("WARNING: CPU: %d PID: %d at %pS\n",
raw_smp_processor_id(), current->pid, caller);
  
+#pragma GCC diagnostic push

+#ifndef __clang__
+#pragma GCC diagnostic ignored "-Wsuggest-attribute=format"
+#endif
if (args)
vprintk(args->fmt, args->args);
+#pragma GCC diagnostic pop
  
  	print_modules();

__warn() clearly isn't such a candidate.  I'm suspecting that gcc's
implementation of this warning is pretty crude.  Is it a new thing in
gcc-13.2?


I suspect the warning is about vprintk(), which does seem a printf-like 
function but something (early inlining?) may be messing up the context and GCC 
warns about __warn(). This may be bug: 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=28492


If vprintk() already has the format attribute, then the messed up function name 
may be confusing GCC into warning again about it.


Best wishes,

Manuel.

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: On(c)e more: optimizer failure

[Manuel López-Ibáñez]

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

Cheers,

Manuel.

Re: rust frontend and UTF-8/unicode processing/properties

[Manuel López-Ibáñez]

For the gcc rust frontend I was thinking of importing a couple of
gnulib modules to help with UTF-8 processing, conversion to/from
unicode codepoints and determining various properties of those
codepoints. But it seems gcc doesn't yet have any gnulib modules
imported, and maybe other frontends already have helpers to this that
the gcc rust frontend could reuse.


Although I agree that factoring out the code in libcpp so that it can be used 
by other FEs would be great and in line with the goals of 
https://gcc.gnu.org/wiki/ModularGCC that is a significant amount of work. 
Importing gnulib has its own advantages and it would allow GCC to finally 
deprecate libiberty:


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

There is a preliminary patch here: 
https://gcc.gnu.org/legacy-ml/gcc-patches/2016-08/msg01554.html


Cheers,

Manuel.

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: Feature request: -Wno-unknown-warnings to silently ignore unknown warning control flags.

[Manuel López-Ibáñez]

On 14/10/17 16:32, Oren Ben-Kiki wrote:

Thanks for the pointers. I'm not currently using auto tools, but I might
end up having to use them, or cmake. Having these macros would help. I
still wish we had `-Wno-unknown-warnings` though - it would make life much
simpler.



Despite the feedback that you have received so far, I think that if you 
actually submitted a proper patch, it will be accepted.


One possible implementation could behave as follows:

1. By default, keep current behavior.
2. With -Wno-error=unknown-option, warn instead of error.
3. With -Wno-unknown-option, do not diagnose unknown options (use at your 
peril!).


--- opts-common.c   (revision 253833)
+++ opts-common.c   (working copy)
@@ -1230,7 +1230,8 @@
   if (decoded->opt_index == OPT_SPECIAL_unknown)
 {
   if (handlers->unknown_option_callback (decoded))
-   error_at (loc, "unrecognized command line option %qs", decoded->arg);
+   warning_at (loc, OPT_Wunknown_option,
+   "unrecognized command line option %qs", decoded->arg);
   return;
 }

--- opts-global.c   (revision 251201)
+++ opts-global.c   (working copy)
@@ -134,7 +134,7 @@
   const char *opt;

   opt = ignored_options.pop ();
-  warning_at (UNKNOWN_LOCATION, 0,
+  warning_at (UNKNOWN_LOCATION, OPT_Wunknown_option,
  "unrecognized command line option %qs", opt);
 }
 }
--- toplev.c(revision 253834)
+++ toplev.c(working copy)
@@ -1650,14 +1650,23 @@
   || !targetm.have_prologue () || !targetm.have_epilogue ())
 flag_ipa_ra = 0;

-  /* Enable -Werror=coverage-mismatch when -Werror and -Wno-error
- have not been set.  */
-  if (!global_options_set.x_warnings_are_errors
-  && warn_coverage_mismatch
-  && (global_dc->classify_diagnostic[OPT_Wcoverage_mismatch] ==
-  DK_UNSPECIFIED))
-diagnostic_classify_diagnostic (global_dc, OPT_Wcoverage_mismatch,
-DK_ERROR, UNKNOWN_LOCATION);
+  if (!global_options_set.x_warnings_are_errors)
+{
+  /* Enable -Werror=coverage-mismatch when -Werror and -Wno-error
+have not been set.  */
+  if (warn_coverage_mismatch
+ && (global_dc->classify_diagnostic[OPT_Wcoverage_mismatch] ==
+ DK_UNSPECIFIED))
+   diagnostic_classify_diagnostic (global_dc, OPT_Wcoverage_mismatch,
+   DK_ERROR, UNKNOWN_LOCATION);
+  /* Enable -Werror=unknown-option when -Werror and -Wno-error
+have not been set.  */
+  if (warn_unknown_option
+ && (global_dc->classify_diagnostic[OPT_Wunknown_option] ==
+ DK_UNSPECIFIED))
+   diagnostic_classify_diagnostic (global_dc, OPT_Wunknown_option,
+   DK_ERROR, UNKNOWN_LOCATION);
+}

   /* Save the current optimization options.  */
   optimization_default_node = build_optimization_node (_options);
--- common.opt  (revision 251201)
+++ common.opt  (working copy)
@@ -634,6 +634,10 @@
 Common Var(warn_null_dereference) Warning
 Warn if dereferencing a NULL pointer may lead to erroneous or undefined 
behavior.

+Wunknown-option
+Common Var(warn_unknown_option) Init(1) Warning
+Warn for unknown command-line options. This is an error by default.
+
 Wunsafe-loop-optimizations
 Common Var(warn_unsafe_loop_optimizations) Warning
 Warn if the loop cannot be optimized due to nontrivial assumptions.



The above is not a proper patch, just a draft of an idea. Now you only need to 
follow the instructions here: 
https://gcc.gnu.org/wiki/GettingStarted#Basics:_Contributing_to_GCC_in_10_easy_steps


Cheers,

Manuel.

Re: Feature request: -Wno-unknown-warnings to silently ignore unknown warning control flags.

[Manuel López-Ibáñez]

On 13/10/17 02:47, Martin Sebor wrote:

[*] We wrote a script scrape those off the online HTML manual
and create a "database" mapping options to GCC versions they
were introduced in (or first documented in, as not every option
always gets documented as it gets added).


I don't understand why you would need to scrape the HTML manual. Grepping the 
output of 'gcc --help=' should quickly tell you if an option is supported.


Unfortunately, it doesn't tell which options imply other options or which 
options take arguments, their types and ranges. However, this is something that 
could be improved in gcc --help= easily if somebody actually cared. And it 
would be far more robust and easier than creating a database (which may need to 
not only encode GCC versions but also handle target-specific options).


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.

Re: Exhaustive Instructions for Toolchain Generation

[Manuel López-Ibáñez]

On 4 Oct 2017 8:01 pm, "Nathan Sidwell" <nat...@acm.org> wrote:

On 10/04/2017 02:10 PM, Manuel López-Ibáñez wrote:

Incidentally, I don't understand why there is no "Professional Support"
> page where we can direct people to find professional support. It could
>

My recollection is that the FSF explicitly prohibit this


I'm thinking about something like: http://www.fsf.org/resources/service
but GCC specific.

Re: Exhaustive Instructions for Toolchain Generation

[Manuel López-Ibáñez]

On 04/10/17 00:22, Sandra Loosemore wrote:

On 10/03/2017 03:27 PM, R0b0t1 wrote:

On Sun, Oct 1, 2017 at 4:35 PM, Sandra Loosemore
> wrote:


[snip]

FAOD, R0b0t1 forwarded mail I deliberately sent off-list back to the list.  I 
do know that business solicitations are frowned upon on the mailing lists.  :-(


-Sandra



For what is worth, I think you did nothing wrong and this was poor 
net-etiquette from Robot1.


Incidentally, I don't understand why there is no "Professional Support" page 
where we can direct people to find professional support. It could have a big 
red and blinking disclaimer on top saying something like: "The GCC project does 
not endorse any of these companies/individuals and cannot be held responsible 
for their behavior. This page is purely informational and may be inaccurate. 
Please, research and compare vendors by looking at whether they are actually 
MAINTAINERS of any part of GCC and regular contributors (link to svn/git 
history). The GCC project advocates free software, thus we encourage you to 
release as free software, and contribute back to GCC when appropriate, any code 
resulting from professional work."


I'm very tempted to add an entry to the FAQ in the wiki, since this question 
has been raised several times in the past, but the people from Codesourcery, 
Linaro, etc surely know better what they would like to say and where to direct 
readers.


Cheers,
Manuel.

Re: Possible Bug Fix/No Reply on Bugzilla

[Manuel López-Ibáñez]

On 27/09/17 21:56, nick wrote:

Greetings All,

I commented here a few names ago, 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82230. Not
to be a annoyance but I have a school assignment and would like someone to 
reply if it's
correct or something. I am assuming it's probably wrong but any comment would 
be very
helpful before I sent in a patch to the patches list to get merged for the fix.

Take care,
Nick


Dear Nick,

Long time ago, I was in your position as a newbie volunteer contributor. Please 
let me share some suggestions that I hope would help you to direct your efforts 
in the most useful and least frustrating way possible:


* Nobody can reply to all comments in bugzilla, there are just too many.

* People contributing to GCC are already very busy with their own projects, 
bugs, priorities. Unfortunately, there is very little time left for mentoring, 
in particular of sporadic contributors: You are more likely to get a reply if 
you are a frequent contributor. You have to be very self-motivated, because the 
start will be slow (I have more tips to share about how to overcome this phase 
if you are interested).


* People may not know the answer to what you are asking or it may require 
investing a significant amount of time to find out the answer, thus they reply 
nothing.


* People are not likely to spend time testing a fix that is "probably wrong". 
They would hope that you make the effort to test it yourself.


How to move your fix forward? My suggestion would be: Check that it works. 
Check out trunk, apply your fix, create a testcase, run the testsuite and check 
that it fixes the issue and it doesn't break anything else.


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


I'd also suggest to read: https://gcc.gnu.org/wiki/Community

Happy hacking!

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: assuming signed overflow does not occur

[Manuel López-Ibáñez]

On 03/09/17 23:00, Bruce Korb wrote:


RFE's are for this list: please improve the message.

The message does not have to be a dissertation, but messages
nowadays can certainly include URL's to direct people to
reasonable places. I'd suggest something like:

 gcc.gnu.org/gcc-messages/xxx

WRT looking at a GIMPLE dump, I'd first have to learn what GIMPLE is,
then learn how to decipher one, then figure out how to get it out of
the compiler and
finally plod through it. Guess what? I won't be doing that. I'll
squish the warning first. :(  Effective messages are very important.


I wrote an explanation of the current status of Wstrict-overflow to the best of 
my knowledge: https://gcc.gnu.org/wiki/VerboseDiagnostics#Wstrict_overflow


I didn't mention GIMPLE because it is often the case that the root of the 
problem is not obvious in gimple unless one also debugs the compiler at the 
same time.


I hope it is useful!

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: terminology: zero character vs. null character

[Manuel López-Ibáñez]

On 06/03/17 21:15, Roland Illig wrote:

Hi,

I am currently translating GCC into German. During that, I noticed that
in some places the term "zero character" means '\0'. The official term
though is "null character", as per the C standard.

Since it is confusing to have two different terms for the same concept,
the term "zero character" should be dropped entirely, both because it is
uncommon and because it can be confused with '0'.

Since this affects several places in the code, I think it's better to
start a small discussion first instead of writing several PRs.


I don't see anything explicit here: https://gcc.gnu.org/codingconventions.html 
But I believe we follow standards' language and it should always be "null 
character". Having a discussion first is likely to get nowhere. It sounds too 
much like "maybe we should" (point 11: https://gcc.gnu.org/wiki/Community).


Please send a patch fixing it:

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

You can also send a patch changing https://gcc.gnu.org/codingconventions.html. 
Once that is accepted, obvious changes to match the coding conventions are 
usually considered pre-approved.


Cheers,

Manuel.

Re: diagnostics: %<%s%> vs. %qs

[Manuel López-Ibáñez]

On 07/03/17 20:38, Roland Illig wrote:

Hi,

in the diagnostics the %qs specifier is used in most of the cases. But
there are some cases left where the more complicated %<%s%> is used. Is
there a good reason to prefer the complicated spelling?

Same for %<%T%> and %qT, and similar letters.


'q' is a flag supported by some format codes but not all. Also, although 
different parts of the compiler may support the same format codes (%T), not all 
may have support for 'q' yet. Finally, there may be some code using %<%T%> that 
predates the existence of '%qT'.


Ideally, we should move all the code to use 'q' and leave %<%> for specific 
cases like quoting text: "%<--help%>"


Any help on this is appreciated: 
https://gcc.gnu.org/wiki/GettingStarted#Basics:_Contributing_to_GCC_in_10_easy_steps


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: GCC Commit Stats [was: [GCC Steering Committee attention] [PING] [PING] [PING] libgomp [...]]

[Manuel López-Ibáñez]

On 5 August 2016 at 18:34, James Greenhalgh  wrote:

> I've given the 2012-2015 numbers below, just to show that (for the files
> in gcc/*.[ch]) your hypothesis doesn't hold. The vast majority of
> committers make <20 commits in a year.

My hypothesis is that fewer people are increasingly doing most of the
work. If the vast majority of committers do very few commits, that
doesn't disprove (neither necessarily prove) my hypothesis. Either
very few commiters do most of the commits, which is consistent with my
hypothesis, or most of the commits are done by a very large number of
committers who individually do very few commits, which contradicts my
hypothesis. Either fact is consistent with your numbers.

However, your numbers do not seem to indicate any sudden changes in
trends, which argues against my hypothesis of "increasingly". Yet, I
would argue that the trend not changing despite the continuous
increase in code is itself worrying. But yes, the "increasingly" is
definitely the weakest part of my hypothesis.

>> * 100 commits is less than 2%. Quite a low threshold. Perhaps 1%, 25%,
>> 50%, 75%, 90% are more informative.
>
> Again, just done for time. I've changed the last two buckets to 100-199
> and 200+ in this run. If you'd like to do, I'd be happy to see the
> results.

200 is around 10% of commits. Thus, 2 people do at least 20% of commits.

>> that is, most of the commits are done by smaller fraction of the
>> total.
>
> For 2015 I found the 4 "25%" marks to be:
>
>   26%1-4
>   25%5-13
>   25%14-39
>   23%40+

> So 75% of the work is being done by people who commit fewer than 40
> patches in a year. Encouragingly 50% of the people who committed in
> 2015 committed at least one patch per month (on average).

Sorry, what is each column? If 26% of people commit between 1-4, this
does not mean that 26% of the work is done by people who commit
between 1-4 patches.

> Personally, I think that looks like a fairly stable and healthy community,
> but you're welcome to draw your own conclusions from the data.

It looks more stable than what I would have expected. I'm not totally
convinced about the thresholds, though. I believe the best way to
measure is more similar to how openhub does: Sort committers by number
of commits, then calculate their respective percentage w.r.t. to the
total, and summarize at given thresholds the cumulative percentage,
then print "%total commits" "%commiters". Do the same for every year.
Exclude Ada, Fortran, and Go if possible.

It would be interesting if someone could generate those numbers. If
you get me the raw numbers of commits per committer per year, I can
easily generate the stats and even nice plots if you wish.

I'd be happy to see my hypothesis disproved.

Cheers,

Manuel.

Re: [GCC Steering Committee attention] [PING] [PING] [PING] libgomp: In OpenACC testing, cycle though $offload_targets, and by default only build for the offload target that we're actually going to te

[Manuel López-Ibáñez]

On 5 August 2016 at 15:06, James Greenhalgh <james.greenha...@arm.com> wrote:
> On Thu, Aug 04, 2016 at 09:12:36PM +0100, Manuel López-Ibáñez wrote:
>> This is a problem throughout GCC. We have a single C++ maintainer, a
>> single part-time C maintainer, none? for libiberty, no regular
>> maintainer for build machinery, and so on and so forth.
>
> I'd object to this being "throughout" GCC. There are a number of very good,
> very responsive, very helpful GCC reviewers in our community. However, as
> you'd expect, the most active areas for contribution are the most active
> areas for review.

Both statements are not conflicting. It may be true at the same time
that reviewer X is very good, very responsive, very helpful, skilled,
and experienced,  and also true that patches for the areas maintained
by reviewer X may still not be reviewed timely. And, in case it needs
repeating, the solution is not that X stops reviewing or maintaining.

>> This has been a problem for a very long time, but it seems to be
>> getting worse now that fewer contributors are doing most of the
>> work.
>
> Rumours of GCC's death have been greatly exagerated!
>
> At times it might feel this way, but the data
> (BlackDuck at www.openhub.net and my own analysis on the Git Mirror)
> suggests that actually the GCC community is near enough as strong as it
> has been for most of the past 20 years, both in terms of volume of
> contributions, number of contributors, and average number of contributions
> per contributor. Some rudimentary stats gathering on the git mirror suggests
> no substantial change in the makeup of the community over the years. If
> anything there are more commits from more people and the average number of
> commits per committer is decreasing.

I think those conclusions are debatable:

* GCC has also grown over the years, there is a lot more code and
areas, specially more targets, which attract their own temporary
developers who do not contribute to the rest of the compiler (much
less review patches for the rest of the compiler).

* Your analysis includes Ada, Go and Fortran. I'd suggest to exclude
them, since in terms of developers and reviewing, they seem to be
doing fine. They also tend to organise themselves mostly independently
of the rest of the compiler. This is also mostly true for targets.

* 100 commits is less than 2%. Quite a low threshold. Perhaps 1%, 25%,
50%, 75%, 90% are more informative.

* https://www.openhub.net/p/taezaza/contributors/summary shows that
more than 25% of the commits in the last 12 months were made by 6
people. Note that those people are also the most active reviewers.

* If I adjust the numbers by the total number of contributors, then we
get a different picture:

   X1998   X2001   X2004   X2007
X2010   X2013   X2015   X2016
0-19 commits   36.363636   47.413793   46.405229   54.491018
60.233918   62.50   60.526316   66.447368
20-39 commits  18.181818   16.379310   16.993464   15.568862
16.374269   15.909091   16.842105   14.473684
40-59 commits   9.0909097.7586216.535948   11.377246
7.6023399.0909096.3157898.552632
60-79 commits   6.8181826.0344836.5359485.389222
1.7543862.2727272.6315791.973684
80-99 commits   4.5454555.1724146.5359482.994012
2.3391812.2727272.1052633.289474
100+ commits   25.00   18.103448   17.647059   10.778443
12.2807028.522727   12.1052635.921053

that is, most of the commits are done by smaller fraction of the
total. If that smaller percentage is also responsible for doing most
of the review work for the rest...

* Numbers for other years might shed more light. 2010, 2013 and 2015
might have been especial in one sense or another.

>> >Global Reviewers are welcome to review OpenACC/OpenMP/offloading patches.
>> >But that doesn't help if that's then not happening in reality.  (With the
>> >exception of Bernd, who then did review such patches for a while, but
>> >also seems to have stopped with that again.)
>>
>> At least half of the global reviewers in MAINTAINERS never review
>> any patches. Most of them are not active any more and presumably do
>> not read GCC emails.
>
> That's just false.

Sorry, this was poorly phrased. I meant most of the half that do not
do global reviews. Still, I should have phrased that better by saying
that it was my biased perception. I tend to only pay attention to FE
and diagnostic patches and...

> I've seen all of these active on the lists during 2016 to a lesser or
> greater exentent.

Actively reviewing patches outside their maintainership area? For
example, reviewing OpenACC/OpenMP/offloading patches? Again, this is
not a criticism of people. My point is that we cannot extrapolate that
there a

Re: [GCC Steering Committee attention] [PING] [PING] [PING] libgomp: In OpenACC testing, cycle though $offload_targets, and by default only build for the offload target that we're actually going to te

[Manuel López-Ibáñez]

On 5 August 2016 at 12:16, Janne Blomqvist  wrote:
> - a "2-week rule"; if a patch by a reviewer goes unreviewed for 2
> weeks, the reviewer can commit it without review. A bit like your
> option a).
>
>
> The 2-week rule, in particular, came about due to frustration with
> lack of reviews.

Two weeks perhaps is too short, given the lack of reviewers and
understanding that they may be offline, on holidays or some other
issue. But 4 pings without any kind of response would be reasonable,
IMHO. And revert swiftly in case of problems.

Cheers,

Manuel.

Re: [GCC Steering Committee attention] [PING] [PING] [PING] libgomp: In OpenACC testing, cycle though $offload_targets, and by default only build for the offload target that we're actually going to te

[Manuel López-Ibáñez]

On 4 August 2016 at 22:01, DJ Delorie  wrote:
>
> Manuel Lpez-Ibñez  writes:
>> I don't see how that helps. Neither my message nor Thomas's is a
>> criticism of people. The question is how to get more people to help
>> and how to improve the situation. For sure, everybody is doing the
>> best that they can with the time that they have.
>
> You complained that there were no libiberty maintainers (there are two)
> or build maintainers (there are many).  As I am listed as one of each of
> those, this makes me wonder if there's no longer a need for such people
> (we're involved so infrequently that nobody notices) or that I'm just
> not able to put enough effort into it to be noticed (which may be true
> anyway).

My perception is that the "official" libiberty and build maintainers
rarely review patches anymore. See:
https://gcc.gnu.org/ml/gcc-patches/2016-07/msg01973.html and the lack
of follow-up.
Jeff Law seems to be doing most of the reviewing work nowadays in both
areas (and many others).

This is not a criticism of them. They are not obliged to review
anything and much less anything in particular. I'm sure there are many
reasons for their lack of involvement. I have basically not
contributed any code in almost a year. Just wanted to point out that
Thomas's email is not an isolated outlier.

The answer is not "let's delete them/ask them to step down". Even if
someone reviews one patch once in a decade, there is no point in
preventing them from doing so by removing them from MAINTAINERS. Maybe
they will become more active in the future. From my point of view,
they may stay in MAINTAINERS as long as they wish and any little help
is welcome.

Cheers,

Manuel.

Re: [GCC Steering Committee attention] [PING] [PING] [PING] libgomp: In OpenACC testing, cycle though $offload_targets, and by default only build for the offload target that we're actually going to te

[Manuel López-Ibáñez]

On 4 August 2016 at 21:34, Manuel López-Ibáñez <lopeziba...@gmail.com> wrote:
> On 4 August 2016 at 21:27, DJ Delorie <d...@redhat.com> wrote:
>> Manuel Lpez-Ibñez <lopeziba...@gmail.com> writes:
>>
>>> none? for libiberty, no regular maintainer for build machinery,
>>
>> Perhaps this is a sign that I should step down as maintainers for those?
>
> I don't see how that helps. Neither my message nor Thomas's is a
> criticism of people. The question is how to get more people to help
> and how to improve the situation. For sure, everybody is doing the
> best that they can with the time that they have.

Another question is how to help existing maintainers such that they
are more motivated to review patches. Is it a lack of time? lack of
Interest in the project? do patches simply fall through the cracks? is
it a dead-lock of people waiting for each other to comment?

There were some comments by Richard and Eric here:
https://gcc.gnu.org/ml/gcc/2010-04/msg00736.html
but they do not seem to apply in this case.

Re: [GCC Steering Committee attention] [PING] [PING] [PING] libgomp: In OpenACC testing, cycle though $offload_targets, and by default only build for the offload target that we're actually going to te

[Manuel López-Ibáñez]

On 4 August 2016 at 21:27, DJ Delorie  wrote:
> Manuel Lpez-Ibñez  writes:
>
>> none? for libiberty, no regular maintainer for build machinery,
>
> Perhaps this is a sign that I should step down as maintainers for those?

I don't see how that helps. Neither my message nor Thomas's is a
criticism of people. The question is how to get more people to help
and how to improve the situation. For sure, everybody is doing the
best that they can with the time that they have.

Cheers,

Manuel.

Re: [GCC Steering Committee attention] [PING] [PING] [PING] libgomp: In OpenACC testing, cycle though $offload_targets, and by default only build for the offload target that we're actually going to te

[Manuel López-Ibáñez]

On 04/08/16 15:49, Thomas Schwinge wrote:

I suppose, if I weren't paid for paid for this, I would have run away
long ago, and would have looked for another project to contribute to.
:-(


You are a *paid* developer for one of the most active companies in the GCC 
community. Imagine how it feels for someone who just convinced their company to 
let them contribute for the first time or a volunteer:


https://gcc.gnu.org/ml/gcc/2010-04/msg00667.html
https://gcc.gnu.org/ml/gcc-patches/2016-08/msg00093.html

They will never bother to send an email like this and just silently go away. 
People do give up and move somewhere else based on this problem only. (And 
nowadays, this decision is quite easy to make or sell to one's boss)



OpenACC/OpenMP/offloading patches.  I'm certainly not going in any way to
disapprove Jakub's help, skills and experience, but I'm more and more
worried about this "bus factor" of one single person
().


This is a problem throughout GCC. We have a single C++ maintainer, a single 
part-time C maintainer, none? for libiberty, no regular maintainer for build 
machinery, and so on and so forth.


This has been a problem for a very long time, but it seems to be getting worse 
now that fewer contributors are doing most of the work.


Note that it has been more than one year now that just Clang has more monthly 
contributors than the whole GCC project:


https://www.openhub.net/p/_compare?project_0=GNU+Compiler+Collection_1=LLVM%2FClang+C+family+frontend_2=The+LLVM+Compiler+Infrastructure



... if I remember correctly), was that in addition to him, all
Global Reviewers are welcome to review OpenACC/OpenMP/offloading patches.
But that doesn't help if that's then not happening in reality.  (With the
exception of Bernd, who then did review such patches for a while, but
also seems to have stopped with that again.)


At least half of the global reviewers in MAINTAINERS never review any patches. 
Most of them are not active any more and presumably do not read GCC emails.


I'm not sure how to address this problem, but there is definitely a problem. 
Some ideas:


a) Follow LLVM's model and allow changes to be reviewed after commit.

Advantages: Faster turnaround, less frustration. No more effort than now, 
possibly less by not requiring pinging. etc.


Disadvantages: code may go unreviewed; churn in the tree because of iterative 
changes or reverts.


b) Anyone with svn-write powers can approve patches.

Advantages: More reviewers, faster turn around, less frustration. Distribute 
the work.
Disadvantages: Poor patches may get accepted interfering with the work from the 
most-active contributors. There is no real incentive to review patches, thus 
the actual situation may not change significantly.


c) Everybody have to get their patches reviewed, promote a "Review-by" economy.

Advantages: Actively encourage people to review each other patches. Distribute 
effort. Ideally, faster turn-around and less frustration.


Disadvantages: It may slow down people that were able to self-approve. 
Additional work for people that contribute a lot in finding reviews. People 
that contribute little have little motivation to review.


d) Delegate hierarchically. Module owners should seek and delegate to people 
with svn-write powers and ask for reviews in exchange of reviews.


Advantages: No loss in quality, distribute work, creates an economy of reviews.

Disadvantages: More work required from module owners to keep track of patches, 
reviews and possible reviewers. Possibly offset by not having to do in-depth 
reviews themselves.


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: diag color

[Manuel López-Ibáñez]

On 31 July 2016 at 22:59, Jonathan Wakely  wrote:
> On 31 July 2016 at 22:06, phi gcc wrote:
>> bugzilla don't likes me, can't get in
>> Ok let's forget participation then...
>
> We were attacked by spammers last week and had to temporarily disable
> account creation. The notice you got from bugzilla should have given
> the details of an admin to contact who will create an account for you.

Actually, I haven't been able to find a way to reach the
createaccount.cgi page from within bugzilla. It seems the links
themselves have been removed.

Cheers,

Manuel.

Re: diag color

[Manuel López-Ibáñez]

On 31 July 2016 at 21:54, phi gcc  wrote:
> Why should I ? I am not a gcc designer, just humbelly reporting a

Anybody can become a GCC developer, if they want to :)

> At some point one suggested reading the source, I did it real quick,
> and it appears trivial that getenv("TERM") is wrongly processed, made

https://gcc.gnu.org/wiki/Community point 4:
If you do not have the time/resources/people to implement your own
idea, do not expect GCC developers dropping what they are doing to
help you. Volunteers have very very limited time and paid developers
are paid to do something else. In fact, asking GCC developers to do
anything for you, no matter how trivial it seems to you, will likely
result in negative feedback. Probably it is no trivial at all.

:)

> Now gcc team can simply trash this, but if gcc can't handle trivial
> request like this, sounds a bit scary for the future :)

Nobody is trashing anything. My intention was only to helpfully point
out the most direct route to get your issue fixed.
:)

Speaking for myself, I don't understand what is "trivially wrong" with
the code that you mention and you didn't explain how you will fix it.
It doesn't matter to me, because even if you had explained, I don't
have enough free time to fix it for you, since it is never as trivial
as you think it is and you will never realize this until you try
yourself.

But if you found a workaround that makes you happy, let's save
everybody's time and stop here :)

> bugzilla don't likes me, can't get in
> Ok let's forget participation then...

Account creation may be disabled at the moment. We have been suffering
spam attacks lately. See
https://gcc.gnu.org/bugzilla/createaccount.cgi

Cheers,

Manuel.

Re: diag color

[Manuel López-Ibáñez]

On 31/07/16 13:16, phi gcc wrote:

I admit I red this a bit too fast, and since the doc sez "if
GCC_COLORS isn't present" I didn't infered what it does if set. I
didn't saw the =never was a goof for the env var.

I guess I must not be the only one trapped here.

Yet I still believe it is wrongly coded, a very little logic in the
init() path would allow every one to be happy :)


If you think you can code it better, prove it:

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

That also applies to documentation changes.

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: [libiberty] does anyone use regex.c with REGEX_MALLOC?

[Manuel López-Ibáñez]

On 29 July 2016 at 16:25, Jeff Law  wrote:
>> Well, if libiberty is going to be replaced en masse by gnulib, then
>> there's no sense in me cleaning up libiberty's regex.

libiberty cannot be replaced completely, because there are bits that
do not even exist in gnulib. And given the time frame, I don't think
Ayush will have time to replace all functions that already exist in
gnulib. This will need to be an incremental process. What I expect to
achieve is that we never need to re-implement in libiberty any
function that already exists in gnulib.

>> Before I spend any more time on this, is there a plan to review Ayush's
>> work for say GCC 7?  I obviously only care about regex* right now, or
>> any alloca users in libiberty :).
>
> I'm not sure who's taking the lead on reviewing Ayush's work.  But I do
> expect someone will own it at some point in advance of stage1 close.

It is mostly build system bits. We need a build-system reviewer (or a
global reviewer). So far, Ayush haven't found any case that needs
upstream (gnulib) or gcc-specific changes.

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: [libiberty] does anyone use regex.c with REGEX_MALLOC?

[Manuel López-Ibáñez]

On 25/07/16 21:16, Joseph Myers wrote:

On Mon, 25 Jul 2016, Jeff Law wrote:


I'll pre-approve removing those bits.  Alternately, you could look to resync
with glibc, though that could prove painful after 15 years of divergence.


The current glibc implementation is completely different; the libiberty
version was replaced in glibc many years ago.  Obsolete libiberty by
gnulib for all its users and you get a portable version of the current
glibc regex that way


BTW, this is what Ayush is trying to do:

https://gcc.gnu.org/ml/gcc-patches/2016-07/msg01302.html

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

It would be great if someone could review his patches. I cannot approve them 
myself.


You could even ask him to look next at replacing the libiberty version of 
regex.c with gnulib's in GCC. It is on his TODO list.


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: Should we import gnulib under gcc/ or at the top-level like libiberty?

[Manuel López-Ibáñez]

On 11 July 2016 at 14:40, Ian Lance Taylor <i...@google.com> wrote:
> On Sun, Jul 10, 2016 at 10:15 AM, Manuel López-Ibáñez
> <lopeziba...@gmail.com> wrote:
>> On 23 June 2016 at 18:02, Pedro Alves <pal...@redhat.com> wrote:
>>> But on the other hand, the idea of maintaining multiple gnulib
>>> copies isn't that appealing either.  Considering that the long
>>> term desired result ends up with a libiberty that is no longer a
>>> portability library, but instead only an utilities library, then to
>>> get to that stage, the other programs in the binutils-gdb repo which
>>> rely on libiberty too, binutils proper, gas, ld, gold, etc., need
>>> to be converted to use gnulib as well.  And then a single
>>> gnulib sounds even more appealing.
>>
>> AFAICT, the only "utilities" found in libiberty not appropriate for
>> gnulib is the demangler. That would be more appropriate for a
>> libdemangler library shared among all gnutools.
>
> Does gnulib have a functional equivalent to the pex and simple-object
> mini-libraries?

I don't really know. Part of the GSoC project is to figure out the
answer to such questions. In any case, the main goal of the GSoC is to
start using gnulib, so that next time we need something not provided
by libiberty we can simply import it from gnulib rather than having to
implement it ourselves or manually import it into libiberty and keep
it in sync.
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: "error: static assertion failed: [...]" (was: [GCC Wiki] Update of "DiagnosticsGuidelines" by MartinSebor)

[Manuel López-Ibáñez]

On 13/07/16 14:26, Thomas Schwinge wrote:

Hi!

I had recently noticed that given:

 #ifndef __cplusplus /* C */
 _Static_assert(0, "foo");
 #else /* C++ */
 static_assert(0, "foo");
 #endif

..., for C we diagnose:

 [...]:2:1: error: static assertion failed: "foo"
  _Static_assert(0, "foo");
  ^~

..., and for C++ we diagnost:

 [...]:4:1: error: static assertion failed: foo
  static_assert(0, "foo");
  ^

("foo" quoted vs. un-quoted.)  Assuming this difference between C and C++
diagnostics is not intentional, which one should we settle on?  I thought
I'd like the un-quoted version better, but judging by Martin's recent
wiki change (see below), "foo" is a string constant, so should be quoted
in diagnostics?  If yes, OK to commit to trunk the obvious changes (plus
any testsuite updates)?



From a diagnostics point-of-view, neither version is quoted:

c/c-parser.c: error_at (assert_loc, "static assertion failed: %E", string);

cp/semantics.c: error ("static assertion failed: %s",

To be "quoted", it would need to use either %q or %<%>. Note that %qs would 
produce `foo' not "foo". Nevertheless, we probably want to print 'x' for 
character literals and not `'x'' and "string" for string literals and not 
`string'. Thus, the wiki should probably be amended to clarify this.


Also, there is a substantial difference between %E and %s when the string 
contains control characters such as \n \t \u etc. Clang uses something similar 
to %E.


For comparison, we use %s to print

test.c:1:9: note: #pragma message:
string
#pragma message "\nstring"
^

Cheers,
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: Should we import gnulib under gcc/ or at the top-level like libiberty?

[Manuel López-Ibáñez]

On 11 July 2016 at 13:53, Mikhail Maltsev <malts...@gmail.com> wrote:
> On 07/10/2016 08:15 PM, Manuel López-Ibáñez wrote:
>> Moving all gnutools to a single git/svn repository that can still be
>> built piece-wise would help sharing gnulib and other useful libraries.
>> If LLVM can do it, there is no reason why gnutools can't. And they
>> have shown that it helps code reuse and modular design. All the manual
>> syncing between gnu projects is a waste of time.
>
> But LLVM does not keep everything in a single repository. In fact, it's quite
> the opposite: they have a separate repo for Clang (the frontend, ~ gcc/c, cp,
> ...), for compiler-rt (~ libgcc), for libc++ (~ libstdc++).
>
> All utilities (~ libiberty) live in the LLVM repo (include/llvm/ADT,
> include/llvm/Support, lib/Support). Other projects, like LLDB, are checked out
> into a subdirectory, and are always built from the combined tree.

I stand corrected. It has been a while since I built the whole LLVM
toolchain and I was misremembering the details.

Thanks,

Manuel.

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: Should we import gnulib under gcc/ or at the top-level like libiberty?

[Manuel López-Ibáñez]

On 23 June 2016 at 18:02, Pedro Alves  wrote:
> But on the other hand, the idea of maintaining multiple gnulib
> copies isn't that appealing either.  Considering that the long
> term desired result ends up with a libiberty that is no longer a
> portability library, but instead only an utilities library, then to
> get to that stage, the other programs in the binutils-gdb repo which
> rely on libiberty too, binutils proper, gas, ld, gold, etc., need
> to be converted to use gnulib as well.  And then a single
> gnulib sounds even more appealing.

AFAICT, the only "utilities" found in libiberty not appropriate for
gnulib is the demangler. That would be more appropriate for a
libdemangler library shared among all gnutools.

Moving all gnutools to a single git/svn repository that can still be
built piece-wise would help sharing gnulib and other useful libraries.
If LLVM can do it, there is no reason why gnutools can't. And they
have shown that it helps code reuse and modular design. All the manual
syncing between gnu projects is a waste of time.

> In any case, we don't really _need_ to consider sharing right now.
> gcc can start slow, and import and convert to use gnulib modules
> incrementally, instead of having it import all the modules
> gdb is importing from the get go.

Nevertheless, it would be good to remove from the gcc's libiberty any
file/function not used by any GNU project.

Moreover, does it make sense that GCC remains the master copy if GCC
manages to remove every use of libiberty except the demangler?

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: Deprecating basic asm in a function - What now?

[Manuel López-Ibáñez]

On 22 June 2016 at 20:28, Andrew Pinski <pins...@gmail.com> wrote:
> On Wed, Jun 22, 2016 at 12:26 PM, Manuel López-Ibáñez
> <lopeziba...@gmail.com> wrote:
>> On 22 June 2016 at 19:05, Andrew Pinski <pins...@gmail.com> wrote:
>>> Note each target in gas has its own way of parsing assembly code which
>>> is one of the reason why it is so hard todo the above and also each
>>> target has its own wording which can confuse people.  I think if you
>>
>> I didn't say it was easy. Yet, Clang/LLVM did it.
>
> That is because there are less targets on the Clang/LLVM side of things.

And not even all of them have an integrated assembler. Yet, having it
for the most popular targets is enough. Having the infrastructure in
place is far more important, since it allows people interested in the
less popular ones to do the work necessary to implement it.

Cheers,

Manuel.

Re: Deprecating basic asm in a function - What now?

[Manuel López-Ibáñez]

On 22 June 2016 at 19:05, Andrew Pinski <pins...@gmail.com> wrote:
> On Wed, Jun 22, 2016 at 10:59 AM, Manuel López-Ibáñez
> <lopeziba...@gmail.com> wrote:
>> On 22/06/16 10:02, Florian Weimer wrote:
>>> GCC could parse the assembly instructions and figure out the clobbers.
>>
>>
>> Which is also needed for various things, such as providing better
>> diagnostics:
>>
>> http://permalink.gmane.org/gmane.comp.compilers.llvm.cvs/70335
>
> Actually GCC outputs markers that modern gas understands and you get
> much better diagnostic already compared to what was reported above.

We get better diagnostics than in the past, but not as good as
Clang's. And there are many limitations of the "markers" approach: Not
only there is no column info, line info is often wrong:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57950

And there remains a lot of work to get GAS to output diagnostics like
GCC does nowadays (colors, caret, fix-it hints, etc.). And additional
work to make GAS obey GCC settings (if GCC disables colors, then GAS
should not output colors). Most of that work will be duplication of
what GCC already does.

Besides diagnostics, the integrated assembler is faster (as proven by
David Malcom's experiments) and it provides additional info to GCC
(the point raised by Florian).

> Note each target in gas has its own way of parsing assembly code which
> is one of the reason why it is so hard todo the above and also each
> target has its own wording which can confuse people.  I think if you

I didn't say it was easy. Yet, Clang/LLVM did it.

> want better diagnostic from assembly code, then working on binutils to
> unify things including error messages (and subtarget support) would be
> a much better use of time than integrating binutils into gcc.

Unifying error messages does not seem to be a feature used to
advertise Clang as a replacement for GCC. It doesn't seem to be what
users discuss in various forums (including the reddit thread I quoted
above). It won't help with the issues discussed in this thread.

Cheers,

Manuel.

Re: Deprecating basic asm in a function - What now?

[Manuel López-Ibáñez]

On 22/06/16 10:02, Florian Weimer wrote:

On 06/21/2016 06:53 PM, Andrew Haley wrote:

Me too.  I wonder if there's anything else we can do to make basic asm
in a function a bit less of a time bomb.


GCC could parse the assembly instructions and figure out the clobbers.


Which is also needed for various things, such as providing better diagnostics:

http://permalink.gmane.org/gmane.comp.compilers.llvm.cvs/70335
http://blog.llvm.org/2010/04/intro-to-llvm-mc-project.html
https://www.reddit.com/r/programming/comments/bnhxb/clang_now_with_inline_assembly_diagnostics/
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57950

Of course, that would require a closer integration with binutils. There has 
been some work in that direction in the past: 
https://sourceware.org/ml/binutils/2015-06/msg00010.html


but like the GCC-GDB integration, it seems to have stalled or be happening 
behind closed doors.


Cheers,

Manuel.

Re: Disabling warn_unused_result warnings on a case-by-case basis

[Manuel López-Ibáñez]

On 14/06/16 10:32, Florian Weimer wrote:

A long time ago, GCC decided that warn_unused_result warnings should *not* be
silenced by casting to void, as in:

   (void) write (STDOUT_FILENO, message, strlen (message));

Apparently, programmers have figured out to use this idiom as a replacement:

   if (write (STDOUT_FILENO, message, strlen (message))) { }

I'm not sure if this is an improvement.  The (void) idiom seems to make the
programmer intention more explicit.

Maybe it's time to reconsider and suppress the warning for casts to (void), too?


There is a thorough discussion and some analysis here:

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

I think most of the pushback against the (void) idiom has actually come from 
GNU libc in the past.


One possible solution, if both behaviours are desired, is to warn for the void 
idiom only for a new -Wstrict-unused-result, such that:


write (STDOUT_FILENO, message, strlen (message));
// warning: ignoring return value of ‘write’, declared with attribute 
warn_unused_result [-Wunused-result]

(void) write (STDOUT_FILENO, message, strlen (message));
// warning: ignoring return value of ‘foo’, declared with attribute 
warn_unused_result [-Wstrict-unused-result]


and -Wno-strict-unused-result disables warnings for (void).

But perhaps it is simpler to just change the default. It seems unlikely to use 
(void) unintentionally.


Cheers,

Manuel.

Re: CppCoreGuidelines warnings

[Manuel López-Ibáñez]

On 17 May 2016 at 12:10, Christopher Di Bella  wrote:
> Just letting you know I'm still alive!
>
> I'm currently waiting on approval from my employer before I move ahead
> with anything; for now, it's just personal research to help ease into
> it. Approval may take a month or two, as I work for a large
> corporation.

Please also note that, in terms of legal papers, the FSF is much more
flexible than one may think, but they are not very pro-active or fast
(in my past experience, things may have changed now). If you find some
internal resistance at your company, it would be good to figure out
what are the key issues for your employer and what are the possible
trade-offs, then explain them to the FSF legal team. If there is any
possible solution at all, they will surely find it out. But it would
be better to discuss all possibilities with your employer to avoid
wasteful back and forth.

>> You may wish to check also the Getting Started checklist at:
>
> Although there isn't much that I can publicly do at the moment, please
> let me know if there's anything else I can do to prepare myself; I'm
> currently researching static analysis and becoming familiar with the
> gcc codebase/contributing tips.

The 10-step checklist should help you with that: how to set-up your
dev environment, what is the process for writing, testing and
submitting patches, and some suggestions on how to interact with the
community. It is not the official contribution documentation where all
the formal details are written, but more of an organized set of tips
and advice. You don't have to finish step #1 before looking at the
rest. Note also that if you want to learn the process, small patches
do not need any legal papers: Find a typo or an obvious bug and submit
a patch following the procedure. I would suggest that an easy, mostly
about process and less about code, not copyright-able task is to look
for testcases that are duplicated in gcc.dg/ and g++.dg/ and merge
them as a unique testcase in c-c++-common/. Potential targets may have
the same name, or they may use the same dg-option flags, or they may
produce the same output when running the testsuite.

Cheers,

Manuel.

Re: CppCoreGuidelines warnings

[Manuel López-Ibáñez]

On 09/05/16 10:18, Jonathan Wakely wrote:

On 8 May 2016@02:10, Christopher Di Bella wrote:

If not, I'd like to get a start on implementing a warning system for
them. I'll create a branch, but I doubt it'll be ready for gcc 7.1's
release.


Hi, I don't think anyone is working on that yet.

See https://gcc.gnu.org/contribute.html for some prerequisites to
contributing significant changes to GCC.


You may wish to check also the Getting Started checklist at:

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

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: Bug maintenance

[Manuel López-Ibáñez]

On 08/05/16 23:13, Oleg Endo wrote:

There are nearly 10,000 still unresolved bugs in Bugzilla, almost
half of which are New, and a third Unconfirmed, so I'm sure any
effort to help reduce the number is of value and appreciated.


That's exactly what prompted me to ask.  There's such a vast number
of them, it's hard to believe that 9 year old bugs are still of
interest.


Sometimes there is.  Before randomly closing any bugs because they are
too old, one should@least have a look@them and see if they're
still an issue etc.  Often things would've been fixed along the way,
but not all of them.


There are some 10-years old bugs that have a very clear description of what 
needs to be done to fix them, it is just that no one has had time to do it yet. 
Others don't have a clear fix, but there is a lot of info about things tried 
but failed. Losing all that info would be bad.


My humble opinion is that going through the list from old to new is not the 
most useful or efficient way to contribute to GCC (if it is the only way you 
want to contribute, then please go ahead, it is still useful). Old bugs do not 
hurt anyone except perhaps when searching for duplicates. In that case, it may 
be worth spending a few minutes checking if it is fixed already, ask the 
submitter for more info, or confirm it if UNCONFIRMED and updating the 
description so one can see clearly that it is not a duplicate.


Triaging old bugs (except for fixing them) is not the most useful: users may 
have simply forgotten all about it or not be able to reproduce it anymore or 
moved on and not care...


On the other hand, it is rather more useful to start with recent bugs, which 
are more likely to be relevant, and confirm them, ask for more info, find 
oldest duplicate with more info, or classify them under various meta-bugs.


Rather than seeing Bugzilla as a TODO list for devs, it is rather more precise 
to see it as a knowledge database about bugs.


Cheers,
Manuel.

Re: Please, take '-Wmisleading-indentation' out of -Wall

[Manuel López-Ibáñez]

On 04/05/16 19:20, David Malcolm wrote:

On Wed, 2016-05-04@18:15 +0200, Antonio Diaz Diaz wrote:

- It can't be portably disabled; older versions of gcc do not
accept
  '-Wno-misleading-indentation'. (At least 4.1.2 does not accept
it).


FWIW "-Wall -Wno-misleading-indentation" works for me with gcc 4.8.3


It should work since GCC 4.4 (7 years old). See 
https://gcc.gnu.org/wiki/FAQ#wnowarning



- -Wempty-body is much simpler to test for, and in general less
  questionable than -Wmisleading-indentation, yet it is not
enabled by
  -Wall.



Many useful warnings are outside -Wall/-Wextra because there were bugs when 
initially implemented, users complained, they were moved out and then either 
the bugs were forgotten or they got fixed but nobody bothered to move them 
again within -Wall/-Wextra. See https://gcc.gnu.org/PR52961


Nowadays it is extremely easy to disable a warning that annoys you (the name is 
written in the message and you can use #pragmas very selectively), but still 
quite hard to discover which warning you need to enable that could have found a 
certain bug. It is also much harder to find regressions in warnings not enabled 
by -Wall -Wextra because they are not tested as much.


I really commend David for being brave and putting the warning in -Wall. The 
easy way out would have been to say: "I know how to enable it, so let's hide it 
so that users do not complain to me about bugs that I don't suffer". We (myself 
included) have done this plenty of times in the past and the result is always 
the same: bugs don't get fixed, regressions appear, and users complain about 
missing warnings that are actually already implemented.



I think that keeping separated categories of warnings (instead of
warning about everything by default) is a valuable feature. Maybe
both
-Wempty-body and -Wmisleading-indentation (and any future similar
options) could be put in a new category (-Wcoding-style or
-Wstatic-analysis, for example).


I agree that GCC warnings could be better categorized, but your proposed 
categories are not clearly defined. A better classification would look at the 
possibilities of false positives, how easy is to silence it, whether it is 
"undefined/unspecified at runtime", "undefined/unspecif under some conditions 
but not others", "atypical code that is often/sometimes a bug", "typical code 
that is often/sometimes a bug", etc. Of course, this would be a lot of work 
that no one wants to do ;-)


Cheers,

Manuel.

Re: Getting format of arg_type

[Manuel López-Ibáñez]

On 15 April 2016 at 14:34, Prasad Ghangal  wrote:
> Hi!
>
> Regarding PR64955, I was observing function format_type_warning() (in
> c-family/c-format.c), how can I get format specifier for arg_type?
>
> Say, if tree arg_type stores 'char', then how can I get its format i.e. 'c' ?

That information is in the various *_table that you can find near the
top of c-format.c. The types in the tables are defined by various
'#define T_*' in c-format.h. I wish the names were a bit more
descriptive.

As far as I know, there is no function that given a type, gives you
the most appropriate format. Writing such a function is the main
challenge in that PR. Basically you need to figure out what the
existing code is doing to find out, given a format such as 'c', that
the wanted_type is 'char', and reverse that logic. (There may be more
than one possible valid format given a type).

Moreover, the whole c-format.* is a very old piece of GCC. If you can
think about simplifications or additional comments that could help
people like you in understanding it, please do not hesitate to propose
patches in that direction. I don't think we even have a short
description on how to add new format specifiers or flags to the
tables. That would be extremely useful to add at the top of the file.

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: Need suggestion about bug 68425

[Manuel López-Ibáñez]

On 3 April 2016 at 16:56, Prasad Ghangal  wrote:
>
> Also for
>
> int array[10];
> array[100]=10;
>
> Currently, GCC doesn't emit any warning (even with -Wall option)
>
> Wouldn't it be nice if GCC gives some warning like Clang, which gives:
>
> foo.c:4:3: warning: array index 100 is past the end of the array
> (which contains 10 elements) [-Warray-bounds]
>   array[100]=10;
>   ^ ~~~

Yes, it would be very nice. This is
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=35587 which is quite old.
However, there is the issue of warning for code that is clearly not
executed (for example, within if(0){}). Not sure if Clang tracks that.

Cheers,

Manuel.

Re: Re: stray quotation marks warning enhancement or extension

[Manuel López-Ibáñez]

On 31/03/16 23:23, Jonathan Wakely wrote:

On 31 March 2016@21:10, Daniel Gutson wrote:

Hi,

   many times we copy code snippets from sources that change the
Unicode quotation marks ( “ ” ) rather than " ". For example

  const std::string a_string(“Hello”);

That line looks innocent but causes gcc to say

x.cpp:4:1: error: stray ‘\342’ in program
  const std::string a_string(“Hello”);
  ^

misleading the poor programmer with such error message and wrong
column. A quick Google search says there are 171,000 matches for "
error: stray ‘\342’ in program" which may show that this is a very
common issue.

[...]

* improve the error message for the case of the Unicode quotes such
as adding "(seems Unicode quotes where used)"


IMHO this would be better.


Note that GCC 6.0 has the capability of issuing something like:

x.cpp:4:28: error: Unicode quotation marks ‘\342’ are not valid
  const std::string a_string(“Hello”);
 ^
 "
or even:

x.cpp:4:28: error: Unicode quotation marks ‘\342’ are not valid
 const std::string a_string(“Hello”);
^~~
"Hello"

Smart editors might be able to apply the fix automatically given the above.

Cheers,

Manuel.