Fix global cache destruction in IdString class #3081
Conversation
|
@Icenowy could you rebase this patch? The hacky workaround that is b59c717 seems to have caused conflicts. @dragonmux we're sucessfully using this patch in Debian to fix LTO-enabled yosys, you might want to pick this one up :) |
|
Our fix isn't that hacky - it fixes a legitimate bit of UB albeit with some different UB that the compiler can't actually notice to optimise out. It should work to fix LTO-enabled yosys on Debian just the same (that's how we're using it!). That said, if Icenowy is happy for us to, we'd be happy to look at rebasing this patch and championing it to help see it merged. It looks to be a much more holistic approach to the problem. |
|
I would describe intentionally invoking UB as "hacky". |
|
My thoughts exactly ;) |
When enabling LTO on GCC, the sequence of destructing static members in IdString class is not promised, which can lead to the cache itself being destructed but the destruct_guard not destructed. Put all static members of IdString to a subclass similar to current desturct_guard_t, to mmakes the guard a real guard. This fixes SEGV when exiting a LTO-ed GCC-built yosys binary. Signed-off-by: Icenowy Zheng <[email protected]>
Icenowy
force-pushed
the
idstring-lto-fix
branch
from
0b41736
to
001a63f
Compare
|
well I think my code just reduces UB instead of removing it... Preventing UB here is just too difficult... |
| log("#X# DB-DUMP index %d: '%s' (ref %d)\n", idx, global_id_storage_.at(idx), global_refcount_storage_.at(idx)); | ||
| } | ||
| static struct global_cache_t { | ||
| bool destruct_guard; // POD, will be initialized to zero |
There was a problem hiding this comment.
I think accessing destruct_guard of a destroyed object isn't legal. Neither is calling a method of a destroyed object. However, if you put static bool destruct_guard; outside of global_cache_t that would make it accessible until the program dies, and if you keep all methods accessing it static calling them would also be fine.
Does that sound reasonable to you?
There was a problem hiding this comment.
You are correct that this re-introduces the exact piece of UB that our original patch was aimed at fixing. The compiler can reasonably assume that destruct_guard is always true because it cannot be false because accessing it after destruction is strictly invalid. That's why the compiler/LTO was able to optimise this out before our PR.
The trick is that the reason for this guard existing is itself due to other much more difficult to fix UB fundamental to how yosys is written, so the best one can do is reduce this to something the compiler cannot reason about in that way, so while the lifetime is still violated, it's done so in a way that is well-enough defined and non-optimisable.
There was a problem hiding this comment.
Calling any and all functions on this class after it's already been destroyed is itself UB. This guard needing to exist in any form is itself indicative of UB in the way yosys tears down it structures and state as it exits.
There was a problem hiding this comment.
I did offer a solution to this, though:
and if you keep all methods accessing it static calling them would also be fine.
|
在 2023-08-29星期二的 11:53 -0700，Catherine写道：
@whitequark commented on this pull request.
> - static int last_created_idx_ptr_;
- static int last_created_idx_[8];
- #endif
-
- static inline void xtrace_db_dump()
- {
- #ifdef YOSYS_XTRACE_GET_PUT
- for (int idx = 0; idx <
GetSize(global_id_storage_); idx++)
- {
- if (global_id_storage_.at(idx) ==
nullptr)
- log("#X# DB-DUMP index %d:
FREE\n", idx);
- else
- log("#X# DB-DUMP index %d:
'%s' (ref %d)\n", idx, global_id_storage_.at(idx),
global_refcount_storage_.at(idx));
- }
+ static struct global_cache_t {
+ bool destruct_guard; // POD, will be
initialized to zero
I did offer a solution to this, though:
> and if you keep all methods accessing it static calling them would
> also be fine.
Ah is this a singleton?
If it is, changing it to static looks okay.
… |
When enabling LTO on GCC, the sequence of destructing static members in
IdString class is not promised, which can lead to the cache itself being
destructed but the destruct_guard not destructed.
Put all static members of IdString to a subclass similar to current
desturct_guard_t, to mmakes the guard a real guard.
This fixes SEGV when exiting a LTO-ed GCC-built yosys binary.
Signed-off-by: Icenowy Zheng [email protected]