[RFC] More detailed diagnostics for section type conflicts

  Sometimes this is a user error, sometimes it is more of an ICE.
In either case, more information about the conflict is helpful.

I used to this to get a better idea about what is going on with
PR116887.  The original diagnostics look like this:

dl-find_object.c: In function ‘_dlfo_process_initial’:
dl-find_object.c:507:1: error: section type conflict with ‘_dlfo_nodelete_mappings’
  507 | _dlfo_process_initial (void)
      | ^~~~~~~~~~~~~~~~~~~~~
dl-find_object.c:73:40: note: ‘_dlfo_nodelete_mappings’ was declared here
   73 | static struct dl_find_object_internal *_dlfo_nodelete_mappings
      |                                        ^~~~~~~~~~~~~~~~~~~~~~~


I don't quite understand what is going on (the symbol that's being
flagged for conflict is somewhat unstable), but at least the new
diagnostics show that the sectio name, and maybe the flags are useful,
too:

/tmp/bug.i:6798:1: error: section ‘.data.rel.ro’ type conflict with ‘_dlfo_main’
 6798 | }
      | ^
/tmp/bug.i:6190:39: note: ‘_dlfo_main’ was declared here
 6190 | static struct dl_find_object_internal _dlfo_main __attribute__ ((section (".data.rel.ro")));
      |                                       ^~~~~~~~~~
/tmp/bug.i:6798:1: error: previous section type: WRITE|NOTYPE|DECLARED|NAMED
 6798 | }
      | ^
/tmp/bug.i:6798:1: error: new section type: WRITE|NOTYPE|NAMED|RELRO

I still need help with producing a non-error, non-anchored diagnostic.
I don't know how to do that.

Thanks,
Florian

	* varasm.cc (section_flags_to_string):  New function.
	(get_section): Include name of section in diagnostics.
	Print old and new section flags, as rendered by
	section_flags_to_string.

---
 gcc/varasm.cc | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 76 insertions(+), 4 deletions(-)


base-commit: 786773d4c12fd78e94f58193ff303cba19ca1b19
  

On Sun, Sep 29, 2024 at 8:13 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> Sometimes this is a user error, sometimes it is more of an ICE.
> In either case, more information about the conflict is helpful.
>
> I used to this to get a better idea about what is going on with
> PR116887.  The original diagnostics look like this:
>
> dl-find_object.c: In function ‘_dlfo_process_initial’:
> dl-find_object.c:507:1: error: section type conflict with ‘_dlfo_nodelete_mappings’
>   507 | _dlfo_process_initial (void)
>       | ^~~~~~~~~~~~~~~~~~~~~
> dl-find_object.c:73:40: note: ‘_dlfo_nodelete_mappings’ was declared here
>    73 | static struct dl_find_object_internal *_dlfo_nodelete_mappings
>       |                                        ^~~~~~~~~~~~~~~~~~~~~~~
>
>
> I don't quite understand what is going on (the symbol that's being
> flagged for conflict is somewhat unstable), but at least the new
> diagnostics show that the sectio name, and maybe the flags are useful,
> too:
>
> /tmp/bug.i:6798:1: error: section ‘.data.rel.ro’ type conflict with ‘_dlfo_main’
>  6798 | }
>       | ^
> /tmp/bug.i:6190:39: note: ‘_dlfo_main’ was declared here
>  6190 | static struct dl_find_object_internal _dlfo_main __attribute__ ((section (".data.rel.ro")));
>       |                                       ^~~~~~~~~~
> /tmp/bug.i:6798:1: error: previous section type: WRITE|NOTYPE|DECLARED|NAMED
>  6798 | }
>       | ^
> /tmp/bug.i:6798:1: error: new section type: WRITE|NOTYPE|NAMED|RELRO
>
> I still need help with producing a non-error, non-anchored diagnostic.
> I don't know how to do that.
>
> Thanks,
> Florian
>
>         * varasm.cc (section_flags_to_string):  New function.
>         (get_section): Include name of section in diagnostics.
>         Print old and new section flags, as rendered by
>         section_flags_to_string.
>
> ---
>  gcc/varasm.cc | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 76 insertions(+), 4 deletions(-)
>
> diff --git a/gcc/varasm.cc b/gcc/varasm.cc
> index 4426e7ce6c6..deba15933aa 100644
> --- a/gcc/varasm.cc
> +++ b/gcc/varasm.cc
> @@ -25,6 +25,7 @@ along with GCC; see the file COPYING3.  If not see
>     We also output the assembler code for constants stored in memory
>     and are responsible for combining constants with the same value.  */
>
> +#define INCLUDE_STRING
>  #include "config.h"
>  #include "system.h"
>  #include "coretypes.h"
> @@ -276,6 +277,65 @@ get_noswitch_section (unsigned int flags, noswitch_section_callback callback)
>    return sect;
>  }
>
> +/* Return a string describing the section flags.  */
> +
> +static std::string
> +section_flags_to_string (unsigned int flags)
> +{
> +  if (flags == 0)
> +    return "UNNAMED";
> +  std::string result;
> +  auto append = [&result] (bool f, const char *name)
> +  {
> +    if (f)
> +      {
> +       if (!result.empty ())
> +         result += '|';
> +       result += name;
> +      }
> +  };
> +
> +  append (flags & SECTION_CODE, "CODE");

I notice you capture result and it seems like you could also capture
flags and change this to:
append (SECTION_CODE, "CODE");

Thanks,
Andrew Pinski

> +  append (flags & SECTION_WRITE, "WRITE");
> +  append (flags & SECTION_DEBUG, "DEBUG");
> +  append (flags & SECTION_LINKONCE, "LINKONCE");
> +  append (flags & SECTION_SMALL, "SMALL");
> +  append (flags & SECTION_BSS, "BSS");
> +  append (flags & SECTION_MERGE, "MERGE");
> +  append (flags & SECTION_STRINGS, "STRINGS");
> +  append (flags & SECTION_OVERRIDE, "OVERRIDE");
> +  append (flags & SECTION_TLS, "TLS");
> +  append (flags & SECTION_NOTYPE, "NOTYPE");
> +  append (flags & SECTION_DECLARED, "DECLARED");
> +  append (flags & SECTION_NAMED, "NAMED");
> +  append (flags & SECTION_NOSWITCH, "NOSWITCH");
> +  append (flags & SECTION_COMMON, "COMMON");
> +  append (flags & SECTION_RELRO, "RELRO");
> +  append (flags & SECTION_EXCLUDE, "EXCLUDE");
> +  append (flags & SECTION_RETAIN, "RETAIN");
> +  append (flags & SECTION_LINK_ORDER, "LINK_ORDER");
> +
> +  unsigned int entsize = flags & SECTION_ENTSIZE;
> +  if (entsize != 0)
> +    {
> +      if (!result.empty ())
> +       result += ',';
> +      result += "size=";
> +      result += std::to_string (entsize);
> +    }
> +
> +  unsigned int mach_dep = flags / SECTION_MACH_DEP;
> +  if (mach_dep != 0)
> +    {
> +      if (!result.empty ())
> +       result += ',';
> +      result += "mach=";
> +      result += std::to_string (mach_dep);
> +    }
> +
> +  return result;
> +}
> +
>  /* Return the named section structure associated with NAME.  Create
>     a new section with the given fields if no such structure exists.
>     When NOT_EXISTING, then fail if the section already exists.  Return
> @@ -312,6 +372,9 @@ get_section (const char *name, unsigned int flags, tree decl,
>         internal_error ("section already exists: %qs", name);
>
>        sect = *slot;
> +      unsigned int original_flags = sect->common.flags;
> +      unsigned int new_flags = flags;
> +
>        /* It is fine if one of the sections has SECTION_NOTYPE as long as
>           the other has none of the contrary flags (see the logic at the end
>           of default_section_type_flags, below).  */
> @@ -355,17 +418,26 @@ get_section (const char *name, unsigned int flags, tree decl,
>               && decl != sect->named.decl)
>             {
>               if (decl != NULL && DECL_P (decl))
> -               error ("%+qD causes a section type conflict with %qD",
> -                      decl, sect->named.decl);
> +               {
> +                 error ("%+qD causes a type conflict with %qD"
> +                        " in section %qs",
> +                        decl, sect->named.decl);
> +               }
>               else
> -               error ("section type conflict with %qD", sect->named.decl);
> +               error ("section %qs type conflict with %qD",
> +                      name, sect->named.decl);
>               inform (DECL_SOURCE_LOCATION (sect->named.decl),
>                       "%qD was declared here", sect->named.decl);
>             }
>           else if (decl != NULL && DECL_P (decl))
> -           error ("%+qD causes a section type conflict", decl);
> +           error ("%+qD causes a section type conflict for section %s",
> +                  decl, name);
>           else
>             error ("section type conflict");
> +         error ("previous section type: %s",
> +                section_flags_to_string (original_flags).c_str ());
> +         error ("new section type: %s",
> +                section_flags_to_string (new_flags).c_str ());
>           /* Make sure we don't error about one section multiple times.  */
>           sect->common.flags |= SECTION_OVERRIDE;
>         }
>
> base-commit: 786773d4c12fd78e94f58193ff303cba19ca1b19
>
  

* Andrew Pinski:

>> +  append (flags & SECTION_CODE, "CODE");
>
> I notice you capture result and it seems like you could also capture
> flags and change this to:
> append (SECTION_CODE, "CODE");

Thanks, I've made the change locally.

Florian
  

On Sun, 2024-09-29 at 17:12 +0200, Florian Weimer wrote:
> Sometimes this is a user error, sometimes it is more of an ICE.
> In either case, more information about the conflict is helpful.
> 
> I used to this to get a better idea about what is going on with
> PR116887.  The original diagnostics look like this:
> 
> dl-find_object.c: In function ‘_dlfo_process_initial’:
> dl-find_object.c:507:1: error: section type conflict with
> ‘_dlfo_nodelete_mappings’
>   507 | _dlfo_process_initial (void)
>       | ^~~~~~~~~~~~~~~~~~~~~
> dl-find_object.c:73:40: note: ‘_dlfo_nodelete_mappings’ was declared
> here
>    73 | static struct dl_find_object_internal
> *_dlfo_nodelete_mappings
>       |                                       
> ^~~~~~~~~~~~~~~~~~~~~~~
> 
> 
> I don't quite understand what is going on (the symbol that's being
> flagged for conflict is somewhat unstable), but at least the new
> diagnostics show that the sectio name, and maybe the flags are
> useful,
> too:
> 
> /tmp/bug.i:6798:1: error: section ‘.data.rel.ro’ type conflict with
> ‘_dlfo_main’
>  6798 | }
>       | ^
> /tmp/bug.i:6190:39: note: ‘_dlfo_main’ was declared here
>  6190 | static struct dl_find_object_internal _dlfo_main
> __attribute__ ((section (".data.rel.ro")));
>       |                                       ^~~~~~~~~~
> /tmp/bug.i:6798:1: error: previous section type:
> WRITE|NOTYPE|DECLARED|NAMED
>  6798 | }
>       | ^
> /tmp/bug.i:6798:1: error: new section type: WRITE|NOTYPE|NAMED|RELRO
> 
> I still need help with producing a non-error, non-anchored
> diagnostic.
> I don't know how to do that.

I'm not quite sure what you mean by "non-error" and "non-anchored". 

By "non-error", do you mean that this should this be a warning?  If so,
use warning_at.  You can use 0 for the option_id whilst prototyping. 
Or use "inform" to get a note.

By "non-anchored", do you mean "not associated with any particular
source location"?  If so, use error_at/warning_at and use
UNKNOWN_LOCATION for the location_t ("error" and "warning" implicitly
use the "input_location" global variable; it's usually best to instead
specify a location, or use UNKNOWN_LOCATION for "global" problems).

Or am I misunderstanding?

Hope this is helpful
Dave




> 
> Thanks,
> Florian
> 
> 	* varasm.cc (section_flags_to_string):  New function.
> 	(get_section): Include name of section in diagnostics.
> 	Print old and new section flags, as rendered by
> 	section_flags_to_string.
> 
> ---
>  gcc/varasm.cc | 80
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 76 insertions(+), 4 deletions(-)
> 
> diff --git a/gcc/varasm.cc b/gcc/varasm.cc
> index 4426e7ce6c6..deba15933aa 100644
> --- a/gcc/varasm.cc
> +++ b/gcc/varasm.cc
> @@ -25,6 +25,7 @@ along with GCC; see the file COPYING3.  If not see
>     We also output the assembler code for constants stored in memory
>     and are responsible for combining constants with the same value. 
> */
>  
> +#define INCLUDE_STRING
>  #include "config.h"
>  #include "system.h"
>  #include "coretypes.h"
> @@ -276,6 +277,65 @@ get_noswitch_section (unsigned int flags,
> noswitch_section_callback callback)
>    return sect;
>  }
>  
> +/* Return a string describing the section flags.  */
> +
> +static std::string
> +section_flags_to_string (unsigned int flags)
> +{
> +  if (flags == 0)
> +    return "UNNAMED";
> +  std::string result;
> +  auto append = [&result] (bool f, const char *name)
> +  {
> +    if (f)
> +      {
> +	if (!result.empty ())
> +	  result += '|';
> +	result += name;
> +      }
> +  };
> +
> +  append (flags & SECTION_CODE, "CODE");
> +  append (flags & SECTION_WRITE, "WRITE");
> +  append (flags & SECTION_DEBUG, "DEBUG");
> +  append (flags & SECTION_LINKONCE, "LINKONCE");
> +  append (flags & SECTION_SMALL, "SMALL");
> +  append (flags & SECTION_BSS, "BSS");
> +  append (flags & SECTION_MERGE, "MERGE");
> +  append (flags & SECTION_STRINGS, "STRINGS");
> +  append (flags & SECTION_OVERRIDE, "OVERRIDE");
> +  append (flags & SECTION_TLS, "TLS");
> +  append (flags & SECTION_NOTYPE, "NOTYPE");
> +  append (flags & SECTION_DECLARED, "DECLARED");
> +  append (flags & SECTION_NAMED, "NAMED");
> +  append (flags & SECTION_NOSWITCH, "NOSWITCH");
> +  append (flags & SECTION_COMMON, "COMMON");
> +  append (flags & SECTION_RELRO, "RELRO");
> +  append (flags & SECTION_EXCLUDE, "EXCLUDE");
> +  append (flags & SECTION_RETAIN, "RETAIN");
> +  append (flags & SECTION_LINK_ORDER, "LINK_ORDER");
> +
> +  unsigned int entsize = flags & SECTION_ENTSIZE;
> +  if (entsize != 0)
> +    {
> +      if (!result.empty ())
> +	result += ',';
> +      result += "size=";
> +      result += std::to_string (entsize);
> +    }
> +
> +  unsigned int mach_dep = flags / SECTION_MACH_DEP;
> +  if (mach_dep != 0)
> +    {
> +      if (!result.empty ())
> +	result += ',';
> +      result += "mach=";
> +      result += std::to_string (mach_dep);
> +    }
> +
> +  return result;
> +}
> +
>  /* Return the named section structure associated with NAME.  Create
>     a new section with the given fields if no such structure exists.
>     When NOT_EXISTING, then fail if the section already exists. 
> Return
> @@ -312,6 +372,9 @@ get_section (const char *name, unsigned int
> flags, tree decl,
>  	internal_error ("section already exists: %qs", name);
>  
>        sect = *slot;
> +      unsigned int original_flags = sect->common.flags;
> +      unsigned int new_flags = flags;
> +
>        /* It is fine if one of the sections has SECTION_NOTYPE as
> long as
>           the other has none of the contrary flags (see the logic at
> the end
>           of default_section_type_flags, below).  */
> @@ -355,17 +418,26 @@ get_section (const char *name, unsigned int
> flags, tree decl,
>  	      && decl != sect->named.decl)
>  	    {
>  	      if (decl != NULL && DECL_P (decl))
> -		error ("%+qD causes a section type conflict with
> %qD",
> -		       decl, sect->named.decl);
> +		{
> +		  error ("%+qD causes a type conflict with %qD"
> +			 " in section %qs",
> +			 decl, sect->named.decl);
> +		}
>  	      else
> -		error ("section type conflict with %qD", sect-
> >named.decl);
> +		error ("section %qs type conflict with %qD",
> +		       name, sect->named.decl);
>  	      inform (DECL_SOURCE_LOCATION (sect->named.decl),
>  		      "%qD was declared here", sect->named.decl);
>  	    }
>  	  else if (decl != NULL && DECL_P (decl))
> -	    error ("%+qD causes a section type conflict", decl);
> +	    error ("%+qD causes a section type conflict for section
> %s",
> +		   decl, name);
>  	  else
>  	    error ("section type conflict");
> +	  error ("previous section type: %s",
> +		 section_flags_to_string (original_flags).c_str ());
> +	  error ("new section type: %s",
> +		 section_flags_to_string (new_flags).c_str ());
>  	  /* Make sure we don't error about one section multiple
> times.  */
>  	  sect->common.flags |= SECTION_OVERRIDE;
>  	}
> 
> base-commit: 786773d4c12fd78e94f58193ff303cba19ca1b19
>
  

On Sun, Sep 29, 2024 at 5:13 PM Florian Weimer <fweimer@redhat.com> wrote:
>
> Sometimes this is a user error, sometimes it is more of an ICE.
> In either case, more information about the conflict is helpful.
>
> I used to this to get a better idea about what is going on with
> PR116887.  The original diagnostics look like this:
>
> dl-find_object.c: In function ‘_dlfo_process_initial’:
> dl-find_object.c:507:1: error: section type conflict with ‘_dlfo_nodelete_mappings’
>   507 | _dlfo_process_initial (void)
>       | ^~~~~~~~~~~~~~~~~~~~~
> dl-find_object.c:73:40: note: ‘_dlfo_nodelete_mappings’ was declared here
>    73 | static struct dl_find_object_internal *_dlfo_nodelete_mappings
>       |                                        ^~~~~~~~~~~~~~~~~~~~~~~
>
>
> I don't quite understand what is going on (the symbol that's being
> flagged for conflict is somewhat unstable), but at least the new
> diagnostics show that the sectio name, and maybe the flags are useful,
> too:
>
> /tmp/bug.i:6798:1: error: section ‘.data.rel.ro’ type conflict with ‘_dlfo_main’
>  6798 | }
>       | ^
> /tmp/bug.i:6190:39: note: ‘_dlfo_main’ was declared here
>  6190 | static struct dl_find_object_internal _dlfo_main __attribute__ ((section (".data.rel.ro")));
>       |                                       ^~~~~~~~~~
> /tmp/bug.i:6798:1: error: previous section type: WRITE|NOTYPE|DECLARED|NAMED
>  6798 | }
>       | ^
> /tmp/bug.i:6798:1: error: new section type: WRITE|NOTYPE|NAMED|RELRO
>
> I still need help with producing a non-error, non-anchored diagnostic.
> I don't know how to do that.
>
> Thanks,
> Florian
>
>         * varasm.cc (section_flags_to_string):  New function.
>         (get_section): Include name of section in diagnostics.
>         Print old and new section flags, as rendered by
>         section_flags_to_string.
>
> ---
>  gcc/varasm.cc | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 76 insertions(+), 4 deletions(-)
>
> diff --git a/gcc/varasm.cc b/gcc/varasm.cc
> index 4426e7ce6c6..deba15933aa 100644
> --- a/gcc/varasm.cc
> +++ b/gcc/varasm.cc
> @@ -25,6 +25,7 @@ along with GCC; see the file COPYING3.  If not see
>     We also output the assembler code for constants stored in memory
>     and are responsible for combining constants with the same value.  */
>
> +#define INCLUDE_STRING
>  #include "config.h"
>  #include "system.h"
>  #include "coretypes.h"
> @@ -276,6 +277,65 @@ get_noswitch_section (unsigned int flags, noswitch_section_callback callback)
>    return sect;
>  }
>
> +/* Return a string describing the section flags.  */
> +
> +static std::string
> +section_flags_to_string (unsigned int flags)
> +{
> +  if (flags == 0)
> +    return "UNNAMED";
> +  std::string result;
> +  auto append = [&result] (bool f, const char *name)
> +  {
> +    if (f)
> +      {
> +       if (!result.empty ())
> +         result += '|';
> +       result += name;
> +      }
> +  };
> +
> +  append (flags & SECTION_CODE, "CODE");
> +  append (flags & SECTION_WRITE, "WRITE");
> +  append (flags & SECTION_DEBUG, "DEBUG");
> +  append (flags & SECTION_LINKONCE, "LINKONCE");
> +  append (flags & SECTION_SMALL, "SMALL");
> +  append (flags & SECTION_BSS, "BSS");
> +  append (flags & SECTION_MERGE, "MERGE");
> +  append (flags & SECTION_STRINGS, "STRINGS");
> +  append (flags & SECTION_OVERRIDE, "OVERRIDE");
> +  append (flags & SECTION_TLS, "TLS");
> +  append (flags & SECTION_NOTYPE, "NOTYPE");
> +  append (flags & SECTION_DECLARED, "DECLARED");
> +  append (flags & SECTION_NAMED, "NAMED");
> +  append (flags & SECTION_NOSWITCH, "NOSWITCH");
> +  append (flags & SECTION_COMMON, "COMMON");
> +  append (flags & SECTION_RELRO, "RELRO");
> +  append (flags & SECTION_EXCLUDE, "EXCLUDE");
> +  append (flags & SECTION_RETAIN, "RETAIN");
> +  append (flags & SECTION_LINK_ORDER, "LINK_ORDER");

I'm not sure printing these internal flags is of help to the user.

> +
> +  unsigned int entsize = flags & SECTION_ENTSIZE;
> +  if (entsize != 0)
> +    {
> +      if (!result.empty ())
> +       result += ',';
> +      result += "size=";
> +      result += std::to_string (entsize);
> +    }
> +
> +  unsigned int mach_dep = flags / SECTION_MACH_DEP;
> +  if (mach_dep != 0)
> +    {
> +      if (!result.empty ())
> +       result += ',';
> +      result += "mach=";
> +      result += std::to_string (mach_dep);
> +    }
> +
> +  return result;
> +}
> +
>  /* Return the named section structure associated with NAME.  Create
>     a new section with the given fields if no such structure exists.
>     When NOT_EXISTING, then fail if the section already exists.  Return
> @@ -312,6 +372,9 @@ get_section (const char *name, unsigned int flags, tree decl,
>         internal_error ("section already exists: %qs", name);
>
>        sect = *slot;
> +      unsigned int original_flags = sect->common.flags;
> +      unsigned int new_flags = flags;
> +
>        /* It is fine if one of the sections has SECTION_NOTYPE as long as
>           the other has none of the contrary flags (see the logic at the end
>           of default_section_type_flags, below).  */
> @@ -355,17 +418,26 @@ get_section (const char *name, unsigned int flags, tree decl,
>               && decl != sect->named.decl)
>             {
>               if (decl != NULL && DECL_P (decl))
> -               error ("%+qD causes a section type conflict with %qD",
> -                      decl, sect->named.decl);
> +               {
> +                 error ("%+qD causes a type conflict with %qD"
> +                        " in section %qs",
> +                        decl, sect->named.decl);
> +               }
>               else
> -               error ("section type conflict with %qD", sect->named.decl);
> +               error ("section %qs type conflict with %qD",
> +                      name, sect->named.decl);
>               inform (DECL_SOURCE_LOCATION (sect->named.decl),
>                       "%qD was declared here", sect->named.decl);
>             }
>           else if (decl != NULL && DECL_P (decl))
> -           error ("%+qD causes a section type conflict", decl);
> +           error ("%+qD causes a section type conflict for section %s",
> +                  decl, name);
>           else
>             error ("section type conflict");
> +         error ("previous section type: %s",
> +                section_flags_to_string (original_flags).c_str ());
> +         error ("new section type: %s",
> +                section_flags_to_string (new_flags).c_str ());

So these are all cases where neither

          /* It is fine if one of the section flags is
             SECTION_WRITE | SECTION_RELRO and the other has none of these
             flags (i.e. read-only) in named sections and either the
             section hasn't been declared yet or has been declared as writable.
             In that case just make sure the resulting flags are
             SECTION_WRITE | SECTION_RELRO, ie. writable only because of
             relocations.  */
          if (((sect->common.flags ^ flags) & (SECTION_WRITE | SECTION_RELRO))
              == (SECTION_WRITE | SECTION_RELRO)
              && (sect->common.flags
                  & ~(SECTION_DECLARED | SECTION_WRITE | SECTION_RELRO))
                 == (flags & ~(SECTION_WRITE | SECTION_RELRO))
              && ((sect->common.flags & SECTION_DECLARED) == 0
                  || (sect->common.flags & SECTION_WRITE)))
            {
              sect->common.flags |= (SECTION_WRITE | SECTION_RELRO);
              return sect;
            }
          /* If the SECTION_RETAIN bit doesn't match, return and switch
             to a new section later.  */
          if ((sect->common.flags & SECTION_RETAIN)
              != (flags & SECTION_RETAIN))
            return sect;

matched.  It should be possible to elaborate on the actual mismatch instead of
dumping all of the random section flags?

>           /* Make sure we don't error about one section multiple times.  */
>           sect->common.flags |= SECTION_OVERRIDE;
>         }
>
> base-commit: 786773d4c12fd78e94f58193ff303cba19ca1b19
>
  

* Richard Biener:

>> +  append (flags & SECTION_RELRO, "RELRO");
>> +  append (flags & SECTION_EXCLUDE, "EXCLUDE");
>> +  append (flags & SECTION_RETAIN, "RETAIN");
>> +  append (flags & SECTION_LINK_ORDER, "LINK_ORDER");
>
> I'm not sure printing these internal flags is of help to the user.

There are cases where at least one of the conflicting sections is
internally created.  I came through this via PR116887.  In those cases,
the diagnostic is more of an ICE than a programmer error.

> So these are all cases where neither
>
>           /* It is fine if one of the section flags is
>              SECTION_WRITE | SECTION_RELRO and the other has none of these
>              flags (i.e. read-only) in named sections and either the
>              section hasn't been declared yet or has been declared as writable.
>              In that case just make sure the resulting flags are
>              SECTION_WRITE | SECTION_RELRO, ie. writable only because of
>              relocations.  */
>           if (((sect->common.flags ^ flags) & (SECTION_WRITE | SECTION_RELRO))
>               == (SECTION_WRITE | SECTION_RELRO)
>               && (sect->common.flags
>                   & ~(SECTION_DECLARED | SECTION_WRITE | SECTION_RELRO))
>                  == (flags & ~(SECTION_WRITE | SECTION_RELRO))
>               && ((sect->common.flags & SECTION_DECLARED) == 0
>                   || (sect->common.flags & SECTION_WRITE)))
>             {
>               sect->common.flags |= (SECTION_WRITE | SECTION_RELRO);
>               return sect;
>             }
>           /* If the SECTION_RETAIN bit doesn't match, return and switch
>              to a new section later.  */
>           if ((sect->common.flags & SECTION_RETAIN)
>               != (flags & SECTION_RETAIN))
>             return sect;
>
> matched.  It should be possible to elaborate on the actual mismatch instead of
> dumping all of the random section flags?

Hmm.  This part

	      && (sect->common.flags
		  & ~(SECTION_DECLARED | SECTION_WRITE | SECTION_RELRO))
		 == (flags & ~(SECTION_WRITE | SECTION_RELRO))

seems to restrict the early return to the case where the internal flags
match.  So I think we should print them in the general case.  I could
special-case the LoongArch error (“attempt to mark an existing section
as RELRO”), but we can't say *what* causes this section type change
attempt in that particular case (it seems to come from constant pool
generation), so it's not going to be useful to programmers either way.

Thanks,
Florian
  

* David Malcolm:

> I'm not quite sure what you mean by "non-error" and "non-anchored". 

Sorry, I'm not familiar with the appropriate terminology.

> By "non-error", do you mean that this should this be a warning?  If so,
> use warning_at.  You can use 0 for the option_id whilst prototyping. 
> Or use "inform" to get a note.

I meant that I want to attach something like a note to another error.
There is just one error here (an ICE really, in case of PR116887).  If
it's okay I can call the error function without a location multiple
times to provide the additional information.  But the thing the
alternative below seems more appropriate.

> By "non-anchored", do you mean "not associated with any particular
> source location"?  If so, use error_at/warning_at and use
> UNKNOWN_LOCATION for the location_t ("error" and "warning" implicitly
> use the "input_location" global variable; it's usually best to instead
> specify a location, or use UNKNOWN_LOCATION for "global" problems).

Ahh, so use inform (UNKNOWN_LOCATION, …)?  I see a couple of examples
like that.

I'll wait for further comments from Richi and repost.  There are also
some test cases that need adjusting.

Thanks,
Florian
  

On Mon, 2024-09-30 at 09:33 +0200, Florian Weimer wrote:
> * David Malcolm:
> 
> > I'm not quite sure what you mean by "non-error" and "non-
> > anchored". 
> 
> Sorry, I'm not familiar with the appropriate terminology.
> 
> > By "non-error", do you mean that this should this be a warning?  If
> > so,
> > use warning_at.  You can use 0 for the option_id whilst
> > prototyping. 
> > Or use "inform" to get a note.
> 
> I meant that I want to attach something like a note to another error.
> There is just one error here (an ICE really, in case of PR116887). 
> If
> it's okay I can call the error function without a location multiple
> times to provide the additional information.  But the thing the
> alternative below seems more appropriate.

To attach notes to another diagnostic, use auto_diagnostic_group, a
RAII class that, during its lifetime, put all diagnostics into a group:

   {
     auto_diagnostic_group d;
     error_at (somewhere, "can't find %qs", "foo");
     inform (somewhere_else, "here's where I last remember seeing it");
    }

The effect isn't visible on the standard text output format, but is in
SARIF.

> 
> > By "non-anchored", do you mean "not associated with any particular
> > source location"?  If so, use error_at/warning_at and use
> > UNKNOWN_LOCATION for the location_t ("error" and "warning"
> > implicitly
> > use the "input_location" global variable; it's usually best to
> > instead
> > specify a location, or use UNKNOWN_LOCATION for "global" problems).
> 
> Ahh, so use inform (UNKNOWN_LOCATION, …)?  I see a couple of examples
> like that.

Yes.

Using UNKNOWN_LOCATION will lead to output like:

  cc1: note: message goes here

which might be appropriate, but can be unhelpful to the user for
tracking down the problem.  Is there no source location that's
relevant?

I got the impression from Richi's comments that this might be more of a
GCC developer thing rather than an end-user thing, so perhaps using the
dumpfile might be more appropriate?  (I'm not sure)


Dave

> 
> I'll wait for further comments from Richi and repost.  There are also
> some test cases that need adjusting.
> 
> Thanks,
> Florian
>