Avoid accessing unset optional, workaround for #100095

[alexfh]

This patch avoids accessing an unset std::optional<>, which is a part of the manifestation of #100095. The other part is an assertion failure that is not addressed here. This is not a proper fix, but enables Clang to continue working with more libc++ runtime checks enabled (specifically, -D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_FAST, which checks access to unset optionals among other things). A proper fix is being discussed on #100095.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Alexander Kornienko (alexfh)

Changes

This is not a proper fix, but enables Clang to continue working with more libc++ runtime checks enabled.

---

Full diff: https://github.com/llvm/llvm-project/pull/100408.diff

1 Files Affected:

• (modified) clang/lib/Sema/SemaTemplateDeduction.cpp (+3-1)

diff --git a/clang/lib/Sema/SemaTemplateDeduction.cpp b/clang/lib/Sema/SemaTemplateDeduction.cpp
index b7b857ebf804b..db7f233dcef73 100644
--- a/clang/lib/Sema/SemaTemplateDeduction.cpp
+++ b/clang/lib/Sema/SemaTemplateDeduction.cpp
@@ -951,9 +951,11 @@ class PackDeductionScope {
 
     // Skip over the pack elements that were expanded into separate arguments.
     // If we partially expanded, this is the number of partial arguments.
+    // FIXME: `&& FixedNumExpansions` is a workaround for UB described in
+    // https://github.com/llvm/llvm-project/issues/100095
     if (IsPartiallyExpanded)
       PackElements += NumPartialPackArgs;
-    else if (IsExpanded)
+    else if (IsExpanded && FixedNumExpansions)
       PackElements += *FixedNumExpansions;
 
     for (auto &Pack : Packs) {

[shafik]

Can you please add a reference to #100095 in the summary so folks just reading the git log have more context w/o going to the commit itself.

I would like to see a more flushed out long-term plan for fixing this properly but I think it makes sense.

[alexfh]

Can you please add a reference to #100095 in the summary so folks just reading the git log have more context w/o going to the commit itself.

I would like to see a more flushed out long-term plan for fixing this properly but I think it makes sense.

I've added more details to the commit message and another reference to #100095 (one was already in the short summary).

[mizvekov]

I don't believe we currently have any code coverage at all for the line affected.

So with this change, which we don't fully understand, we could be breaking currently working code, and we could not even be fixing the known broken cases, as there could be further UB down the line.

I wouldn't object this patch, if we don't have other means of figuring this out soon.
But I'd like to try first getting a workaround implemented in libc++, I think that would be preferable.
If we don't get an answer about this soon, sure I'd say let's go ahead.

But in that case, I'd like to see at least a test added to this PR.

[alexfh]

... I'd like to see at least a test added to this PR.

See #100556. The test would fail with assertions enabled (thus, marked XFAIL: asserts), and when compiled against hardened libc++ (e.g. libc++ built with -D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_FAST).

[ilya-biryukov]

and when compiled against hardened libc++

Could we merge the test into this change? I think we could then land this as a temporary workaround indeed.
I'll keep looking into fixing this proper and will remove the XFAIL line when appropriate.

It seems that having this is strictly better than what we have today (adds real code where it fails, fails with an assertion when they are enabled, produces some deterministic, even if incorrect, result, rather than having a UB).

[alexfh]

Could we merge the test into this change? I think we could then land this as a temporary workaround indeed. I'll keep looking into fixing this proper and will remove the XFAIL line when appropriate.

I've added the test to the change and verified it:

• fails without the SemaTemplateDeduction.cpp change, when compiled against libc++ with hardening enabled and with LLVM assertions disabled
• passes with the SemaTemplateDeduction.cpp change, when compiled against libc++ with hardening enabled and with LLVM assertions disabled
• XFAILs with the SemaTemplateDeduction.cpp change, when compiled against libc++ with hardening enabled and with LLVM assertions enabled.

Unfortunately, we don't have a buildbot with LLVM tests built against libc++ + hardening, but we're going to test it routinely internally at Google.

[ilya-biryukov]

LGTM, since it now has a test that executes this code path and changes from UB to defined (even if undesired) behavior.

As mentioned earlier, I'm working on a proper fix for those cases