Re: [PATCH] gcc: pass-manager: Fix memory leak. [PR jit/63854]
Hi Jeff and David, any news on this fix? Thanks, Marc Am Mo., 31. Jan. 2022 um 12:42 Uhr schrieb Marc Nieper-Wißkirchen : > > Attached to this email is the patch updated to the recent renaming from *.c > to *.cc. > > > Am So., 23. Jan. 2022 um 14:18 Uhr schrieb Marc Nieper-Wißkirchen > : >> >> Am Sa., 15. Jan. 2022 um 14:56 Uhr schrieb Marc Nieper-Wißkirchen >> : >> > >> > Jeff, David, do you need any more input from my side? >> > >> > -- Marc >> > >> > Am Sa., 8. Jan. 2022 um 17:32 Uhr schrieb Jeff Law : >> > > >> > > >> > > >> > > On 1/6/2022 6:53 AM, David Malcolm via Gcc-patches wrote: >> > > > On Sun, 2021-12-19 at 22:30 +0100, Marc Nieper-Wißkirchen wrote: >> > > >> This patch fixes a memory leak in the pass manager. In the existing >> > > >> code, >> > > >> the m_name_to_pass_map is allocated in >> > > >> pass_manager::register_pass_name, but >> > > >> never deallocated. This is fixed by adding a deletion in >> > > >> pass_manager::~pass_manager. Moreover the string keys in >> > > >> m_name_to_pass_map are >> > > >> all dynamically allocated. To free them, this patch adds a new hash >> > > >> trait for >> > > >> string hashes that are to be freed when the corresponding hash entry >> > > >> is removed. >> > > >> >> > > >> This fix is particularly relevant for using GCC as a library through >> > > >> libgccjit. >> > > >> The memory leak also occurs when libgccjit is instructed to use an >> > > >> external >> > > >> driver. >> > > >> >> > > >> Before the patch, compiling the hello world example of libgccjit with >> > > >> the external driver under Valgrind shows a loss of 12,611 (48 direct) >> > > >> bytes. After the patch, no memory leaks are reported anymore. >> > > >> (Memory leaks occurring when using the internal driver are mostly in >> > > >> the driver code in gcc/gcc.c and have to be fixed separately.) >> > > >> >> > > >> The patch has been tested by fully bootstrapping the compiler with >> > > >> the >> > > >> frontends C, C++, Fortran, LTO, ObjC, JIT and running the test suite >> > > >> under a x86_64-pc-linux-gnu host. >> > > > Thanks for the patch. >> > > > >> > > > It looks correct to me, given that pass_manager::register_pass_name >> > > > does an xstrdup and puts the result in the map. >> > > > >> > > > That said: >> > > > - I'm not officially a reviewer for this part of gcc (though I probably >> > > > touched this code last) >> > > > - is it cleaner to instead change m_name_to_pass_map's key type from >> > > > const char * to char *, to convey that the map "owns" the name? That >> > > > way we probably wouldn't need struct typed_const_free_remove, and (I >> > > > hope) works better with the type system. >> > > > >> > > > Dave >> > > > >> > > >> gcc/ChangeLog: >> > > >> >> > > >> PR jit/63854 >> > > >> * hash-traits.h (struct typed_const_free_remove): New. >> > > >> (struct free_string_hash): New. >> > > >> * pass_manager.h: Use free_string_hash. >> > > >> * passes.c (pass_manager::register_pass_name): Use >> > > >> free_string_hash. >> > > >> (pass_manager::~pass_manager): Delete allocated >> > > >> m_name_to_pass_map. >> > > My concern (and what I hadn't had time to dig into) was we initially >> > > used nofree_string_hash -- I wanted to make sure there wasn't any path >> > > where the name came from the stack (can't be free'd), was saved >> > > elsewhere (danging pointer) and the like. ie, why were we using >> > > nofree_string_hash to begin with? I've never really mucked around with >> > > these bits, so the analysis side kept falling off the daily todo list. >> >> The only occurrences of m_name_to_pass_map are in pass-manager.h >> (where it is defined as a private field of the class pass_manager) and >> in passes.cc. There is just one instance where a name is added to the >> map in passes.cc, namely through the put method. There, the name has >> been xstrdup'ed. >> >> The name (as a const char *) escapes the pass map in >> pass_manager::create_pass_tab through the call to >> m_name_pass_map->traverse. This inserts the name into the pass_tab, >> which is a static vec of const char *s. The pass_tab does not escape >> the translation unit of passes.c. It is used in dump_one_pass where >> the name is used as an argument to fprintf. The important point is >> that it is not freed and not further copied. >> >> > > >> > > If/once you're comfortable with the patch David, then go ahead and apply >> > > it on Marc's behalf. >> > > >> > > jeff >> > >
Re: [PATCH] gcc: pass-manager: Fix memory leak. [PR jit/63854]
Attached to this email is the patch updated to the recent renaming from *.c to *.cc. Am So., 23. Jan. 2022 um 14:18 Uhr schrieb Marc Nieper-Wißkirchen < m...@nieper-wisskirchen.de>: > Am Sa., 15. Jan. 2022 um 14:56 Uhr schrieb Marc Nieper-Wißkirchen > : > > > > Jeff, David, do you need any more input from my side? > > > > -- Marc > > > > Am Sa., 8. Jan. 2022 um 17:32 Uhr schrieb Jeff Law < > jeffreya...@gmail.com>: > > > > > > > > > > > > On 1/6/2022 6:53 AM, David Malcolm via Gcc-patches wrote: > > > > On Sun, 2021-12-19 at 22:30 +0100, Marc Nieper-Wißkirchen wrote: > > > >> This patch fixes a memory leak in the pass manager. In the existing > > > >> code, > > > >> the m_name_to_pass_map is allocated in > > > >> pass_manager::register_pass_name, but > > > >> never deallocated. This is fixed by adding a deletion in > > > >> pass_manager::~pass_manager. Moreover the string keys in > > > >> m_name_to_pass_map are > > > >> all dynamically allocated. To free them, this patch adds a new hash > > > >> trait for > > > >> string hashes that are to be freed when the corresponding hash entry > > > >> is removed. > > > >> > > > >> This fix is particularly relevant for using GCC as a library through > > > >> libgccjit. > > > >> The memory leak also occurs when libgccjit is instructed to use an > > > >> external > > > >> driver. > > > >> > > > >> Before the patch, compiling the hello world example of libgccjit > with > > > >> the external driver under Valgrind shows a loss of 12,611 (48 > direct) > > > >> bytes. After the patch, no memory leaks are reported anymore. > > > >> (Memory leaks occurring when using the internal driver are mostly in > > > >> the driver code in gcc/gcc.c and have to be fixed separately.) > > > >> > > > >> The patch has been tested by fully bootstrapping the compiler with > > > >> the > > > >> frontends C, C++, Fortran, LTO, ObjC, JIT and running the test suite > > > >> under a x86_64-pc-linux-gnu host. > > > > Thanks for the patch. > > > > > > > > It looks correct to me, given that pass_manager::register_pass_name > > > > does an xstrdup and puts the result in the map. > > > > > > > > That said: > > > > - I'm not officially a reviewer for this part of gcc (though I > probably > > > > touched this code last) > > > > - is it cleaner to instead change m_name_to_pass_map's key type from > > > > const char * to char *, to convey that the map "owns" the name? That > > > > way we probably wouldn't need struct typed_const_free_remove, and (I > > > > hope) works better with the type system. > > > > > > > > Dave > > > > > > > >> gcc/ChangeLog: > > > >> > > > >> PR jit/63854 > > > >> * hash-traits.h (struct typed_const_free_remove): New. > > > >> (struct free_string_hash): New. > > > >> * pass_manager.h: Use free_string_hash. > > > >> * passes.c (pass_manager::register_pass_name): Use > > > >> free_string_hash. > > > >> (pass_manager::~pass_manager): Delete allocated > > > >> m_name_to_pass_map. > > > My concern (and what I hadn't had time to dig into) was we initially > > > used nofree_string_hash -- I wanted to make sure there wasn't any path > > > where the name came from the stack (can't be free'd), was saved > > > elsewhere (danging pointer) and the like. ie, why were we using > > > nofree_string_hash to begin with? I've never really mucked around with > > > these bits, so the analysis side kept falling off the daily todo list. > > The only occurrences of m_name_to_pass_map are in pass-manager.h > (where it is defined as a private field of the class pass_manager) and > in passes.cc. There is just one instance where a name is added to the > map in passes.cc, namely through the put method. There, the name has > been xstrdup'ed. > > The name (as a const char *) escapes the pass map in > pass_manager::create_pass_tab through the call to > m_name_pass_map->traverse. This inserts the name into the pass_tab, > which is a static vec of const char *s. The pass_tab does not escape
Re: [PATCH] gcc: pass-manager: Fix memory leak. [PR jit/63854]
Am Sa., 15. Jan. 2022 um 14:56 Uhr schrieb Marc Nieper-Wißkirchen : > > Jeff, David, do you need any more input from my side? > > -- Marc > > Am Sa., 8. Jan. 2022 um 17:32 Uhr schrieb Jeff Law : > > > > > > > > On 1/6/2022 6:53 AM, David Malcolm via Gcc-patches wrote: > > > On Sun, 2021-12-19 at 22:30 +0100, Marc Nieper-Wißkirchen wrote: > > >> This patch fixes a memory leak in the pass manager. In the existing > > >> code, > > >> the m_name_to_pass_map is allocated in > > >> pass_manager::register_pass_name, but > > >> never deallocated. This is fixed by adding a deletion in > > >> pass_manager::~pass_manager. Moreover the string keys in > > >> m_name_to_pass_map are > > >> all dynamically allocated. To free them, this patch adds a new hash > > >> trait for > > >> string hashes that are to be freed when the corresponding hash entry > > >> is removed. > > >> > > >> This fix is particularly relevant for using GCC as a library through > > >> libgccjit. > > >> The memory leak also occurs when libgccjit is instructed to use an > > >> external > > >> driver. > > >> > > >> Before the patch, compiling the hello world example of libgccjit with > > >> the external driver under Valgrind shows a loss of 12,611 (48 direct) > > >> bytes. After the patch, no memory leaks are reported anymore. > > >> (Memory leaks occurring when using the internal driver are mostly in > > >> the driver code in gcc/gcc.c and have to be fixed separately.) > > >> > > >> The patch has been tested by fully bootstrapping the compiler with > > >> the > > >> frontends C, C++, Fortran, LTO, ObjC, JIT and running the test suite > > >> under a x86_64-pc-linux-gnu host. > > > Thanks for the patch. > > > > > > It looks correct to me, given that pass_manager::register_pass_name > > > does an xstrdup and puts the result in the map. > > > > > > That said: > > > - I'm not officially a reviewer for this part of gcc (though I probably > > > touched this code last) > > > - is it cleaner to instead change m_name_to_pass_map's key type from > > > const char * to char *, to convey that the map "owns" the name? That > > > way we probably wouldn't need struct typed_const_free_remove, and (I > > > hope) works better with the type system. > > > > > > Dave > > > > > >> gcc/ChangeLog: > > >> > > >> PR jit/63854 > > >> * hash-traits.h (struct typed_const_free_remove): New. > > >> (struct free_string_hash): New. > > >> * pass_manager.h: Use free_string_hash. > > >> * passes.c (pass_manager::register_pass_name): Use > > >> free_string_hash. > > >> (pass_manager::~pass_manager): Delete allocated > > >> m_name_to_pass_map. > > My concern (and what I hadn't had time to dig into) was we initially > > used nofree_string_hash -- I wanted to make sure there wasn't any path > > where the name came from the stack (can't be free'd), was saved > > elsewhere (danging pointer) and the like. ie, why were we using > > nofree_string_hash to begin with? I've never really mucked around with > > these bits, so the analysis side kept falling off the daily todo list. The only occurrences of m_name_to_pass_map are in pass-manager.h (where it is defined as a private field of the class pass_manager) and in passes.cc. There is just one instance where a name is added to the map in passes.cc, namely through the put method. There, the name has been xstrdup'ed. The name (as a const char *) escapes the pass map in pass_manager::create_pass_tab through the call to m_name_pass_map->traverse. This inserts the name into the pass_tab, which is a static vec of const char *s. The pass_tab does not escape the translation unit of passes.c. It is used in dump_one_pass where the name is used as an argument to fprintf. The important point is that it is not freed and not further copied. > > > > If/once you're comfortable with the patch David, then go ahead and apply > > it on Marc's behalf. > > > > jeff > >
Re: [PATCH] gcc: pass-manager: Fix memory leak. [PR jit/63854]
Jeff, David, do you need any more input from my side? -- Marc Am Sa., 8. Jan. 2022 um 17:32 Uhr schrieb Jeff Law : > > > > On 1/6/2022 6:53 AM, David Malcolm via Gcc-patches wrote: > > On Sun, 2021-12-19 at 22:30 +0100, Marc Nieper-Wißkirchen wrote: > >> This patch fixes a memory leak in the pass manager. In the existing > >> code, > >> the m_name_to_pass_map is allocated in > >> pass_manager::register_pass_name, but > >> never deallocated. This is fixed by adding a deletion in > >> pass_manager::~pass_manager. Moreover the string keys in > >> m_name_to_pass_map are > >> all dynamically allocated. To free them, this patch adds a new hash > >> trait for > >> string hashes that are to be freed when the corresponding hash entry > >> is removed. > >> > >> This fix is particularly relevant for using GCC as a library through > >> libgccjit. > >> The memory leak also occurs when libgccjit is instructed to use an > >> external > >> driver. > >> > >> Before the patch, compiling the hello world example of libgccjit with > >> the external driver under Valgrind shows a loss of 12,611 (48 direct) > >> bytes. After the patch, no memory leaks are reported anymore. > >> (Memory leaks occurring when using the internal driver are mostly in > >> the driver code in gcc/gcc.c and have to be fixed separately.) > >> > >> The patch has been tested by fully bootstrapping the compiler with > >> the > >> frontends C, C++, Fortran, LTO, ObjC, JIT and running the test suite > >> under a x86_64-pc-linux-gnu host. > > Thanks for the patch. > > > > It looks correct to me, given that pass_manager::register_pass_name > > does an xstrdup and puts the result in the map. > > > > That said: > > - I'm not officially a reviewer for this part of gcc (though I probably > > touched this code last) > > - is it cleaner to instead change m_name_to_pass_map's key type from > > const char * to char *, to convey that the map "owns" the name? That > > way we probably wouldn't need struct typed_const_free_remove, and (I > > hope) works better with the type system. > > > > Dave > > > >> gcc/ChangeLog: > >> > >> PR jit/63854 > >> * hash-traits.h (struct typed_const_free_remove): New. > >> (struct free_string_hash): New. > >> * pass_manager.h: Use free_string_hash. > >> * passes.c (pass_manager::register_pass_name): Use > >> free_string_hash. > >> (pass_manager::~pass_manager): Delete allocated > >> m_name_to_pass_map. > My concern (and what I hadn't had time to dig into) was we initially > used nofree_string_hash -- I wanted to make sure there wasn't any path > where the name came from the stack (can't be free'd), was saved > elsewhere (danging pointer) and the like. ie, why were we using > nofree_string_hash to begin with? I've never really mucked around with > these bits, so the analysis side kept falling off the daily todo list. > > If/once you're comfortable with the patch David, then go ahead and apply > it on Marc's behalf. > > jeff >
Re: [PATCH] gcc: pass-manager: Fix memory leak. [PR jit/63854]
Thanks for replying so quickly! Am Do., 6. Jan. 2022 um 14:53 Uhr schrieb David Malcolm : [...] > Thanks for the patch. > > It looks correct to me, given that pass_manager::register_pass_name > does an xstrdup and puts the result in the map. > > That said: > - I'm not officially a reviewer for this part of gcc (though I probably > touched this code last) I am a newcomer to the codebase of GCC and haven't yet been able to figure out whom to contact. I bothered you because the patch is mostly relevant for the libgccjit frontend. > - is it cleaner to instead change m_name_to_pass_map's key type from > const char * to char *, to convey that the map "owns" the name? That > way we probably wouldn't need struct typed_const_free_remove, and (I > hope) works better with the type system. The problem with that approach is that we would then need a new version of string_hash in hash-traits.h, say owned_string_hash, which derives from pointer_hash and not pointer_hash . This would add roughly as much code as struct typed_const_free_remove. Using the hypothetical owned_string_hash in the definition of m_name_to_pass_map in passes.c would then produce a map taking "char *" strings instead of "const char *" strings. This, however, would then lead to problems in pass_manager::register_pass_name where name is a "const char *" string (coming from outside) but m_name_to_pass_map->get would take a "char *" string. I don't see how to resolve this without bigger refactoring, so I think my struct typed_const_free_remove approach is less intrusive. This conveys at least that the key isn't changed by the hashmap operations and that it is yet owned (because this is something that typed_const_free_remove presupposes. Thanks, Marc [...]
Re: [PATCH] gcc: pass-manager: Fix memory leak. [PR jit/63854]
Am Do., 6. Jan. 2022 um 14:57 Uhr schrieb David Malcolm via Jit
:
> [...snip...]
>
> >
> > > diff --git a/gcc/passes.c b/gcc/passes.c
> > > index 4bea6ae5b6a..0c70ece5321 100644
> > > --- a/gcc/passes.c
> > > +++ b/gcc/passes.c
>
> [...snip...]
>
> > > @@ -1943,7 +1944,7 @@ pass_manager::dump_profile_report () const
> > >" |in count |out
> > > prob "
> > >"|in count |out prob "
> > >"|size |time |\n");
> > > -
> > > +
> > >for (int i = 1; i < passes_by_id_size; i++)
> > > if (profile_record[i].run)
> > >{
> >
>
> ...and there's a stray whitespace change here (in
> pass_manager::dump_profile_report), which probably shouldn't be in the
> patch.
There was stray whitespace in that line the unpatched version of
`passes.c`, which my Emacs silently cleaned up.
Shall I retain this whitespace although it should probably haven't
been there in the first place? Or should I just add a remark in the
patch notes about that?
Thanks,
Marc
[PATCH] gcc: pass-manager: Fix memory leak. [PR jit/63854]
This patch fixes a memory leak in the pass manager. In the existing code,
the m_name_to_pass_map is allocated in pass_manager::register_pass_name, but
never deallocated. This is fixed by adding a deletion in
pass_manager::~pass_manager. Moreover the string keys in m_name_to_pass_map are
all dynamically allocated. To free them, this patch adds a new hash trait for
string hashes that are to be freed when the corresponding hash entry is removed.
This fix is particularly relevant for using GCC as a library through libgccjit.
The memory leak also occurs when libgccjit is instructed to use an external
driver.
Before the patch, compiling the hello world example of libgccjit with
the external driver under Valgrind shows a loss of 12,611 (48 direct)
bytes. After the patch, no memory leaks are reported anymore.
(Memory leaks occurring when using the internal driver are mostly in
the driver code in gcc/gcc.c and have to be fixed separately.)
The patch has been tested by fully bootstrapping the compiler with the
frontends C, C++, Fortran, LTO, ObjC, JIT and running the test suite
under a x86_64-pc-linux-gnu host.
gcc/ChangeLog:
PR jit/63854
* hash-traits.h (struct typed_const_free_remove): New.
(struct free_string_hash): New.
* pass_manager.h: Use free_string_hash.
* passes.c (pass_manager::register_pass_name): Use free_string_hash.
(pass_manager::~pass_manager): Delete allocated m_name_to_pass_map.
---
gcc/hash-traits.h | 17 +
gcc/pass_manager.h | 3 +--
gcc/passes.c | 5 +++--
3 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/gcc/hash-traits.h b/gcc/hash-traits.h
index 6f0373ec27f..1c08d6874ab 100644
--- a/gcc/hash-traits.h
+++ b/gcc/hash-traits.h
@@ -28,6 +28,11 @@ struct typed_free_remove
static inline void remove (Type *p);
};
+template
+struct typed_const_free_remove
+{
+ static inline void remove (const Type *p);
+};
/* Remove with free. */
@@ -38,6 +43,13 @@ typed_free_remove ::remove (Type *p)
free (p);
}
+template
+inline void
+typed_const_free_remove ::remove (const Type *p)
+{
+ free (const_cast (p));
+}
+
/* Helpful type for removing with delete. */
template
@@ -305,6 +317,11 @@ struct ggc_ptr_hash : pointer_hash , ggc_remove
{};
template
struct ggc_cache_ptr_hash : pointer_hash , ggc_cache_remove {};
+/* Traits for string elements that should be freed when an element is
+ deleted. */
+
+struct free_string_hash : string_hash, typed_const_free_remove {};
+
/* Traits for string elements that should not be freed when an element
is deleted. */
diff --git a/gcc/pass_manager.h b/gcc/pass_manager.h
index aaf72cf6803..f5615e1fda8 100644
--- a/gcc/pass_manager.h
+++ b/gcc/pass_manager.h
@@ -106,7 +106,7 @@ private:
private:
context *m_ctxt;
- hash_map *m_name_to_pass_map;
+ hash_map *m_name_to_pass_map;
/* References to all of the individual passes.
These fields are generated via macro expansion.
@@ -146,4 +146,3 @@ private:
} // namespace gcc
#endif /* ! GCC_PASS_MANAGER_H */
-
diff --git a/gcc/passes.c b/gcc/passes.c
index 4bea6ae5b6a..0c70ece5321 100644
--- a/gcc/passes.c
+++ b/gcc/passes.c
@@ -903,7 +903,7 @@ void
pass_manager::register_pass_name (opt_pass *pass, const char *name)
{
if (!m_name_to_pass_map)
-m_name_to_pass_map = new hash_map (256);
+m_name_to_pass_map = new hash_map (256);
if (m_name_to_pass_map->get (name))
return; /* Ignore plugin passes. */
@@ -1674,6 +1674,7 @@ pass_manager::~pass_manager ()
GCC_PASS_LISTS
#undef DEF_PASS_LIST
+ delete m_name_to_pass_map;
}
/* If we are in IPA mode (i.e., current_function_decl is NULL), call
@@ -1943,7 +1944,7 @@ pass_manager::dump_profile_report () const
" |in count |out prob "
"|in count |out prob "
"|size |time |\n");
-
+
for (int i = 1; i < passes_by_id_size; i++)
if (profile_record[i].run)
{
--
2.32.0
Re: [PATCH] jit: Add support for global rvalue initialization and ctors
Hi,
I would favor adding support for this kind of initialization to libgccjit.
Does it also support the libgccjit equivalent of the following C module,
which contains forward references in the struct initializers?
struct bar bar;
struct foo foo;
struct foo
{
struct bar *b;
};
struct bar
{
struct foo *f;
};
struct bar bar = {.f = };
struct foo foo = {.b = };
Thanks,
Marc
Am Mo., 29. Nov. 2021 um 21:04 Uhr schrieb Petter Tomner via Jit <
j...@gcc.gnu.org>:
> Hi!
>
> I have wrapped up the patch than adds support for initialization of global
> variables
> with rvalues aswell as rvalue constructors for structs, arrays and unions.
>
> New entrypoints are:
>
> gcc_jit_global_set_initializer_rvalue
>
> Which sets the initial value of a global to a rvalue.
>
> And:
>
> gcc_jit_context_new_array_constructor
> gcc_jit_context_new_struct_constructor
> gcc_jit_context_new_union_constructor
>
> Those three makes a constructor with a rvalue that e.g. can be assigned to
> a local or returned
> from a function, or most importantly used to set the initial value of
> global variables
> with gcc_jit_global_set_initializer_rvalue.
>
> If no fields are specified for a struct or union to the constructors,
> definition order is assumed.
>
> There can be gaps in the fields specified to the struct constructor, but
> they need to be in order.
>
> For pointer arithmetic to work with setting DECL_INITIAL, alot of folding
> is added.
>
> make check-jit runs fine on gnu-linux-x64 Debian.
>
> Regards,
Re: [PATCH][jit] Add thread-local globals to the libgccjit frontend
[...]
2019-01-08 Marc Nieper-Wißkirchen
* docs/topics/compatibility.rst: Add LIBGCCJIT_ABI_11.
* docs/topics/expressions.rst (Global variables): Add
documentation of gcc_jit_lvalue_set_bool_thread_local.
* docs/_build/texinfo/libgccjit.texi: Regenerate.
* jit-playback.c: Include "varasm.h".
Within namespace gcc::jit::playback...
(context::new_global) Add "thread_local_p" param and use it
to set DECL_TLS_MODEL.
* jit-playback.h: Within namespace gcc::jit::playback...
(context::new_global): Add "thread_local_p" param.
* jit-recording.c: Within namespace gcc::jit::recording...
(global::replay_into): Provide m_thread_local to call to
new_global.
(global::write_reproducer): Call write_reproducer_thread_local.
(global::write_reproducer_thread_local): New method.
* jit-recording.h: Within namespace gcc::jit::recording...
(lvalue::dyn_cast_global): New virtual function.
(global::m_thread_local): New field.
* libgccjit.c (gcc_jit_lvalue_set_bool_thread_local): New
function.
* libgccjit.h
(LIBGCCJIT_HAVE_gcc_jit_lvalue_set_bool_thread_local): New
macro.
(gcc_jit_lvalue_set_bool_thread_local): New function.
* libgccjit.map (LIBGCCJIT_ABI_11): New.
(gcc_jit_lvalue_set_bool_thread_local): Add.
* ../testsuite/jit.dg/all-non-failing-tests.h: Include new
test.
* ../testsuite/jit.dg/jit.exp: Load pthread for tests involving
thread-local globals.
* ../testsuite/jit.dg/test-thread-local.c: New test case for
thread-local globals.
BTW, the convention here is to split out the ChangeLog entries by
directory based on the presence of ChangeLog files.
There's a gcc/jit/ChangeLog and a gcc/testsuite/ChangeLog, so for this
patch there should be two sets of ChangeLog entries, one for each of
these, with the paths expressed relative to the directory holding the
ChangeLog.
So the testsuite entries would go into gcc/testsuite/ChangeLog, and
look like:
* jit.dg/all-non-failing-tests.h: Include new test.
...etc.
Thanks for explaining the policy. Corrected ChangeLogs:
gcc/jit/ChangeLog
=====
2019-01-08 Marc Nieper-Wißkirchen
* docs/topics/compatibility.rst: Add LIBGCCJIT_ABI_11.
* docs/topics/expressions.rst (Global variables): Add
documentation of gcc_jit_lvalue_set_bool_thread_local.
* docs/_build/texinfo/libgccjit.texi: Regenerate.
* jit-playback.c: Include "varasm.h".
Within namespace gcc::jit::playback...
(context::new_global) Add "thread_local_p" param and use it
to set DECL_TLS_MODEL.
* jit-playback.h: Within namespace gcc::jit::playback...
(context::new_global): Add "thread_local_p" param.
* jit-recording.c: Within namespace gcc::jit::recording...
(global::replay_into): Provide m_thread_local to call to
new_global.
(global::write_reproducer): Call write_reproducer_thread_local.
(global::write_reproducer_thread_local): New method.
* jit-recording.h: Within namespace gcc::jit::recording...
(lvalue::dyn_cast_global): New virtual function.
(global::m_thread_local): New field.
* libgccjit.c (gcc_jit_lvalue_set_bool_thread_local): New
function.
* libgccjit.h
(LIBGCCJIT_HAVE_gcc_jit_lvalue_set_bool_thread_local): New
macro.
(gcc_jit_lvalue_set_bool_thread_local): New function.
* libgccjit.map (LIBGCCJIT_ABI_11): New.
(gcc_jit_lvalue_set_bool_thread_local): Add.
gcc/testsuite/ChangeLog
===
2019-01-08 Marc Nieper-Wißkirchen
* jit.dg/all-non-failing-tests.h: Include new test.
* jit.dg/jit.exp: Load pthread for tests involving
thread-local globals.
* jit.dg/test-thread-local.c: New test case for
thread-local globals.
[...]
diff --git a/gcc/testsuite/jit.dg/test-thread-local.c
b/gcc/testsuite/jit.dg/test-thread-local.c
new file mode 100644
index 0..287ba85e4
--- /dev/null
+++ b/gcc/testsuite/jit.dg/test-thread-local.c
@@ -0,0 +1,99 @@
+#include
+#include
+#include
+#include
+
+#include "libgccjit.h"
+
+#include "harness.h"
+
+void
+create_code (gcc_jit_context *ctxt, void *user_data)
+{
+ /* Let's try to inject the equivalent of:
+
+ static thread_local int tl;
+ set_tl (int v)
+ {
+tl = v;
+ }
+
+ int
+ get_tl (void)
+ {
+return tl;
+ }
Thanks for posting this.
This test is OK as far as it goes, but there are some gaps in test
coverage: the test verifies that jit-generated code can create a new
thread-local variable, and writes to it, but it doesn't seem to test
reading from it; e.g. there doesn't seem to be a CHECK_VALUE that
Re: [PATCH][jit] Add thread-local globals to the libgccjit frontend
Dear David, thank you very much for your timely response and for talking a thorough look at my proposed patch. Am 07.01.19 um 21:34 schrieb David Malcolm: Have you done the legal paperwork with the FSF for contributing to GCC? See https://gcc.gnu.org/contribute.html#legal Not yet; this is my first patch I would like to contribute to GCC. You should have received a private email to get the legal matters done. ChangeLog 2019-01-05 Marc Nieper-Wißkirchen * docs/topics/compatibility.rst: Add LIBGCCJIT_ABI_11. * docs/topics/expressions.rst (Global variables): Add documentation of gcc_jit_lvalue_set_bool_thread_local. * docs/_build/texinfo/libgccjit.texi: Regenerate. * jit-playback.c: Include "varasm.h". Within namespace gcc::jit::playback... (context::new_global) Add "thread_local_p" param and use it to set DECL_TLS_MODEL. * jit-playback.h: Within namespace gcc::jit::playback... (context::new_global: Add "thread_local_p" param. * jit-recording.c: Within namespace gcc::jit::recording... (global::replay_into): Provide m_thread_local to call to new_global. (global::write_reproducer): Call write_reproducer_thread_local. (global::write_reproducer_thread_local): New method. * jit-recording.h: Within namespace gcc::jit::recording... (lvalue::dyn_cast_global): New virtual function. (global::m_thread_local): New field. * libgccjit.c (gcc_jit_lvalue_set_bool_thread_local): New function. * libgccjit.h (LIBGCCJIT_HAVE_gcc_jit_lvalue_set_bool_thread_local): New macro. (gcc_jit_lvalue_set_bool_thread_local): New function. * libgccjit.map (LIBGCCJIT_ABI_11): New. (gcc_jit_lvalue_set_bool_thread_local): Add. Testing The result of `make check-jit' is (the failing test in `test-sum-squares.c` is also failing without this patch on my machine): Native configuration is x86_64-pc-linux-gnu [...] FAIL: test-combination.c.exe iteration 1 of 5: verify_code_sum_of_squares: dump_vrp1: actual: " FAIL: test-combination.c.exe killed: 20233 exp6 0 0 CHILDKILLED SIGABRT SIGABRT FAIL: test-sum-of-squares.c.exe iteration 1 of 5: verify_code: dump_vrp1: actual: " FAIL: test-sum-of-squares.c.exe killed: 22698 exp6 0 0 CHILDKILLED SIGABRT SIGABRT FAIL: test-threads.c.exe: verify_code_sum_of_squares: dump_vrp1: actual: " FAIL: test-threads.c.exe killed: 22840 exp6 0 0 CHILDKILLED SIGABRT SIGABRT That one's failing for me as well. I'm investigating (I've filed it as PR jit/88747). I have applied your recent patch. With the patch, there are no more failures. Including the new test case for thread-local globals (see below), the final output of `make check-jit' is now as follows: Test run by mnieper on Tue Jan 8 14:18:27 2019 Native configuration is x86_64-pc-linux-gnu === jit tests === Schedule of variations: unix Running target unix Using /usr/share/dejagnu/baseboards/unix.exp as board description file for target. Using /usr/share/dejagnu/config/unix.exp as generic interface file for target. Using /home/mnieper/gcc/src/gcc/testsuite/config/default.exp as tool-and-target-specific interface file. Running /home/mnieper/gcc/src/gcc/testsuite/jit.dg/jit.exp ... === jit Summary === # of expected passes 10394 diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h index bf02e1258..c2654ff09 100644 --- a/gcc/testsuite/jit.dg/all-non-failing-tests.h +++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h @@ -224,6 +224,13 @@ #undef create_code #undef verify_code +/* test-factorial-must-tail-call.c */ Looks like a cut error: presumably the above comment is meant to refer to the new test... Yep. +#define create_code create_code_thread_local +#define verify_code verify_code_thread_local +#include "test-thread-local.c" ...but it looks like the new test file is missing from the patch. My fault. I supplied the wrong arguments to `git diff'. At the end of this email, you'll find the updated ChangeLog and the amended patch. Thanks again, Marc 2019-01-08 Marc Nieper-Wißkirchen * docs/topics/compatibility.rst: Add LIBGCCJIT_ABI_11. * docs/topics/expressions.rst (Global variables): Add documentation of gcc_jit_lvalue_set_bool_thread_local. * docs/_build/texinfo/libgccjit.texi: Regenerate. * jit-playback.c: Include "varasm.h". Within namespace gcc::jit::playback... (context::new_global) Add "thread_local_p" param and use it to set DECL_TLS_MODEL. * jit-playback.h: Within namespace gcc::jit::playback... (context::new_global): Add "thread_local_p" param. * jit-recording.c: Within namespace gcc::jit::recording... (global::replay_into): Provide m_thread_local to call to new_global. (global::write_reproducer): Call write_reproducer_thread_local. (global::write_reproducer_thread_local): New metho
[PATCH][jit] Add thread-local globals to the libgccjit frontend
This patch adds thread-local globals to the libgccjit frontend. The
library user can mark a global as being thread-local by calling
`gcc_jit_lvalue_set_bool_thread_local'. It is implemented by calling
`set_decl_tls_model (inner, decl_default_tls_model (inner))', where
`inner' is the GENERIC tree corresponding to the global.
ChangeLog
2019-01-05 Marc Nieper-Wißkirchen
* docs/topics/compatibility.rst: Add LIBGCCJIT_ABI_11.
* docs/topics/expressions.rst (Global variables): Add
documentation of gcc_jit_lvalue_set_bool_thread_local.
* docs/_build/texinfo/libgccjit.texi: Regenerate.
* jit-playback.c: Include "varasm.h".
Within namespace gcc::jit::playback...
(context::new_global) Add "thread_local_p" param and use it
to set DECL_TLS_MODEL.
* jit-playback.h: Within namespace gcc::jit::playback...
(context::new_global: Add "thread_local_p" param.
* jit-recording.c: Within namespace gcc::jit::recording...
(global::replay_into): Provide m_thread_local to call to
new_global.
(global::write_reproducer): Call write_reproducer_thread_local.
(global::write_reproducer_thread_local): New method.
* jit-recording.h: Within namespace gcc::jit::recording...
(lvalue::dyn_cast_global): New virtual function.
(global::m_thread_local): New field.
* libgccjit.c (gcc_jit_lvalue_set_bool_thread_local): New
function.
* libgccjit.h
(LIBGCCJIT_HAVE_gcc_jit_lvalue_set_bool_thread_local): New
macro.
(gcc_jit_lvalue_set_bool_thread_local): New function.
* libgccjit.map (LIBGCCJIT_ABI_11): New.
(gcc_jit_lvalue_set_bool_thread_local): Add.
Testing
The result of `make check-jit' is (the failing test in
`test-sum-squares.c` is also failing without this patch on my
machine):
Native configuration is x86_64-pc-linux-gnu
=== jit tests ===
Schedule of variations:
unix
Running target unix
Using /usr/share/dejagnu/baseboards/unix.exp as board description file
for target.
Using /usr/share/dejagnu/config/unix.exp as generic interface file for target.
Using /home/mnieper/gcc/src/gcc/testsuite/config/default.exp as
tool-and-target-specific interface file.
Running /home/mnieper/gcc/src/gcc/testsuite/jit.dg/jit.exp ...
FAIL: test-combination.c.exe iteration 1 of 5:
verify_code_sum_of_squares: dump_vrp1: actual: "
FAIL: test-combination.c.exe killed: 20233 exp6 0 0 CHILDKILLED SIGABRT SIGABRT
FAIL: test-sum-of-squares.c.exe iteration 1 of 5: verify_code:
dump_vrp1: actual: "
FAIL: test-sum-of-squares.c.exe killed: 22698 exp6 0 0 CHILDKILLED
SIGABRT SIGABRT
FAIL: test-threads.c.exe: verify_code_sum_of_squares: dump_vrp1: actual: "
FAIL: test-threads.c.exe killed: 22840 exp6 0 0 CHILDKILLED SIGABRT SIGABRT
=== jit Summary ===
# of expected passes 6506
# of unexpected failures 6
Patch
diff --git a/gcc/jit/docs/topics/compatibility.rst
b/gcc/jit/docs/topics/compatibility.rst
index 38d338b1f..10926537d 100644
--- a/gcc/jit/docs/topics/compatibility.rst
+++ b/gcc/jit/docs/topics/compatibility.rst
@@ -171,3 +171,9 @@ entrypoints:
``LIBGCCJIT_ABI_10`` covers the addition of
:func:`gcc_jit_context_new_rvalue_from_vector`
+
+``LIBGCCJIT_ABI_11``
+
+
+``LIBGCCJIT_ABI_11`` covers the addition of
+:func:`gcc_jit_lvalue_set_bool_thread_local`
diff --git a/gcc/jit/docs/topics/expressions.rst
b/gcc/jit/docs/topics/expressions.rst
index 9dee2d87a..984d02cc8 100644
--- a/gcc/jit/docs/topics/expressions.rst
+++ b/gcc/jit/docs/topics/expressions.rst
@@ -576,6 +576,21 @@ Global variables
referring to it. Analogous to using an "extern" global from a
header file.
+.. function:: void\
+ gcc_jit_lvalue_set_bool_thread_local (gcc_jit_lvalue *global,\
+int thread_local_p)
+
+ Given a :c:type:`gcc_jit_lvalue *` for a global created through
+ :c:func:`gcc_jit_context_new_global`, mark/clear the global as being
+ thread-local. Analogous to a global with thread storage duration in C11.
+
+ This entrypoint was added in :ref:`LIBGCCJIT_ABI_11`; you can test for
+ its presence using
+
+ .. code-block:: c
+
+ #ifdef LIBGCCJIT_HAVE_gcc_jit_lvalue_set_bool_thread_local
+
Working with pointers, structs and unions
-
diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c
index 86f588db9..6c00a98c0 100644
--- a/gcc/jit/jit-playback.c
+++ b/gcc/jit/jit-playback.c
@@ -39,6 +39,7 @@ along with GCC; see the file COPYING3. If not see
#include "opt-suggestions.h"
#include "gcc.h"
#include "diagnostic.h"
+#include "varasm.h"
#include
@@ -461,7 +462,8 @@ playback::context::
new_global (location *loc,
enum gcc_jit_global_kind kind,
type *type,
- const char *name)
+ const char *name,
+ bool thread_local_p)
{
gcc_assert (type);
gcc_assert (name);
@@ -487,6 +489,8 @@ new_global (location *loc,
DECL_EXTERNAL (inner) = 1;
break;
}
+ i
