Re: [pushed 2/3] libcpp: move label_text to its own header

[Bert Wesarg]

Dear David,

On Tue, May 28, 2024 at 10:07 PM David Malcolm  wrote:
>
> No functional change intended.
>
> Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
> Pushed to trunk as r15-874-g9bda2c4c81b668.
>
> libcpp/ChangeLog:
> * Makefile.in (TAGS_SOURCES): Add include/label-text.h.
> * include/label-text.h: New file.
> * include/rich-location.h: Include "label-text.h".
> (class label_text): Move to label-text.h.
>
> Signed-off-by: David Malcolm 
> ---
>  libcpp/Makefile.in |   2 +-
>  libcpp/include/label-text.h| 102 +
>  libcpp/include/rich-location.h |  79 +
>  3 files changed, 105 insertions(+), 78 deletions(-)
>  create mode 100644 libcpp/include/label-text.h
>
> diff --git a/libcpp/Makefile.in b/libcpp/Makefile.in
> index ebbca3fb..7e47153264c0 100644
> --- a/libcpp/Makefile.in
> +++ b/libcpp/Makefile.in
> @@ -271,7 +271,7 @@ ETAGS = @ETAGS@
>
>  TAGS_SOURCES = $(libcpp_a_SOURCES) internal.h system.h ucnid.h \
>  include/cpplib.h include/line-map.h include/mkdeps.h include/symtab.h \
> -include/rich-location.h
> +include/rich-location.h include/label-text.h

this does not seem to be enough that the new header will be installed.
I get compile errors when compiling an plug-in with this patch:

In file included from
/home/bitten/opt/gcc-15-20240602/lib/gcc/x86_64-pc-linux-gnu/15.0.0/plugin/include/diagnostic.h:24,
from 
/home/bitten/builds/oCyPvWN6/1/perftools/cicd/scorep/src/build-gcc-plugin/../src/adapters/compiler/gcc-plugin/scorep_plugin_inst_descriptor.cpp:43:
/home/bitten/opt/gcc-15-20240602/lib/gcc/x86_64-pc-linux-gnu/15.0.0/plugin/include/rich-location.h:25:10:
fatal error: label-text.h: No such file or directory
25 | #include "label-text.h"
| ^~
compilation terminated.

Best,
Bert

>
>
>  TAGS: $(TAGS_SOURCES)
> diff --git a/libcpp/include/label-text.h b/libcpp/include/label-text.h
> new file mode 100644
> index ..13562cda41f9
> --- /dev/null
> +++ b/libcpp/include/label-text.h
> @@ -0,0 +1,102 @@
> +/* A very simple string class.
> +   Copyright (C) 2015-2024 Free Software Foundation, Inc.
> +
> +This program is free software; you can redistribute it and/or modify it
> +under the terms of the GNU General Public License as published by the
> +Free Software Foundation; either version 3, or (at your option) any
> +later version.
> +
> +This program is distributed in the hope that it will be useful,
> +but WITHOUT ANY WARRANTY; without even the implied warranty of
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +GNU General Public License for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with this program; see the file COPYING3.  If not see
> +.
> +
> + In other words, you are welcome to use, share and improve this program.
> + You are forbidden to forbid anyone else to use, share and improve
> + what you give them.   Help stamp out software-hoarding!  */
> +
> +#ifndef LIBCPP_LABEL_TEXT_H
> +#define LIBCPP_LABEL_TEXT_H
> +
> +/* A struct for the result of range_label::get_text: a NUL-terminated buffer
> +   of localized text, and a flag to determine if the caller should "free" the
> +   buffer.  */
> +
> +class label_text
> +{
> +public:
> +  label_text ()
> +  : m_buffer (NULL), m_owned (false)
> +  {}
> +
> +  ~label_text ()
> +  {
> +if (m_owned)
> +  free (m_buffer);
> +  }
> +
> +  /* Move ctor.  */
> +  label_text (label_text &&other)
> +  : m_buffer (other.m_buffer), m_owned (other.m_owned)
> +  {
> +other.release ();
> +  }
> +
> +  /* Move assignment.  */
> +  label_text & operator= (label_text &&other)
> +  {
> +if (m_owned)
> +  free (m_buffer);
> +m_buffer = other.m_buffer;
> +m_owned = other.m_owned;
> +other.release ();
> +return *this;
> +  }
> +
> +  /* Delete the copy ctor and copy-assignment operator.  */
> +  label_text (const label_text &) = delete;
> +  label_text & operator= (const label_text &) = delete;
> +
> +  /* Create a label_text instance that borrows BUFFER from a
> + longer-lived owner.  */
> +  static label_text borrow (const char *buffer)
> +  {
> +return label_text (const_cast  (buffer), false);
> +  }
> +
> +  /* Create a label_text instance that takes ownership of BUFFER.  */
> +  static label_text take (char *buffer)
> +  {
> +return label_text (buffer, true);
> +  }
> +
> +  void release ()
> +  {
> +m_buffer = NULL;
> +m_owned = false;
> +  }
> +
> +  const char *get () const
> +  {
> +return m_buffer;
> +  }
> +
> +  bool is_owner () const
> +  {
> +return m_owned;
> +  }
> +
> +private:
> +  char *m_buffer;
> +  bool m_owned;
> +
> +  label_text (char *buffer, bool owned)
> +  : m_buffer (buffer), m_owned (owned)
> +  {}
> +};
> +
> +#endif /* !LIBCPP_LABEL_TEXT_H  */
> diff --git a/libcpp/include/rich-location.h b/libcpp/include/rich-location

Re: [pushed 2/3] libcpp: move label_text to its own header

[Bert Wesarg]

Hi,

On Thu, Jun 6, 2024 at 7:05 PM Andrew Pinski  wrote:
>
> On Thu, Jun 6, 2024 at 9:00 AM David Malcolm  wrote:
> >
> > On Thu, 2024-06-06 at 08:40 -0700, Andrew Pinski wrote:
> > > On Thu, Jun 6, 2024 at 6:02 AM Bert Wesarg
> > >  wrote:
> > > >
> > > > Dear David,
> > > >
> > > > On Tue, May 28, 2024 at 10:07 PM David Malcolm
> > > >  wrote:
> > > > >
> > > > > No functional change intended.
> > > > >
> > > > > Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
> > > > > Pushed to trunk as r15-874-g9bda2c4c81b668.
> > > > >
> > > > > libcpp/ChangeLog:
> > > > > * Makefile.in (TAGS_SOURCES): Add include/label-text.h.
> > > > > * include/label-text.h: New file.
> > > > > * include/rich-location.h: Include "label-text.h".
> > > > > (class label_text): Move to label-text.h.
> > > > >
> > > > > Signed-off-by: David Malcolm 
> > > > > ---
> > > > >  libcpp/Makefile.in |   2 +-
> > > > >  libcpp/include/label-text.h| 102
> > > > > +
> > > > >  libcpp/include/rich-location.h |  79 +
> > > > >  3 files changed, 105 insertions(+), 78 deletions(-)
> > > > >  create mode 100644 libcpp/include/label-text.h
> > > > >
> > > > > diff --git a/libcpp/Makefile.in b/libcpp/Makefile.in
> > > > > index ebbca3fb..7e47153264c0 100644
> > > > > --- a/libcpp/Makefile.in
> > > > > +++ b/libcpp/Makefile.in
> > > > > @@ -271,7 +271,7 @@ ETAGS = @ETAGS@
> > > > >
> > > > >  TAGS_SOURCES = $(libcpp_a_SOURCES) internal.h system.h ucnid.h \
> > > > >  include/cpplib.h include/line-map.h include/mkdeps.h
> > > > > include/symtab.h \
> > > > > -include/rich-location.h
> > > > > +include/rich-location.h include/label-text.h
> > > >
> > > > this does not seem to be enough that the new header will be
> > > > installed.
> > > > I get compile errors when compiling an plug-in with this patch:
> > > >
> > > > In file included from
> > > > /home/bitten/opt/gcc-15-20240602/lib/gcc/x86_64-pc-linux-
> > > > gnu/15.0.0/plugin/include/diagnostic.h:24,
> > > > from
> > > > /home/bitten/builds/oCyPvWN6/1/perftools/cicd/scorep/src/build-gcc-
> > > > plugin/../src/adapters/compiler/gcc-
> > > > plugin/scorep_plugin_inst_descriptor.cpp:43:
> > > > /home/bitten/opt/gcc-15-20240602/lib/gcc/x86_64-pc-linux-
> > > > gnu/15.0.0/plugin/include/rich-location.h:25:10:
> > > > fatal error: label-text.h: No such file or directory
> > > > 25 | #include "label-text.h"
> > > > > ^~
> > > > compilation terminated.
> > >
> > > I have a fix which I am testing.
> >
> > Likewise (and sorry about the breakage)
>
> Committed as r15-1076-g6e6471806d886b .

Thanks. I can confirm, that my external plugin builds again.

Bert

>
> >
> > Dave
> >

Re: [PATCH, stage1] Better error recovery for merge-conflict markers

[Bert Wesarg]

David,

On Thu, Apr 9, 2015 at 10:29 AM, Bert Wesarg  wrote:
> Hi David,
>
>> Various tools that operate on source code files will inject markers
>> into them when an unfixable conflict occurs in a merger.
>>
>> There appears to be no blessed standard for these conflict markers,
>> but an ad-hoc convention is for 7 '<' , '=', or '>' characters at
>> the start of a line, followed optionally by a space and optional
>> text
>>
>> e.g.:
>> <<<<<<< HEAD
>> extern int some_var;
>> ===
>> extern short some_var;
>> >>>>>>> Some other branch
>>
>> This convention is followed by GNU patch:
>>   http://git.savannah.gnu.org/cgit/patch.git/tree/src/merge.c
>> by git:
>>
>> http://git.kernel.org/cgit/git/git.git/tree/Documentation/merge-config.txt
>> and by various other tools.
>
>
> if you read both of these tools carefully, you will notice an alternative
> conflict style (named 'diff3' in both of them), that includes a third
> section, the common pre-image. Here is an example:
>
> <<<<<<< HEAD
> extern int some_var;
> ||| merge base
> extern int var;
> ===
> extern short var;
>>>>>>>>
>>>>>>>> Some other branch
>
>
> Additionally, git supports a custom conflict-marker-size to change the
> default of 7 on a per file name (the conflict-marker-size attribute). So it
> may be worthwhile to support other sizes than 7 in this patch too.

you never commentewd on my mail, but I saw this now in trunk. I can
only repeat myself here.

Thanks.

Bert

>
> Bert
>

Re: [PATCH] Add options -finstrument-functions-include-{file,function}-list

[Bert Wesarg]

Hi,

better write your own instrumentation plug-in and do the filtering on
your own. The plug-in interface exists since 4.5 so you have a much
greater version base that can support your feature already, than some
future version of GCC which may have this patch. While we didn't
announced it here on GCC, we maintain such plug-in already in Score-P
[1], and the overhead is also much lower (we also have a runtime
filter), we do not instrument inlined functions and functions from
system headers by default, and we do not need debug symbols to get
function names.

Best,
Bert

[1] www.score-p.org

On Thu, Dec 3, 2015 at 7:06 PM, Andi Drebes  wrote:
> By default -finstrument-functions instruments all functions. To limit
> instrumentation to certain functions or files it is necessary to
> specify the complement using -finstrument-functions-exclude-file-list
> or -finstrument-functions-exclude-function-list.
>
> The new options -finstrument-functions-include-file-list and
> -finstrument-functions-include-function-list make the specification of
> the complement unnecessary by allowing the user to limit
> instrumentation to a set of file names and functions.
> ---
>  gcc/common.opt   | 16 ++-
>  gcc/doc/invoke.texi  | 52 
> 
>  gcc/gimplify.c   | 51 ---
>  gcc/opts.c   | 10 +++
>  gcc/testsuite/ChangeLog  | 10 +++
>  gcc/testsuite/gcc.dg/instrument-10.c |  7 +
>  gcc/testsuite/gcc.dg/instrument-4.c  |  7 +
>  gcc/testsuite/gcc.dg/instrument-5.c  |  7 +
>  gcc/testsuite/gcc.dg/instrument-6.c  |  7 +
>  gcc/testsuite/gcc.dg/instrument-7.c  |  7 +
>  gcc/testsuite/gcc.dg/instrument-8.c  |  7 +
>  gcc/testsuite/gcc.dg/instrument-9.c  |  7 +
>  12 files changed, 172 insertions(+), 16 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/instrument-10.c
>  create mode 100644 gcc/testsuite/gcc.dg/instrument-4.c
>  create mode 100644 gcc/testsuite/gcc.dg/instrument-5.c
>  create mode 100644 gcc/testsuite/gcc.dg/instrument-6.c
>  create mode 100644 gcc/testsuite/gcc.dg/instrument-7.c
>  create mode 100644 gcc/testsuite/gcc.dg/instrument-8.c
>  create mode 100644 gcc/testsuite/gcc.dg/instrument-9.c
>
> diff --git a/gcc/common.opt b/gcc/common.opt
> index 3eb520e..ac797b3 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -97,7 +97,7 @@ int flag_gen_aux_info = 0
>  Variable
>  int flag_shlib
>
> -; These two are really VEC(char_p,heap) *.
> +; These are really VEC(char_p,heap) *.
>
>  Variable
>  void *flag_instrument_functions_exclude_functions
> @@ -105,6 +105,12 @@ void *flag_instrument_functions_exclude_functions
>  Variable
>  void *flag_instrument_functions_exclude_files
>
> +Variable
> +void *flag_instrument_functions_include_functions
> +
> +Variable
> +void *flag_instrument_functions_include_files
> +
>  ; Generic structs (e.g. templates not explicitly specialized)
>  ; may not have a compilation unit associated with them, and so
>  ; may need to be treated differently from ordinary structs.
> @@ -1477,6 +1483,14 @@ finstrument-functions-exclude-file-list=
>  Common RejectNegative Joined
>  -finstrument-functions-exclude-file-list=filename,...  Do not instrument 
> functions listed in files.
>
> +finstrument-functions-include-function-list=
> +Common RejectNegative Joined
> +-finstrument-functions-include-function-list=name,...  Only instrument 
> listed functions.
> +
> +finstrument-functions-include-file-list=
> +Common RejectNegative Joined
> +-finstrument-functions-include-file-list=filename,...  Only instrument 
> functions listed in files.
> +
>  fipa-cp
>  Common Report Var(flag_ipa_cp) Optimization
>  Perform interprocedural constant propagation.
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index 53f1fe2..ba9a3bd 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -1150,6 +1150,8 @@ See S/390 and zSeries Options.
>  -finhibit-size-directive  -finstrument-functions @gol
>  -finstrument-functions-exclude-function-list=@var{sym},@var{sym},@dots{} @gol
>  -finstrument-functions-exclude-file-list=@var{file},@var{file},@dots{} @gol
> +-finstrument-functions-include-function-list=@var{sym},@var{sym},@dots{} @gol
> +-finstrument-functions-include-file-list=@var{file},@var{file},@dots{} @gol
>  -fno-common  -fno-ident @gol
>  -fpcc-struct-return  -fpic  -fPIC -fpie -fPIE -fno-plt @gol
>  -fno-jump-tables @gol
> @@ -24529,6 +24531,56 @@ of the function name, it is considered to be a 
> match.  For C99 and C++
>  extended identifiers, the function name must be given in UTF-8, not
>  using universal character names.
>
> +@item -finstrument-functions-include-file-list=@var{file},@var{file},@dots{}
> +@opindex finstrument-functions-include-file-list
> +
> +Limit function instrumentation to functions from files specified in
> +the list. The matching of file names is identical to the matchi

Re: [PATCH 02/16] gcc: Embed the driver in-process within libgccjit

[Bert Wesarg]

On Mon, Jun 1, 2015 at 11:04 PM, David Malcolm  wrote:
> Provide a way to clean up state within the driver code, and use this
> from libgccjit to embed it in-process, rather that via pex.  Part of
> this requires restoring the environment after any putenv calls, so the
> patch introduces an env_manager class.  No effort is made to restore
> the environment for the classic use-case.
>
> This embedding gives a slight performance win for
> jit.dg/test-benchmark.c, and enables bigger performance gains in
> followup patches.
>
> I've fixed the worst of the memory leaks, but this does still
> leak somewhat.
>
> gcc/ChangeLog:
> * gcc-main.c (main): Add params to driver ctor.
> * gcc.c (class env_manager): New.
> (env): New global.
> (env_manager::init): New.
> (env_manager::getenv): New.
> (env_manager::xputenv): New.
> (env_manager::restore): New.
> Poison genenv and putenv.

getenv? The same applies to the actual #pragma statement.

Bert

Re: [PATCH, stage1] Better error recovery for merge-conflict markers

[Bert Wesarg]

Hi David,


Various tools that operate on source code files will inject markers
into them when an unfixable conflict occurs in a merger.

There appears to be no blessed standard for these conflict markers,
but an ad-hoc convention is for 7 '<' , '=', or '>' characters at
the start of a line, followed optionally by a space and optional
text

e.g.:
<<< HEAD
extern int some_var;
===
extern short some_var;
>>> Some other branch

This convention is followed by GNU patch:
  http://git.savannah.gnu.org/cgit/patch.git/tree/src/merge.c
by git:
  http://git.kernel.org/cgit/git/git.git/tree/Documentation/merge-config.txt
and by various other tools.


if you read both of these tools carefully, you will notice an 
alternative conflict style (named 'diff3' in both of them), that 
includes a third section, the common pre-image. Here is an example:


<<< HEAD
extern int some_var;
||| merge base
extern int var;
===
extern short var;

Some other branch


Additionally, git supports a custom conflict-marker-size to change the 
default of 7 on a per file name (the conflict-marker-size attribute). So 
it may be worthwhile to support other sizes than 7 in this patch too.


Bert