[ESI] Extend instead of override CMAKE_CXX_FLAGS

[mortbopet]

No description provided.

[teqdruid]

The problem with this approach is that a flag disabling exceptions may be in the flags already. So appending isn't enough. I think I looked into it awhile back, but cmake should really have a function to deal with this.

…

On Tue, Jun 10, 2025, 9:57 AM Morten Borup Petersen < ***@***.***> wrote: @mortbopet <https://github.com/mortbopet> requested your review on: #8551 <#8551> [ESI] Extend instead of override CMAKE_CXX_FLAGS as a code owner. — Reply to this email directly, view it on GitHub <#8551 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALNXYDXYEWT24QIT32VB2D3C3P3NAVCNFSM6AAAAAB67ZBHR6VHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJYGA3TMMJWGIZDEMI> . You are receiving this because your review was requested.Message ID: ***@***.***>

[teqdruid]

The problem with this approach is that a flag disabling exceptions may be in the flags already. So appending isn't enough.

Which is the situation when the runtime is compiled in a CIRCT context.

[mortbopet]

Sure, but that's not a blank check to override all CXX flags. In that case, the ESI runtime should scan the existing flags and remove any flags that it definitely does not want to build with.
An example - in an internal build, it is required that /FS is set on all targets, when using sccache.
This, unfortunately, is impossible when ESI overrides all flags - and we do not want to manually add /FS to the one-to-many targets which the ESI runtime builds (i.e. the runtime itself, perhaps zlib if it's also building that, ...).

[teqdruid]

Oh sure. What it currently does is pretty bad. I'm not arguing against changing it. There's some cmake code somewhere else in CIRCT (or at least used to be) which did a string replace based removal... I never liked that approach but we can use it here since it's better than what we're currently doing. Can you think of any other way? I'm surprised that cmake doesn't have functionality for exactly this purpose. Is there something I'm missing?

…

On Wed, Jun 11, 2025, 2:45 AM Morten Borup Petersen < ***@***.***> wrote: *mortbopet* left a comment (llvm/circt#8551) <#8551 (comment)> Sure, but that's not a blank check to override all CXX flags. In that case, the ESI runtime should scan the existing flags and remove any flags that it definitely does *not* want to build with. An example - in an internal build, it is required that /FS is set on *all* targets, when using sccache. This, unfortunately, is impossible when ESI overrides all flags - and we do *not* want to manually add /FS to the one-to-many targets which the ESI runtime builds (i.e. the runtime itself, perhaps zlib if it's also building that, ...). — Reply to this email directly, view it on GitHub <#8551 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALNXYDN2ES3WVO2NROFADL3C7GALAVCNFSM6AAAAAB67ZBHR6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDSNRRGQ2DAMJWHE> . You are receiving this because your review was requested.Message ID: ***@***.***>

[mortbopet]

@teqdruid unfortunately, i am not aware of anything better... added a regex replace for exception-related flags, hopefully that is a sufficient enough catch-all.

[teqdruid]

LGTM! Thanks