Re: [PING^3] Generalize 'gcc/input.h:struct location_hash' (was: [Committed] [PATCH 2/4] (v4) On-demand locations within string-literals)

[Thomas Schwinge]

Hi!

On 2021-10-17T16:33:03-0600, Jeff Law via Gcc-patches  
wrote:
> On 9/30/2021 12:47 AM, Thomas Schwinge wrote:
>> On 2021-09-17T13:16:14+0200, I wrote:
>>> On 2021-09-10T09:48:56+0200, I wrote:
 On 2021-09-03T18:33:37+0200, I wrote:
> On 2021-09-02T21:09:54+0200, I wrote:
>> On 2021-09-02T15:59:14+0200, I wrote:
>>> On 2016-08-05T14:16:58-0400, David Malcolm  wrote:
 Committed to trunk as r239175; I'm attaching the final version of the
 patch for reference.
>>> David, you've added here 'gcc/input.h:struct location_hash' (see quoted
>>> below), which will be useful elsewhere, so:
 --- a/gcc/input.h
 +++ b/gcc/input.h
 +struct location_hash : int_hash  { };
 +
 +class GTY(()) string_concat_db
 +{
 +[...]
 +  hash_map  *m_table;
 +};
>>> OK to push the attached
>>> "Generalize 'gcc/input.h:struct location_hash'"?

> OK

Thanks.  With the commentary slightly updated for PR103157 "'gengtype':
'typedef' causing infinite-recursion code to be generated", I've pushed
to master branch commit 088199e5d0fc0d54f48af0783a2630a773bbb387
"Generalize 'gcc/input.h:struct location_hash'", see attached.


Grüße
 Thomas


-
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 
München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas 
Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht 
München, HRB 106955
>From 088199e5d0fc0d54f48af0783a2630a773bbb387 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge 
Date: Tue, 31 Aug 2021 23:30:25 +0200
Subject: [PATCH] Generalize 'gcc/input.h:struct location_hash'

This is currently only used here ('gcc/input.h:class string_concat_db'), but is
actually generally useful, so advertize it as such.

Per the rationale given, we may use 'BUILTINS_LOCATION' as spare value for
'Deleted', in addition to the existing use of 'UNKNOWN_LOCATION' as spare value
for 'Empty'.

	gcc/
	* input.h (location_hash): Use 'BUILTINS_LOCATION' as spare value
	for 'Deleted'.  Turn into a '#define'.
---
 gcc/input.h | 24 ++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/gcc/input.h b/gcc/input.h
index f7b08bdc444..bc44ba2507f 100644
--- a/gcc/input.h
+++ b/gcc/input.h
@@ -36,6 +36,28 @@ extern GTY(()) class line_maps *saved_line_table;
both UNKNOWN_LOCATION and BUILTINS_LOCATION fit into that.  */
 STATIC_ASSERT (BUILTINS_LOCATION < RESERVED_LOCATION_COUNT);
 
+/* Hasher for 'location_t' values satisfying '!RESERVED_LOCATION_P', thus able
+   to use 'UNKNOWN_LOCATION'/'BUILTINS_LOCATION' as spare values for
+   'Empty'/'Deleted'.  */
+/* Per PR103157 "'gengtype': 'typedef' causing infinite-recursion code to be
+   generated", don't use
+   typedef int_hash
+ location_hash;
+   here.
+
+   It works for a single-use case, but when using a 'struct'-based variant
+   struct location_hash
+ : int_hash {};
+   in more than one place, 'gengtype' generates duplicate functions (thus:
+   "error: redefinition of 'void gt_ggc_mx(location_hash&)'" etc.).
+   Attempting to mark that one up with GTY options, we run into a 'gengtype'
+   "parse error: expected '{', have '<'", which probably falls into category
+   "understanding of C++ is limited", as documented in 'gcc/doc/gty.texi'.
+
+   Thus, use a plain ol' '#define':
+*/
+#define location_hash int_hash
+
 extern bool is_location_from_builtin_token (location_t);
 extern expanded_location expand_location (location_t);
 
@@ -233,8 +255,6 @@ public:
   location_t * GTY ((atomic)) m_locs;
 };
 
-struct location_hash : int_hash  { };
-
 class GTY(()) string_concat_db
 {
  public:
-- 
2.33.0

Re: [PING^3] Generalize 'gcc/input.h:struct location_hash' (was: [Committed] [PATCH 2/4] (v4) On-demand locations within string-literals)

[Jeff Law via Gcc-patches]

On 9/30/2021 12:47 AM, Thomas Schwinge wrote:

Hi!

On 2021-09-17T13:16:14+0200, I wrote:

On 2021-09-10T09:48:56+0200, I wrote:

Ping.  My patches again attached, for easy reference.

Ping once again.

Jeff had ACKed "Don't record string concatenation data for
'RESERVED_LOCATION_P'" (thanks!), but "Generalize 'gcc/input.h:struct
location_hash'" is still awaiting review:


On 2021-09-03T18:33:37+0200, I wrote:

On 2021-09-02T21:09:54+0200, I wrote:

On 2021-09-02T15:59:14+0200, I wrote:

On 2016-08-05T14:16:58-0400, David Malcolm  wrote:

Committed to trunk as r239175; I'm attaching the final version of the
patch for reference.

David, you've added here 'gcc/input.h:struct location_hash' (see quoted
below), which will be useful elsewhere, so:

--- a/gcc/input.h
+++ b/gcc/input.h
+struct location_hash : int_hash  { };
+
+class GTY(()) string_concat_db
+{
+[...]
+  hash_map  *m_table;
+};

OK to push the attached
"Generalize 'gcc/input.h:struct location_hash'"?

Attached again, for easy reference.


Grüße
  Thomas


-
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 
München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas 
Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht 
München, HRB 106955

0002-Generalize-gcc-input.h-struct-location_hash.patch

 From 349a3172f64db93ee98ea39b36489b702b6596ab Mon Sep 17 00:00:00 2001
From: Thomas Schwinge 
Date: Tue, 31 Aug 2021 23:30:25 +0200
Subject: [PATCH 2/2] Generalize 'gcc/input.h:struct location_hash'

This is currently only used here ('gcc/input.h:class string_concat_db'), but is
actually generally useful, so advertize it as such.

Per the rationale given, we may use 'BUILTINS_LOCATION' as spare value for
'Deleted', in addition to the existing use of 'UNKNOWN_LOCATION' as spare value
for 'Empty'.

gcc/
* input.h (location_hash): Use 'BUILTINS_LOCATION' as spare value
for 'Deleted'.  Turn into a '#define'.

OK
jeff

[PING^3] Generalize 'gcc/input.h:struct location_hash' (was: [Committed] [PATCH 2/4] (v4) On-demand locations within string-literals)

[Thomas Schwinge]

Hi!

On 2021-09-17T13:16:14+0200, I wrote:
> On 2021-09-10T09:48:56+0200, I wrote:
>> Ping.  My patches again attached, for easy reference.
>
> Ping once again.

Jeff had ACKed "Don't record string concatenation data for
'RESERVED_LOCATION_P'" (thanks!), but "Generalize 'gcc/input.h:struct
location_hash'" is still awaiting review:

>> On 2021-09-03T18:33:37+0200, I wrote:
>>> On 2021-09-02T21:09:54+0200, I wrote:
 On 2021-09-02T15:59:14+0200, I wrote:
> On 2016-08-05T14:16:58-0400, David Malcolm  wrote:
>> Committed to trunk as r239175; I'm attaching the final version of the
>> patch for reference.
>
> David, you've added here 'gcc/input.h:struct location_hash' (see quoted
> below), which will be useful elsewhere, so:

>> --- a/gcc/input.h
>> +++ b/gcc/input.h
>
>> +struct location_hash : int_hash  { };
>> +
>> +class GTY(()) string_concat_db
>> +{
>> +[...]
>> +  hash_map  *m_table;
>> +};
>
> OK to push the attached
> "Generalize 'gcc/input.h:struct location_hash'"?

Attached again, for easy reference.


Grüße
 Thomas


-
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 
München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas 
Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht 
München, HRB 106955
>From 349a3172f64db93ee98ea39b36489b702b6596ab Mon Sep 17 00:00:00 2001
From: Thomas Schwinge 
Date: Tue, 31 Aug 2021 23:30:25 +0200
Subject: [PATCH 2/2] Generalize 'gcc/input.h:struct location_hash'

This is currently only used here ('gcc/input.h:class string_concat_db'), but is
actually generally useful, so advertize it as such.

Per the rationale given, we may use 'BUILTINS_LOCATION' as spare value for
'Deleted', in addition to the existing use of 'UNKNOWN_LOCATION' as spare value
for 'Empty'.

	gcc/
	* input.h (location_hash): Use 'BUILTINS_LOCATION' as spare value
	for 'Deleted'.  Turn into a '#define'.
---
 gcc/input.h | 21 +++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/gcc/input.h b/gcc/input.h
index e6881072c5f..46971a2684c 100644
--- a/gcc/input.h
+++ b/gcc/input.h
@@ -36,6 +36,25 @@ extern GTY(()) class line_maps *saved_line_table;
both UNKNOWN_LOCATION and BUILTINS_LOCATION fit into that.  */
 STATIC_ASSERT (BUILTINS_LOCATION < RESERVED_LOCATION_COUNT);
 
+/* Hasher for 'location_t' values satisfying '!RESERVED_LOCATION_P', thus able
+   to use 'UNKNOWN_LOCATION'/'BUILTINS_LOCATION' as spare values for
+   'Empty'/'Deleted'.  */
+/* If the following is used more than once, 'gengtype' generates duplicate
+   functions (thus: "error: redefinition of 'void gt_ggc_mx(location_hash&)'"
+   etc.):
+
+   struct location_hash
+ : int_hash {};
+
+   Likewise for this:
+
+   typedef int_hash
+ location_hash;
+
+   Thus, use a plain ol' '#define':
+*/
+#define location_hash int_hash
+
 extern bool is_location_from_builtin_token (location_t);
 extern expanded_location expand_location (location_t);
 
@@ -230,8 +249,6 @@ public:
   location_t * GTY ((atomic)) m_locs;
 };
 
-struct location_hash : int_hash  { };
-
 class GTY(()) string_concat_db
 {
  public:
-- 
2.33.0

Re: [PING] Re: [Committed] [PATCH 2/4] (v4) On-demand locations within string-literals

[Jeff Law via Gcc-patches]

On 9/10/2021 1:48 AM, Thomas Schwinge wrote:

Hi!

Ping.  My patches again attached, for easy reference.


Grüße
  Thomas


On 2021-09-03T18:33:37+0200, I wrote:

Hi!

On 2021-09-02T21:09:54+0200, I wrote:

On 2021-09-02T15:59:14+0200, I wrote:

On 2016-08-05T14:16:58-0400, David Malcolm  wrote:

Committed to trunk as r239175; I'm attaching the final version of the
patch for reference.

David, you've added here 'gcc/input.h:struct location_hash' (see quoted
below), which will be useful elsewhere, so:


--- a/gcc/input.c
+++ b/gcc/input.c
+/* Internal function.  Canonicalize LOC into a form suitable for
+   use as a key within the database, stripping away macro expansion,
+   ad-hoc information, and range information, using the location of
+   the start of LOC within an ordinary linemap.  */
+
+location_t
+string_concat_db::get_key_loc (location_t loc)
+{
+  loc = linemap_resolve_location (line_table, loc, LRK_SPELLING_LOCATION,
+ NULL);
+
+  loc = get_range_from_loc (line_table, loc).m_start;
+
+  return loc;
+}

OK to push the attached
"Harden 'gcc/input.c:string_concat_db::get_key_loc'"?  (This fell out of
my analysis for development work elsewhere.)

My suggested patch was:

 --- a/gcc/input.c
 +++ b/gcc/input.c
 @@ -1483,6 +1483,9 @@ string_concat_db::get_key_loc (location_t loc)

loc = get_range_from_loc (line_table, loc).m_start;

 +  /* Ascertain that 'loc' is valid as a key in 'm_table'.  */
 +  gcc_checking_assert (!RESERVED_LOCATION_P (loc));
 +
return loc;
  }

Uh, I should've looked at the correct test logs...  This change actually
does regress 'c-c++-common/substring-location-PR-87721.c' and
'gcc.dg/plugin/diagnostic-test-string-literals-1.c': for these, we do see
'BUILTINS_LOCATION' (via 'string_concat_db::record_string_concatenation').
Unless someone tell me that's unexpected (I'm completely lost in this
code...)

I think I convinced myself that the current code doesn't have stable
behavior, so...


I shall change/generalize my changes to provide both a
'location_hash' only using 'UNKNOWN_LOCATION' as a spare value for
'Empty' (as currently used here) and another variant additionally using
'BUILTINS_LOCATION' as spare value for 'Deleted'.

... I didn't do this, but instead would like to push the attached
"Don't record string concatenation data for 'RESERVED_LOCATION_P'"
(replacing "Harden 'gcc/input.c:string_concat_db::get_key_loc'" as
originally proposed).  OK?


... and then re:


--- a/gcc/input.h
+++ b/gcc/input.h
+struct location_hash : int_hash  { };
+
+class GTY(()) string_concat_db
+{
+[...]
+  hash_map  *m_table;
+};

OK to push the attached
"Generalize 'gcc/input.h:struct location_hash'"?

Attached again.


Grüße
  Thomas


-
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 
München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas 
Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht 
München, HRB 106955

0001-Don-t-record-string-concatenation-data-for-RESERVED_.patch

 From 9f1066fcb770397d6e791aa0594f067a755e2ed6 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge 
Date: Fri, 3 Sep 2021 18:25:10 +0200
Subject: [PATCH] Don't record string concatenation data for
  'RESERVED_LOCATION_P'

'RESERVED_LOCATION_P' means 'UNKNOWN_LOCATION' or 'BUILTINS_LOCATION.
We're using 'UNKNOWN_LOCATION' as a spare value for 'Empty', so should
ascertain that we don't use it as a key additionally.  Similarly for
'BUILTINS_LOCATION' that we'd later like to use as a spare value for
'Deleted'.

As discussed in the source code comment added, for these we didn't have
stable behavior anyway.

Follow-up to r239175 (commit 88faa309e5d6c6171b957daaf2f800920869)
"On-demand locations within string-literals".

gcc/
* input.c (string_concat_db::record_string_concatenation)
(string_concat_db::get_string_concatenation): Skip for
'RESERVED_LOCATION_P'.
gcc/testsuite/
* gcc.dg/plugin/diagnostic-test-string-literals-1.c: Adjust
expected error diagnostics.

OK
jeff

[PING^2] Re: [Committed] [PATCH 2/4] (v4) On-demand locations within string-literals

[Thomas Schwinge]

Hi!

On 2021-09-10T09:48:56+0200, I wrote:
> Ping.  My patches again attached, for easy reference.

Ping once again.


Grüße
 Thomas


> On 2021-09-03T18:33:37+0200, I wrote:
>> Hi!
>>
>> On 2021-09-02T21:09:54+0200, I wrote:
>>> On 2021-09-02T15:59:14+0200, I wrote:
 On 2016-08-05T14:16:58-0400, David Malcolm  wrote:
> Committed to trunk as r239175; I'm attaching the final version of the
> patch for reference.

 David, you've added here 'gcc/input.h:struct location_hash' (see quoted
 below), which will be useful elsewhere, so:

> --- a/gcc/input.c
> +++ b/gcc/input.c

> +/* Internal function.  Canonicalize LOC into a form suitable for
> +   use as a key within the database, stripping away macro expansion,
> +   ad-hoc information, and range information, using the location of
> +   the start of LOC within an ordinary linemap.  */
> +
> +location_t
> +string_concat_db::get_key_loc (location_t loc)
> +{
> +  loc = linemap_resolve_location (line_table, loc, LRK_SPELLING_LOCATION,
> + NULL);
> +
> +  loc = get_range_from_loc (line_table, loc).m_start;
> +
> +  return loc;
> +}

 OK to push the attached
 "Harden 'gcc/input.c:string_concat_db::get_key_loc'"?  (This fell out of
 my analysis for development work elsewhere.)
>>>
>>> My suggested patch was:
>>>
>>> --- a/gcc/input.c
>>> +++ b/gcc/input.c
>>> @@ -1483,6 +1483,9 @@ string_concat_db::get_key_loc (location_t loc)
>>>
>>>loc = get_range_from_loc (line_table, loc).m_start;
>>>
>>> +  /* Ascertain that 'loc' is valid as a key in 'm_table'.  */
>>> +  gcc_checking_assert (!RESERVED_LOCATION_P (loc));
>>> +
>>>return loc;
>>>  }
>>>
>>> Uh, I should've looked at the correct test logs...  This change actually
>>> does regress 'c-c++-common/substring-location-PR-87721.c' and
>>> 'gcc.dg/plugin/diagnostic-test-string-literals-1.c': for these, we do see
>>> 'BUILTINS_LOCATION' (via 'string_concat_db::record_string_concatenation').
>>> Unless someone tell me that's unexpected (I'm completely lost in this
>>> code...)
>>
>> I think I convinced myself that the current code doesn't have stable
>> behavior, so...
>>
>>> I shall change/generalize my changes to provide both a
>>> 'location_hash' only using 'UNKNOWN_LOCATION' as a spare value for
>>> 'Empty' (as currently used here) and another variant additionally using
>>> 'BUILTINS_LOCATION' as spare value for 'Deleted'.
>>
>> ... I didn't do this, but instead would like to push the attached
>> "Don't record string concatenation data for 'RESERVED_LOCATION_P'"
>> (replacing "Harden 'gcc/input.c:string_concat_db::get_key_loc'" as
>> originally proposed).  OK?
>>
>>
>> ... and then re:
>>
> --- a/gcc/input.h
> +++ b/gcc/input.h

> +struct location_hash : int_hash  { };
> +
> +class GTY(()) string_concat_db
> +{
> +[...]
> +  hash_map  *m_table;
> +};

 OK to push the attached
 "Generalize 'gcc/input.h:struct location_hash'"?
>>
>> Attached again.
>>
>>
>> Grüße
>>  Thomas


-
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 
München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas 
Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht 
München, HRB 106955
>From 9f1066fcb770397d6e791aa0594f067a755e2ed6 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge 
Date: Fri, 3 Sep 2021 18:25:10 +0200
Subject: [PATCH] Don't record string concatenation data for
 'RESERVED_LOCATION_P'

'RESERVED_LOCATION_P' means 'UNKNOWN_LOCATION' or 'BUILTINS_LOCATION.
We're using 'UNKNOWN_LOCATION' as a spare value for 'Empty', so should
ascertain that we don't use it as a key additionally.  Similarly for
'BUILTINS_LOCATION' that we'd later like to use as a spare value for
'Deleted'.

As discussed in the source code comment added, for these we didn't have
stable behavior anyway.

Follow-up to r239175 (commit 88faa309e5d6c6171b957daaf2f800920869)
"On-demand locations within string-literals".

	gcc/
	* input.c (string_concat_db::record_string_concatenation)
	(string_concat_db::get_string_concatenation): Skip for
	'RESERVED_LOCATION_P'.
	gcc/testsuite/
	* gcc.dg/plugin/diagnostic-test-string-literals-1.c: Adjust
	expected error diagnostics.
---
 gcc/input.c  | 9 +
 .../gcc.dg/plugin/diagnostic-test-string-literals-1.c| 4 ++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/gcc/input.c b/gcc/input.c
index 4b809862e02..dd753decfa0 100644
--- a/gcc/input.c
+++ b/gcc/input.c
@@ -1437,6 +1437,11 @@ string_concat_db::record_string_concatenation (int num, location_t *locs)
   gcc_assert (locs);
 
   location_t key_loc = get_key_loc (locs[0]);
+  /* We don't record data for 'RESERVED_LOCATION_P (key_loc)' key values:
+ any data now recorded under key 'key_loc' would be 

[PING] Re: [Committed] [PATCH 2/4] (v4) On-demand locations within string-literals

[Thomas Schwinge]

Hi!

Ping.  My patches again attached, for easy reference.


Grüße
 Thomas


On 2021-09-03T18:33:37+0200, I wrote:
> Hi!
>
> On 2021-09-02T21:09:54+0200, I wrote:
>> On 2021-09-02T15:59:14+0200, I wrote:
>>> On 2016-08-05T14:16:58-0400, David Malcolm  wrote:
 Committed to trunk as r239175; I'm attaching the final version of the
 patch for reference.
>>>
>>> David, you've added here 'gcc/input.h:struct location_hash' (see quoted
>>> below), which will be useful elsewhere, so:
>>>
 --- a/gcc/input.c
 +++ b/gcc/input.c
>>>
 +/* Internal function.  Canonicalize LOC into a form suitable for
 +   use as a key within the database, stripping away macro expansion,
 +   ad-hoc information, and range information, using the location of
 +   the start of LOC within an ordinary linemap.  */
 +
 +location_t
 +string_concat_db::get_key_loc (location_t loc)
 +{
 +  loc = linemap_resolve_location (line_table, loc, LRK_SPELLING_LOCATION,
 + NULL);
 +
 +  loc = get_range_from_loc (line_table, loc).m_start;
 +
 +  return loc;
 +}
>>>
>>> OK to push the attached
>>> "Harden 'gcc/input.c:string_concat_db::get_key_loc'"?  (This fell out of
>>> my analysis for development work elsewhere.)
>>
>> My suggested patch was:
>>
>> --- a/gcc/input.c
>> +++ b/gcc/input.c
>> @@ -1483,6 +1483,9 @@ string_concat_db::get_key_loc (location_t loc)
>>
>>loc = get_range_from_loc (line_table, loc).m_start;
>>
>> +  /* Ascertain that 'loc' is valid as a key in 'm_table'.  */
>> +  gcc_checking_assert (!RESERVED_LOCATION_P (loc));
>> +
>>return loc;
>>  }
>>
>> Uh, I should've looked at the correct test logs...  This change actually
>> does regress 'c-c++-common/substring-location-PR-87721.c' and
>> 'gcc.dg/plugin/diagnostic-test-string-literals-1.c': for these, we do see
>> 'BUILTINS_LOCATION' (via 'string_concat_db::record_string_concatenation').
>> Unless someone tell me that's unexpected (I'm completely lost in this
>> code...)
>
> I think I convinced myself that the current code doesn't have stable
> behavior, so...
>
>> I shall change/generalize my changes to provide both a
>> 'location_hash' only using 'UNKNOWN_LOCATION' as a spare value for
>> 'Empty' (as currently used here) and another variant additionally using
>> 'BUILTINS_LOCATION' as spare value for 'Deleted'.
>
> ... I didn't do this, but instead would like to push the attached
> "Don't record string concatenation data for 'RESERVED_LOCATION_P'"
> (replacing "Harden 'gcc/input.c:string_concat_db::get_key_loc'" as
> originally proposed).  OK?
>
>
> ... and then re:
>
 --- a/gcc/input.h
 +++ b/gcc/input.h
>>>
 +struct location_hash : int_hash  { };
 +
 +class GTY(()) string_concat_db
 +{
 +[...]
 +  hash_map  *m_table;
 +};
>>>
>>> OK to push the attached
>>> "Generalize 'gcc/input.h:struct location_hash'"?
>
> Attached again.
>
>
> Grüße
>  Thomas


-
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 
München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas 
Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht 
München, HRB 106955
>From 9f1066fcb770397d6e791aa0594f067a755e2ed6 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge 
Date: Fri, 3 Sep 2021 18:25:10 +0200
Subject: [PATCH] Don't record string concatenation data for
 'RESERVED_LOCATION_P'

'RESERVED_LOCATION_P' means 'UNKNOWN_LOCATION' or 'BUILTINS_LOCATION.
We're using 'UNKNOWN_LOCATION' as a spare value for 'Empty', so should
ascertain that we don't use it as a key additionally.  Similarly for
'BUILTINS_LOCATION' that we'd later like to use as a spare value for
'Deleted'.

As discussed in the source code comment added, for these we didn't have
stable behavior anyway.

Follow-up to r239175 (commit 88faa309e5d6c6171b957daaf2f800920869)
"On-demand locations within string-literals".

	gcc/
	* input.c (string_concat_db::record_string_concatenation)
	(string_concat_db::get_string_concatenation): Skip for
	'RESERVED_LOCATION_P'.
	gcc/testsuite/
	* gcc.dg/plugin/diagnostic-test-string-literals-1.c: Adjust
	expected error diagnostics.
---
 gcc/input.c  | 9 +
 .../gcc.dg/plugin/diagnostic-test-string-literals-1.c| 4 ++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/gcc/input.c b/gcc/input.c
index 4b809862e02..dd753decfa0 100644
--- a/gcc/input.c
+++ b/gcc/input.c
@@ -1437,6 +1437,11 @@ string_concat_db::record_string_concatenation (int num, location_t *locs)
   gcc_assert (locs);
 
   location_t key_loc = get_key_loc (locs[0]);
+  /* We don't record data for 'RESERVED_LOCATION_P (key_loc)' key values:
+ any data now recorded under key 'key_loc' would be overwritten by a
+ subsequent call with the same key 'key_loc'.  */
+  if (RESERVED_LOCATION_P (key_loc))
+return;
 
   string_concat *concat
 

Re: [Committed] [PATCH 2/4] (v4) On-demand locations within string-literals

[Thomas Schwinge]

Hi!

On 2021-09-02T21:09:54+0200, I wrote:
> On 2021-09-02T15:59:14+0200, I wrote:
>> On 2016-08-05T14:16:58-0400, David Malcolm  wrote:
>>> Committed to trunk as r239175; I'm attaching the final version of the
>>> patch for reference.
>>
>> David, you've added here 'gcc/input.h:struct location_hash' (see quoted
>> below), which will be useful elsewhere, so:
>>
>>> --- a/gcc/input.c
>>> +++ b/gcc/input.c
>>
>>> +/* Internal function.  Canonicalize LOC into a form suitable for
>>> +   use as a key within the database, stripping away macro expansion,
>>> +   ad-hoc information, and range information, using the location of
>>> +   the start of LOC within an ordinary linemap.  */
>>> +
>>> +location_t
>>> +string_concat_db::get_key_loc (location_t loc)
>>> +{
>>> +  loc = linemap_resolve_location (line_table, loc, LRK_SPELLING_LOCATION,
>>> + NULL);
>>> +
>>> +  loc = get_range_from_loc (line_table, loc).m_start;
>>> +
>>> +  return loc;
>>> +}
>>
>> OK to push the attached
>> "Harden 'gcc/input.c:string_concat_db::get_key_loc'"?  (This fell out of
>> my analysis for development work elsewhere.)
>
> My suggested patch was:
>
> --- a/gcc/input.c
> +++ b/gcc/input.c
> @@ -1483,6 +1483,9 @@ string_concat_db::get_key_loc (location_t loc)
>
>loc = get_range_from_loc (line_table, loc).m_start;
>
> +  /* Ascertain that 'loc' is valid as a key in 'm_table'.  */
> +  gcc_checking_assert (!RESERVED_LOCATION_P (loc));
> +
>return loc;
>  }
>
> Uh, I should've looked at the correct test logs...  This change actually
> does regress 'c-c++-common/substring-location-PR-87721.c' and
> 'gcc.dg/plugin/diagnostic-test-string-literals-1.c': for these, we do see
> 'BUILTINS_LOCATION' (via 'string_concat_db::record_string_concatenation').
> Unless someone tell me that's unexpected (I'm completely lost in this
> code...)

I think I convinced myself that the current code doesn't have stable
behavior, so...

> I shall change/generalize my changes to provide both a
> 'location_hash' only using 'UNKNOWN_LOCATION' as a spare value for
> 'Empty' (as currently used here) and another variant additionally using
> 'BUILTINS_LOCATION' as spare value for 'Deleted'.

... I didn't do this, but instead would like to push the attached
"Don't record string concatenation data for 'RESERVED_LOCATION_P'"
(replacing "Harden 'gcc/input.c:string_concat_db::get_key_loc'" as
originally proposed).  OK?


... and then re:

>>> --- a/gcc/input.h
>>> +++ b/gcc/input.h
>>
>>> +struct location_hash : int_hash  { };
>>> +
>>> +class GTY(()) string_concat_db
>>> +{
>>> +[...]
>>> +  hash_map  *m_table;
>>> +};
>>
>> OK to push the attached
>> "Generalize 'gcc/input.h:struct location_hash'"?

Attached again.


Grüße
 Thomas


-
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 
München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas 
Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht 
München, HRB 106955
>From 9f1066fcb770397d6e791aa0594f067a755e2ed6 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge 
Date: Fri, 3 Sep 2021 18:25:10 +0200
Subject: [PATCH] Don't record string concatenation data for
 'RESERVED_LOCATION_P'

'RESERVED_LOCATION_P' means 'UNKNOWN_LOCATION' or 'BUILTINS_LOCATION.
We're using 'UNKNOWN_LOCATION' as a spare value for 'Empty', so should
ascertain that we don't use it as a key additionally.  Similarly for
'BUILTINS_LOCATION' that we'd later like to use as a spare value for
'Deleted'.

As discussed in the source code comment added, for these we didn't have
stable behavior anyway.

Follow-up to r239175 (commit 88faa309e5d6c6171b957daaf2f800920869)
"On-demand locations within string-literals".

	gcc/
	* input.c (string_concat_db::record_string_concatenation)
	(string_concat_db::get_string_concatenation): Skip for
	'RESERVED_LOCATION_P'.
	gcc/testsuite/
	* gcc.dg/plugin/diagnostic-test-string-literals-1.c: Adjust
	expected error diagnostics.
---
 gcc/input.c  | 9 +
 .../gcc.dg/plugin/diagnostic-test-string-literals-1.c| 4 ++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/gcc/input.c b/gcc/input.c
index 4b809862e02..dd753decfa0 100644
--- a/gcc/input.c
+++ b/gcc/input.c
@@ -1437,6 +1437,11 @@ string_concat_db::record_string_concatenation (int num, location_t *locs)
   gcc_assert (locs);
 
   location_t key_loc = get_key_loc (locs[0]);
+  /* We don't record data for 'RESERVED_LOCATION_P (key_loc)' key values:
+ any data now recorded under key 'key_loc' would be overwritten by a
+ subsequent call with the same key 'key_loc'.  */
+  if (RESERVED_LOCATION_P (key_loc))
+return;
 
   string_concat *concat
 = new (ggc_alloc  ()) string_concat (num, locs);
@@ -1460,6 +1465,10 @@ string_concat_db::get_string_concatenation (location_t loc,
   gcc_assert (out_locs);
 
   location_t key_loc = get_key_loc (loc);
+  /* We 

Re: [Committed] [PATCH 2/4] (v4) On-demand locations within string-literals

[Thomas Schwinge]

Hi!

On 2021-09-02T15:59:14+0200, I wrote:
> On 2016-08-05T14:16:58-0400, David Malcolm  wrote:
>> Committed to trunk as r239175; I'm attaching the final version of the
>> patch for reference.
>
> David, you've added here 'gcc/input.h:struct location_hash' (see quoted
> below), which will be useful elsewhere, so:
>
>> --- a/gcc/input.c
>> +++ b/gcc/input.c
>
>> +/* Internal function.  Canonicalize LOC into a form suitable for
>> +   use as a key within the database, stripping away macro expansion,
>> +   ad-hoc information, and range information, using the location of
>> +   the start of LOC within an ordinary linemap.  */
>> +
>> +location_t
>> +string_concat_db::get_key_loc (location_t loc)
>> +{
>> +  loc = linemap_resolve_location (line_table, loc, LRK_SPELLING_LOCATION,
>> +  NULL);
>> +
>> +  loc = get_range_from_loc (line_table, loc).m_start;
>> +
>> +  return loc;
>> +}
>
> OK to push the attached
> "Harden 'gcc/input.c:string_concat_db::get_key_loc'"?  (This fell out of
> my analysis for development work elsewhere.)

My suggested patch was:

--- a/gcc/input.c
+++ b/gcc/input.c
@@ -1483,6 +1483,9 @@ string_concat_db::get_key_loc (location_t loc)

   loc = get_range_from_loc (line_table, loc).m_start;

+  /* Ascertain that 'loc' is valid as a key in 'm_table'.  */
+  gcc_checking_assert (!RESERVED_LOCATION_P (loc));
+
   return loc;
 }

Uh, I should've looked at the correct test logs...  This change actually
does regress 'c-c++-common/substring-location-PR-87721.c' and
'gcc.dg/plugin/diagnostic-test-string-literals-1.c': for these, we do see
'BUILTINS_LOCATION' (via 'string_concat_db::record_string_concatenation').
Unless someone tell me that's unexpected (I'm completely lost in this
code...), I shall change/generalize my changes to provide both a
'location_hash' only using 'UNKNOWN_LOCATION' as a spare value for
'Empty' (as currently used here) and another variant additionally using
'BUILTINS_LOCATION' as spare value for 'Deleted'.


Grüße
 Thomas


>> --- a/gcc/input.h
>> +++ b/gcc/input.h
>
>> +struct location_hash : int_hash  { };
>> +
>> +class GTY(()) string_concat_db
>> +{
>> +[...]
>> +  hash_map  *m_table;
>> +};
>
> OK to push the attached
> "Generalize 'gcc/input.h:struct location_hash'"?

My suggested patch was:

> Subject: [PATCH 2/2] Generalize 'gcc/input.h:struct location_hash'
>
> This is currently only used here ('gcc/input.h:class string_concat_db'), but 
> is
> actually generally useful, so advertize it as such.
>
> Per the rationale given, we may use 'BUILTINS_LOCATION' as spare value for
> 'Deleted', in addition to the existing use of 'UNKNOWN_LOCATION' as spare 
> value
> for 'Empty'.
>
>   gcc/
>   * input.h (location_hash): Use 'BUILTINS_LOCATION' as spare value
>   for 'Deleted'.  Turn into a '#define'.
> ---
>  gcc/input.h | 21 +++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/gcc/input.h b/gcc/input.h
> index e6881072c5f..46971a2684c 100644
> --- a/gcc/input.h
> +++ b/gcc/input.h
> @@ -36,6 +36,25 @@ extern GTY(()) class line_maps *saved_line_table;
> both UNKNOWN_LOCATION and BUILTINS_LOCATION fit into that.  */
>  STATIC_ASSERT (BUILTINS_LOCATION < RESERVED_LOCATION_COUNT);
>
> +/* Hasher for 'location_t' values satisfying '!RESERVED_LOCATION_P', thus 
> able
> +   to use 'UNKNOWN_LOCATION'/'BUILTINS_LOCATION' as spare values for
> +   'Empty'/'Deleted'.  */
> +/* If the following is used more than once, 'gengtype' generates duplicate
> +   functions (thus: "error: redefinition of 'void gt_ggc_mx(location_hash&)'"
> +   etc.):
> +
> +   struct location_hash
> + : int_hash {};
> +
> +   Likewise for this:
> +
> +   typedef int_hash
> + location_hash;
> +
> +   Thus, use a plain ol' '#define':
> +*/
> +#define location_hash int_hash BUILTINS_LOCATION>
> +
>  extern bool is_location_from_builtin_token (location_t);
>  extern expanded_location expand_location (location_t);
>
> @@ -230,8 +249,6 @@ public:
>location_t * GTY ((atomic)) m_locs;
>  };
>
> -struct location_hash : int_hash  { };
> -
>  class GTY(()) string_concat_db
>  {
>   public:
-
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 
München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas 
Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht 
München, HRB 106955

Re: [Committed] [PATCH 2/4] (v4) On-demand locations within string-literals

[Thomas Schwinge]

Hi!

On 2016-08-05T14:16:58-0400, David Malcolm  wrote:
> Committed to trunk as r239175; I'm attaching the final version of the
> patch for reference.

David, you've added here 'gcc/input.h:struct location_hash' (see quoted
below), which will be useful elsewhere, so:

> --- a/gcc/input.c
> +++ b/gcc/input.c

> +/* Internal function.  Canonicalize LOC into a form suitable for
> +   use as a key within the database, stripping away macro expansion,
> +   ad-hoc information, and range information, using the location of
> +   the start of LOC within an ordinary linemap.  */
> +
> +location_t
> +string_concat_db::get_key_loc (location_t loc)
> +{
> +  loc = linemap_resolve_location (line_table, loc, LRK_SPELLING_LOCATION,
> +   NULL);
> +
> +  loc = get_range_from_loc (line_table, loc).m_start;
> +
> +  return loc;
> +}

OK to push the attached
"Harden 'gcc/input.c:string_concat_db::get_key_loc'"?  (This fell out of
my analysis for development work elsewhere.)

> --- a/gcc/input.h
> +++ b/gcc/input.h

> +struct location_hash : int_hash  { };
> +
> +class GTY(()) string_concat_db
> +{
> +[...]
> +  hash_map  *m_table;
> +};

OK to push the attached
"Generalize 'gcc/input.h:struct location_hash'"?


Grüße
 Thomas


-
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 
München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas 
Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht 
München, HRB 106955
>From 521c94471ae2f044f8cca8025bfa8db2d2936aea Mon Sep 17 00:00:00 2001
From: Thomas Schwinge 
Date: Tue, 31 Aug 2021 23:05:46 +0200
Subject: [PATCH 1/2] Harden 'gcc/input.c:string_concat_db::get_key_loc'

We're using 'UNKNOWN_LOCATION' as a spare value for 'Empty', so should
ascertain that we don't use it as a key additionally.

Follow-up to r239175 (commit 88faa309e5d6c6171b957daaf2f800920869)
"On-demand locations within string-literals".

	gcc/
	* input.c (string_concat_db::get_key_loc): Harden.
---
 gcc/input.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/gcc/input.c b/gcc/input.c
index 4b809862e02..98b8bb64618 100644
--- a/gcc/input.c
+++ b/gcc/input.c
@@ -1483,6 +1483,9 @@ string_concat_db::get_key_loc (location_t loc)
 
   loc = get_range_from_loc (line_table, loc).m_start;
 
+  /* Ascertain that 'loc' is valid as a key in 'm_table'.  */
+  gcc_checking_assert (!RESERVED_LOCATION_P (loc));
+
   return loc;
 }
 
-- 
2.33.0

>From 349a3172f64db93ee98ea39b36489b702b6596ab Mon Sep 17 00:00:00 2001
From: Thomas Schwinge 
Date: Tue, 31 Aug 2021 23:30:25 +0200
Subject: [PATCH 2/2] Generalize 'gcc/input.h:struct location_hash'

This is currently only used here ('gcc/input.h:class string_concat_db'), but is
actually generally useful, so advertize it as such.

Per the rationale given, we may use 'BUILTINS_LOCATION' as spare value for
'Deleted', in addition to the existing use of 'UNKNOWN_LOCATION' as spare value
for 'Empty'.

	gcc/
	* input.h (location_hash): Use 'BUILTINS_LOCATION' as spare value
	for 'Deleted'.  Turn into a '#define'.
---
 gcc/input.h | 21 +++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/gcc/input.h b/gcc/input.h
index e6881072c5f..46971a2684c 100644
--- a/gcc/input.h
+++ b/gcc/input.h
@@ -36,6 +36,25 @@ extern GTY(()) class line_maps *saved_line_table;
both UNKNOWN_LOCATION and BUILTINS_LOCATION fit into that.  */
 STATIC_ASSERT (BUILTINS_LOCATION < RESERVED_LOCATION_COUNT);
 
+/* Hasher for 'location_t' values satisfying '!RESERVED_LOCATION_P', thus able
+   to use 'UNKNOWN_LOCATION'/'BUILTINS_LOCATION' as spare values for
+   'Empty'/'Deleted'.  */
+/* If the following is used more than once, 'gengtype' generates duplicate
+   functions (thus: "error: redefinition of 'void gt_ggc_mx(location_hash&)'"
+   etc.):
+
+   struct location_hash
+ : int_hash {};
+
+   Likewise for this:
+
+   typedef int_hash
+ location_hash;
+
+   Thus, use a plain ol' '#define':
+*/
+#define location_hash int_hash
+
 extern bool is_location_from_builtin_token (location_t);
 extern expanded_location expand_location (location_t);
 
@@ -230,8 +249,6 @@ public:
   location_t * GTY ((atomic)) m_locs;
 };
 
-struct location_hash : int_hash  { };
-
 class GTY(()) string_concat_db
 {
  public:
-- 
2.33.0

Re: [Committed] [PATCH 2/4] (v4) On-demand locations within string-literals

[Prathamesh Kulkarni]

On 6 August 2016 at 11:16, Markus Trippelsdorf  wrote:
> On 2016.08.05 at 14:16 -0400, David Malcolm wrote:
>> Successfully bootstrapped the updated patch on x86_64-pc
>> -linux-gnu, and successfully ran the stage 1 selftests on powerpc-ibm
>> -aix7.1.3.0 (gcc111)
>>
>> Committed to trunk as r239175; I'm attaching the final version of the
>> patch for reference.
>
> It breaks the build on ppc64le (gcc112):
FWIW I am observing the same error on x86_64-unknown-linux-gnu:
http://pastebin.com/63k4CRVY

Thanks,
Prathamesh
>
> /home/trippels/gcc_build_dir/./gcc/xgcc -B/home/trippels/gcc_build_dir/./gcc/ 
> -xc -S -c /dev/null -fself-test
> cc1: internal compiler error: Segmentation fault
> 0x1088b293 crash_signal
> ../../gcc/gcc/toplev.c:335
> 0x1115c694 cpp_string_location_reader::get_next()
> ../../gcc/libcpp/charset.c:2143
> 0x1115c694 _cpp_valid_ucn
> ../../gcc/libcpp/charset.c:1079
> Please submit a full bug report,
> with preprocessed source if appropriate.
> Please include the complete backtrace with any bug report.
> See  for instructions.
> Makefile:1898: recipe for target 's-selftest' failed
>
> --
> Markus

Re: [Committed] [PATCH 2/4] (v4) On-demand locations within string-literals

[Markus Trippelsdorf]

On 2016.08.05 at 14:16 -0400, David Malcolm wrote:
> Successfully bootstrapped the updated patch on x86_64-pc
> -linux-gnu, and successfully ran the stage 1 selftests on powerpc-ibm
> -aix7.1.3.0 (gcc111)
> 
> Committed to trunk as r239175; I'm attaching the final version of the
> patch for reference.

It breaks the build on ppc64le (gcc112):

/home/trippels/gcc_build_dir/./gcc/xgcc -B/home/trippels/gcc_build_dir/./gcc/ 
-xc -S -c /dev/null -fself-test
cc1: internal compiler error: Segmentation fault
0x1088b293 crash_signal
../../gcc/gcc/toplev.c:335
0x1115c694 cpp_string_location_reader::get_next()
../../gcc/libcpp/charset.c:2143
0x1115c694 _cpp_valid_ucn
../../gcc/libcpp/charset.c:1079
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See  for instructions.
Makefile:1898: recipe for target 's-selftest' failed

-- 
Markus