public inbox for gcc-cvs@sourceware.org
help / color / mirror / Atom feed
From: Lewis Hyatt <lhyatt@gcc.gnu.org>
To: gcc-cvs@gcc.gnu.org
Subject: [gcc r13-2539] pch: Fix the reconstruction of adhoc data hash table
Date: Thu,  8 Sep 2022 13:11:30 +0000 (GMT)	[thread overview]
Message-ID: <20220908131130.6619B3858421@sourceware.org> (raw)

https://gcc.gnu.org/g:95c7d5899521a9e266c68cbcc92edfd2cde8694e

commit r13-2539-g95c7d5899521a9e266c68cbcc92edfd2cde8694e
Author: Lewis Hyatt <lhyatt@gmail.com>
Date:   Wed Sep 7 09:33:26 2022 -0400

    pch: Fix the reconstruction of adhoc data hash table
    
    The function rebuild_location_adhoc_htab() was meant to reconstruct the
    adhoc location hash map after restoring a line_maps instance from a
    PCH. However, the function has never performed as intended because it
    missed the last step of adding the data into the newly reconstructed hash
    map. This patch fixes that.
    
    It does not seem possible to construct a test case such that the current
    incorrect behavior is observable as a compiler issue. It would be
    observable, if it were possible for a precompiled header to contain an
    adhoc location with a non-zero custom data pointer. But currently, such
    data pointers are used only by the middle end to track inlining
    information, and this happens later, too late to show up in a PCH.
    
    I also noted that location_adhoc_data_update, which updates the hash map
    pointers in a different scenario, was relying on undefined pointer
    arithmetic behavior. I'm not aware of this having caused any issue in
    practice, but in this patch I have also changed it to use defined pointer
    operations instead.
    
    libcpp/ChangeLog:
    
            * line-map.cc (location_adhoc_data_update): Remove reliance on
            undefined behavior.
            (get_combined_adhoc_loc): Likewise.
            (rebuild_location_adhoc_htab): Fix issue where the htab was not
            properly updated.

Diff:
---
 libcpp/line-map.cc | 41 +++++++++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/libcpp/line-map.cc b/libcpp/line-map.cc
index 62077c3857c..391f1d4bbc1 100644
--- a/libcpp/line-map.cc
+++ b/libcpp/line-map.cc
@@ -85,27 +85,38 @@ location_adhoc_data_eq (const void *l1, const void *l2)
 	  && lb1->data == lb2->data);
 }
 
-/* Update the hashtable when location_adhoc_data is reallocated.  */
+/* Update the hashtable when location_adhoc_data_map::data is reallocated.
+   The param is an array of two pointers, the previous value of the data
+   pointer, and then the new value.  The pointers stored in the hash map
+   are then rebased to be relative to the new data pointer instead of the
+   old one.  */
 
 static int
-location_adhoc_data_update (void **slot, void *data)
+location_adhoc_data_update (void **slot_v, void *param_v)
 {
-  *((char **) slot)
-    = (char *) ((uintptr_t) *((char **) slot) + *((ptrdiff_t *) data));
+  const auto slot = reinterpret_cast<location_adhoc_data **> (slot_v);
+  const auto param = static_cast<location_adhoc_data **> (param_v);
+  *slot = (*slot - param[0]) + param[1];
   return 1;
 }
 
-/* Rebuild the hash table from the location adhoc data.  */
+/* The adhoc data hash table is not part of the GGC infrastructure, so it was
+   not initialized when SET was reconstructed from PCH; take care of that by
+   rebuilding it from scratch.  */
 
 void
 rebuild_location_adhoc_htab (line_maps *set)
 {
-  unsigned i;
   set->location_adhoc_data_map.htab =
       htab_create (100, location_adhoc_data_hash, location_adhoc_data_eq, NULL);
-  for (i = 0; i < set->location_adhoc_data_map.curr_loc; i++)
-    htab_find_slot (set->location_adhoc_data_map.htab,
-		    set->location_adhoc_data_map.data + i, INSERT);
+  for (auto p = set->location_adhoc_data_map.data,
+	    end = p + set->location_adhoc_data_map.curr_loc;
+      p != end; ++p)
+    {
+      const auto slot = reinterpret_cast<location_adhoc_data **>
+	(htab_find_slot (set->location_adhoc_data_map.htab, p, INSERT));
+      *slot = p;
+    }
 }
 
 /* Helper function for get_combined_adhoc_loc.
@@ -211,8 +222,7 @@ get_combined_adhoc_loc (line_maps *set,
       if (set->location_adhoc_data_map.curr_loc >=
 	  set->location_adhoc_data_map.allocated)
 	{
-	  char *orig_data = (char *) set->location_adhoc_data_map.data;
-	  ptrdiff_t offset;
+	  const auto orig_data = set->location_adhoc_data_map.data;
 	  /* Cast away extern "C" from the type of xrealloc.  */
 	  line_map_realloc reallocator = (set->reallocator
 					  ? set->reallocator
@@ -226,10 +236,13 @@ get_combined_adhoc_loc (line_maps *set,
 	      reallocator (set->location_adhoc_data_map.data,
 			   set->location_adhoc_data_map.allocated
 			   * sizeof (struct location_adhoc_data));
-	  offset = (char *) (set->location_adhoc_data_map.data) - orig_data;
 	  if (set->location_adhoc_data_map.allocated > 128)
-	    htab_traverse (set->location_adhoc_data_map.htab,
-			   location_adhoc_data_update, &offset);
+	    {
+	      location_adhoc_data *param[2]
+		= {orig_data, set->location_adhoc_data_map.data};
+	      htab_traverse (set->location_adhoc_data_map.htab,
+			     location_adhoc_data_update, param);
+	    }
 	}
       *slot = set->location_adhoc_data_map.data
 	      + set->location_adhoc_data_map.curr_loc;

                 reply	other threads:[~2022-09-08 13:11 UTC|newest]

Thread overview: [no followups] expand[flat|nested]  mbox.gz  Atom feed

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220908131130.6619B3858421@sourceware.org \
    --to=lhyatt@gcc.gnu.org \
    --cc=gcc-cvs@gcc.gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).