[libc++] ECMAScript IdentityEscape is ambiguous (2584)

[zoecarver]

Summary

This patch fixes 2584. Now the following works:

const std::regex r1("\\z");
assert(std::regex_match("z", r1));

I wasn't sure if the following should also work (see commented out tests):

const std::regex r1("\\z");
assert(std::regex_match("zz", r1));

[zoecarver]

Friendly ping :)

[ldionne]

libcxx/test/std/re/re.alg/re.alg.match/ecma.pass.cpp

1394

Use LWG#2584 to make it clear it's a LWG issue

1394

Also, this should be identity escapes instead of identify escape.

1401

Why are those tests commented out?

[zoecarver]

• rebase off master
• address review comments

[zoecarver]

@ldionne thanks for the review! Sorry for the delayed response/update. Last week I was super busy with another project I work on.

libcxx/test/std/re/re.alg/re.alg.match/ecma.pass.cpp

1401

I wasn't sure if they should also work (they currently don't because, while the first part is matched, it fails when it sees the extra z). Looking at it again I think that's the correct behavior (not to match).

I'm going to remove it from this patch. If it is supposed to work, I think that's a different issue than the one addressed here.

[zoecarver]

• Remove now valid bad escape tests

[zoecarver]

• Diff from master (sorry for the bad diff)

[zoecarver]

Ping.

[ldionne]

libcxx/test/std/re/re.alg/re.alg.match/ecma.pass.cpp

1401

I think regex("\\z") clearly should NOT match "zz". Basically, regex("\\z") is just equivalent to regex("z"), so whether regex("\\z") should match "zz" is the same as whether regex("z") should match "zz" (the answer is no).

[zoecarver]

libcxx/test/std/re/re.alg/re.alg.match/ecma.pass.cpp

1401

Good explanation. That makes sense. Looking at it again, I'm not sure why I ever thought that should work...

[davezarzycki]

Hello, git bisect of a private stage two builder identified this commit as breaking lld's test suite. Can we either revert this or commit a quick fix to the regular expressions in lld's test suite?

FAIL: lld :: ELF/vs-diagnostics-duplicate-split.s (63239 of 63760)
******************** TEST 'lld :: ELF/vs-diagnostics-duplicate-split.s' FAILED ********************
Script:
--
: 'RUN: at line 2';   /tmp/_update_lc/t/bin/llvm-mc -filetype=obj -triple=x86_64 /home/dave/s/lp/lld/test/ELF/vs-diagnostics-duplicate-split.s -o /tmp/_update_lc/t/tools/lld/test/ELF/Output/vs-diagnostics-duplicate-split.s.tmp.o
: 'RUN: at line 3';   not /tmp/_update_lc/t/bin/ld.lld --vs-diagnostics --shared /tmp/_update_lc/t/tools/lld/test/ELF/Output/vs-diagnostics-duplicate-split.s.tmp.o /tmp/_update_lc/t/tools/lld/test/ELF/Output/vs-diagnostics-duplicate-split.s.tmp.o -o /dev/null 2>&1 | /tmp/_update_lc/t/bin/FileCheck /home/dave/s/lp/lld/test/ELF/vs-diagnostics-duplicate-split.s
--
Exit Code: 1

Command Output (stderr):
--
/home/dave/s/lp/lld/test/ELF/vs-diagnostics-duplicate-split.s:5:11: error: CHECK: expected string not found in input
// CHECK: /tmp{{/|\\}}duplicate.s(15): error: duplicate symbol: foo
          ^
<stdin>:1:1: note: scanning from here
s(15): error: duplicate symbol: foo
^

--

********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..
FAIL: lld :: ELF/vs-diagnostics-undefined-symbol-3.s (63255 of 63760)
******************** TEST 'lld :: ELF/vs-diagnostics-undefined-symbol-3.s' FAILED ********************
Script:
--
: 'RUN: at line 2';   /tmp/_update_lc/t/bin/llvm-mc -filetype=obj -triple=x86_64-pc-linux /home/dave/s/lp/lld/test/ELF/vs-diagnostics-undefined-symbol-3.s -o /tmp/_update_lc/t/tools/lld/test/ELF/Output/vs-diagnostics-undefined-symbol-3.s.tmp1.o
: 'RUN: at line 3';   not /tmp/_update_lc/t/bin/ld.lld --vs-diagnostics /tmp/_update_lc/t/tools/lld/test/ELF/Output/vs-diagnostics-undefined-symbol-3.s.tmp1.o -o /dev/null 2>&1    | /tmp/_update_lc/t/bin/FileCheck -check-prefix=ERR -check-prefix=CHECK /home/dave/s/lp/lld/test/ELF/vs-diagnostics-undefined-symbol-3.s
: 'RUN: at line 5';   /tmp/_update_lc/t/bin/ld.lld --vs-diagnostics --warn-unresolved-symbols /tmp/_update_lc/t/tools/lld/test/ELF/Output/vs-diagnostics-undefined-symbol-3.s.tmp1.o -o /tmp/_update_lc/t/tools/lld/test/ELF/Output/vs-diagnostics-undefined-symbol-3.s.tmpout 2>&1    | /tmp/_update_lc/t/bin/FileCheck -check-prefix=WARN -check-prefix=CHECK /home/dave/s/lp/lld/test/ELF/vs-diagnostics-undefined-symbol-3.s
--
Exit Code: 1

Command Output (stderr):
--
/home/dave/s/lp/lld/test/ELF/vs-diagnostics-undefined-symbol-3.s:8:9: error: ERR: expected string not found in input
// ERR: undef3.s(15): error: undefined symbol: foo
        ^
<stdin>:1:1: note: scanning from here
s(15): error: undefined symbol: foo
^

--

********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..
FAIL: lld :: ELF/vs-diagnostics-undefined-hidden.s (63265 of 63760)
******************** TEST 'lld :: ELF/vs-diagnostics-undefined-hidden.s' FAILED ********************
Script:
--
: 'RUN: at line 2';   /tmp/_update_lc/t/bin/llvm-mc -filetype=obj -triple=x86_64 /home/dave/s/lp/lld/test/ELF/vs-diagnostics-undefined-hidden.s -o /tmp/_update_lc/t/tools/lld/test/ELF/Output/vs-diagnostics-undefined-hidden.s.tmp.o
: 'RUN: at line 3';   not /tmp/_update_lc/t/bin/ld.lld --vs-diagnostics -shared /tmp/_update_lc/t/tools/lld/test/ELF/Output/vs-diagnostics-undefined-hidden.s.tmp.o -o /dev/null 2>&1    | /tmp/_update_lc/t/bin/FileCheck /home/dave/s/lp/lld/test/ELF/vs-diagnostics-undefined-hidden.s
--
Exit Code: 1

Command Output (stderr):
--
/home/dave/s/lp/lld/test/ELF/vs-diagnostics-undefined-hidden.s:6:11: error: CHECK: expected string not found in input
// CHECK: undef.s(15): error: undefined hidden symbol: foo
          ^
<stdin>:1:1: note: scanning from here
s(15): error: undefined hidden symbol: foo
^

--

********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..
FAIL: lld :: ELF/vs-diagnostics-duplicate.s (63267 of 63760)
******************** TEST 'lld :: ELF/vs-diagnostics-duplicate.s' FAILED ********************
Script:
--
: 'RUN: at line 2';   /tmp/_update_lc/t/bin/llvm-mc -filetype=obj -triple=x86_64-unknown-linux /home/dave/s/lp/lld/test/ELF/vs-diagnostics-duplicate.s -o /tmp/_update_lc/t/tools/lld/test/ELF/Output/vs-diagnostics-duplicate.s.tmp1.o
: 'RUN: at line 3';   /tmp/_update_lc/t/bin/llvm-mc -filetype=obj -triple=x86_64-unknown-linux /home/dave/s/lp/lld/test/ELF/Inputs/vs-diagnostics-duplicate2.s -o /tmp/_update_lc/t/tools/lld/test/ELF/Output/vs-diagnostics-duplicate.s.tmp2.o
: 'RUN: at line 4';   /tmp/_update_lc/t/bin/llvm-mc -filetype=obj -triple=x86_64-unknown-linux /home/dave/s/lp/lld/test/ELF/Inputs/vs-diagnostics-duplicate3.s -o /tmp/_update_lc/t/tools/lld/test/ELF/Output/vs-diagnostics-duplicate.s.tmp3.o
: 'RUN: at line 5';   not /tmp/_update_lc/t/bin/ld.lld --vs-diagnostics /tmp/_update_lc/t/tools/lld/test/ELF/Output/vs-diagnostics-duplicate.s.tmp1.o /tmp/_update_lc/t/tools/lld/test/ELF/Output/vs-diagnostics-duplicate.s.tmp2.o /tmp/_update_lc/t/tools/lld/test/ELF/Output/vs-diagnostics-duplicate.s.tmp3.o -o /dev/null 2>&1 | /tmp/_update_lc/t/bin/FileCheck /home/dave/s/lp/lld/test/ELF/vs-diagnostics-duplicate.s
--
Exit Code: 1

Command Output (stderr):
--
/home/dave/s/lp/lld/test/ELF/vs-diagnostics-duplicate.s:8:11: error: CHECK: expected string not found in input
// CHECK: duplicate.s(15): error: duplicate symbol: bar
          ^
<stdin>:1:1: note: scanning from here
s(15): error: duplicate symbol: bar
^

--

********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..
FAIL: lld :: ELF/vs-diagnostics-dynamic-relocation.s (63280 of 63760)
******************** TEST 'lld :: ELF/vs-diagnostics-dynamic-relocation.s' FAILED ********************
Script:
--
: 'RUN: at line 2';   /tmp/_update_lc/t/bin/llvm-mc -filetype=obj -triple=x86_64-pc-linux /home/dave/s/lp/lld/test/ELF/vs-diagnostics-dynamic-relocation.s -o /tmp/_update_lc/t/tools/lld/test/ELF/Output/vs-diagnostics-dynamic-relocation.s.tmp.o
: 'RUN: at line 3';   not /tmp/_update_lc/t/bin/ld.lld -shared --vs-diagnostics /tmp/_update_lc/t/tools/lld/test/ELF/Output/vs-diagnostics-dynamic-relocation.s.tmp.o -o /dev/null 2>&1 | /tmp/_update_lc/t/bin/FileCheck /home/dave/s/lp/lld/test/ELF/vs-diagnostics-dynamic-relocation.s
--
Exit Code: 1

Command Output (stderr):
--
/home/dave/s/lp/lld/test/ELF/vs-diagnostics-dynamic-relocation.s:5:11: error: CHECK: expected string not found in input
// CHECK: dyn.s(15): error: can't create dynamic relocation R_X86_64_64 against local symbol in readonly segment; recompile object files with -fPIC or pass '-Wl,-z,notext' to allow text relocations in the output
          ^
<stdin>:1:1: note: scanning from here
s(15): error: can't create dynamic relocation R_X86_64_64 against local symbol in readonly segment; recompile object files with -fPIC or pass '-Wl,-z,notext' to allow text relocations in the output
^

--

********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..

10 warning(s) in tests
********************
Failing Tests (5):
  lld :: ELF/vs-diagnostics-duplicate-split.s
  lld :: ELF/vs-diagnostics-duplicate.s
  lld :: ELF/vs-diagnostics-dynamic-relocation.s
  lld :: ELF/vs-diagnostics-undefined-hidden.s
  lld :: ELF/vs-diagnostics-undefined-symbol-3.s


Testing Time: 144.74s
  Unsupported Tests  : 11836
  Expected Passes    : 51763
  Expected Failures  :   156
  Unexpected Failures:     5
FAILED: CMakeFiles/check-all

[zoecarver]

@davezarzycki thanks for letting me know. I'll see if I can easily reproduce those failures on my machine. If so, I'll submit a fix, otherwise, I'll revert. Either way, I'll try to have it done in the next ~hour.

[davezarzycki]

Great. Thanks. As a friendly reminder, you’ll need to configure the first stage to use libcxx and also have clang to prefer libcxx, otherwise lld will just use whatever CMake autodetects/decides.

[zoecarver]

Thanks for the reminder :)

I'm going to revert. It's taking longer than expected to get the test infrastructure set up properly. I'll try to fix those tests and re-commit later today.

[zoecarver]

Reverted in 26466efe0879dde8772bc336d81fcccaa9514b3a.

[davezarzycki]

In D66610#2027215, @zoecarver wrote:

Reverted in 26466efe0879dde8772bc336d81fcccaa9514b3a.

Thanks. When you have a new patch ready, CC me and I can preflight it

[zoecarver]

@davezarzycki what's the configuration flag to get filecheck / lld to use the just-built libcxx? I thought it would be LLVM_ENABLE_LIBCXX but that doesn't appear to work.

[davezarzycki]

In D66610#2028359, @zoecarver wrote:

@davezarzycki what's the configuration flag to get filecheck / lld to use the just-built libcxx? I thought it would be LLVM_ENABLE_LIBCXX but that doesn't appear to work.

I don't know of such a flag. My two stage setup actually installs the first stage and then uses the first stage to compile/test the second stage.

Also and for what it's worth, "just built" doesn't really make sense for libcxx since so much of the standard C++ library is at compile time. If you want to avoid two stages and use the latest libcxx, that would require some kind of build flag to override the system C++ headers and use the ones it the git checkout.

[zoecarver]

I don't know of such a flag. My two stage setup actually installs the first stage and then uses the first stage to compile/test the second stage.

Fair enough. I'll try to set up a VM for testing (which I honestly should have done long ago) and install/test this weekend.

Also and for what it's worth, "just built" doesn't really make sense for libcxx since so much of the standard C++ library is at compile time. If you want to avoid two stages and use the latest libcxx, that would require some kind of build flag to override the system C++ headers and use the ones it the git checkout.

That or have two builds (which I tried at one point and I suspect _could_ be done if I knew the correct cmake flags). Libcxx is mostly headers, yes, but there are enough source files to cause problems when the headers and libraries don't match. That being said, you're right, "just built" doesn't convey the thing I meant.

[davezarzycki]

In D66610#2028442, @zoecarver wrote:

I don't know of such a flag. My two stage setup actually installs the first stage and then uses the first stage to compile/test the second stage.

Fair enough. I'll try to set up a VM for testing (which I honestly should have done long ago) and install/test this weekend.

I can see the advantage of a VM, but personally, I don't go that far. Non-standard install prefixes are handy sometimes. For example, have the first phase build and install with -DCMAKE_INSTALL_PREFIX=/some/other/path and then export PATH=/some/other/path/bin:$PATH before running the second phase. When you're done, rm -rf /some/other/path. This assumes, of course, that one is already using -DCMAKE_C_COMPILER=clang and -DCMAKE_CXX_COMPILER=clang++

Here are all the flags I use in case it matter. And yes, the various install prefixes are overridden during my phased tests.

cmake -G Ninja \

-DCMAKE_INSTALL_PREFIX=/my/production/compiler/path \
-DLIBCXX_INSTALL_PREFIX=/my/production/compiler/path \
-DLIBCXX_INSTALL_HEADER_PREFIX=/my/production/compiler/path \
-DCMAKE_C_COMPILER=clang \
-DCMAKE_CXX_COMPILER=clang++ \
-DCMAKE_C_FLAGS="$CFLAGS" \
-DCMAKE_CXX_FLAGS="$CXXFLAGS" \
-DCMAKE_LINKER=ld.lld \
-DLLVM_TARGETS_TO_BUILD="X86;AArch64;PowerPC;RISCV" \
-DLLVM_INCLUDE_GO_TESTS=FALSE \
-DLLVM_TOOL_LTO_BUILD=FALSE \
-DLLVM_ENABLE_PROJECTS="clang;lld;libcxxabi;libcxx;libc" \
-DLLVM_INCLUDE_GO_TESTS=FALSE \
-DLLVM_LIBDIR_SUFFIX=64 \
-DLLVM_USE_LINKER=lld \
-DLLVM_ENABLE_LIBCXX=TRUE \
-DCLANG_DEFAULT_LINKER=lld \
-DCLANG_DEFAULT_CXX_STDLIB="libc++" \
-DLIBCXX_ENABLE_SHARED=FALSE \
-DLIBCXXABI_ENABLE_SHARED=FALSE \
-DLIBUNWIND_ENABLE_SHARED=FALSE \
-DLLVM_LIBC_ENABLE_LINTING=FALSE

[ldionne]

FWIW, this patch should not require modifying the regular expressions in LLD (or anywhere). If it does, it means the patch is most likely wrong.

[ldionne]

Requesting changes since this has been reverted, and it needs changes to be committed again.

libcxx/include/regex

if (__first != __last)
             ++__first;
             break;
         case 'W':
             __ml = __start_matching_list(true);
             __ml->__add_class(ctype_base::alnum);
             __ml->__add_char('_');
             ++__first;
             break;
+        default:
+            __ml = __start_matching_list(true);
+            ++__first;
         }
     }
     return __first;
 }
 
 template <class _CharT, class _Traits>
 template <class _ForwardIterator>
 _ForwardIterator

libcxx/test/std/re/re.alg/re.alg.match/ecma.pass.cpp

std::locale::global(std::locale("C"));
         assert(m.prefix().second == m[0].first);
         assert(!m.suffix().matched);
         assert(m.suffix().first == m[0].second);
         assert(m.suffix().second == s + std::char_traits<wchar_t>::length(s));
         assert(m.length(0) >= 0 && static_cast<size_t>(m.length(0)) == std::char_traits<wchar_t>::length(s));
         assert(m.position(0) == 0);
         assert(m.str(0) == s);
     }
+    {
+        // LWG#2584: identity escapes. Match exact chars/strings.
+        const std::regex r1("\\z");
+        assert(std::regex_match("z", r1));
+
+        const std::regex r2("\\zz");
+        assert(std::regex_match("zz", r2));
+
+        const std::regex r3("\\zx");
+        assert(std::regex_match("zx", r3));
+    }
 
   return 0;
 }

libcxx/test/std/re/re.regex/re.regex.construct/bad_escape.pass.cpp

try {
         result = (ex.code() == std::regex_constants::error_escape);
     }
     return result;
 }
 
 int main(int, char**)
 {
     assert(error_escape_thrown("[\\a]"));
-    assert(error_escape_thrown("\\a"));
     assert(error_escape_thrown("\\"));
 
     assert(error_escape_thrown("[\\e]"));
-    assert(error_escape_thrown("\\e"));
 
     assert(error_escape_thrown("[\\c:]"));
-    assert(error_escape_thrown("\\c:"));
-    assert(error_escape_thrown("\\c"));
     assert(!error_escape_thrown("[\\cA]"));
     assert(!error_escape_thrown("\\cA"));
 
 
   return 0;
 }

libcxx/www/cxx1z_status.html

<!--   <I>Note: "NAD" means that the issue was deemed "Not a defect"</I> -->
   <tr><td><a href="https://wg21.link/LWG2544">2544</a></td><td>istreambuf_iterator(basic_streambuf<charT, traits>* s) effects unclear when s is 0</td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2556">2556</a></td><td>Wide contract for future::share()</td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2562">2562</a></td><td>Consistent total ordering of pointers by comparison functors</td><td>Issaquah</td><td></td></tr>
   <tr><td><a href="https://wg21.link/LWG2567">2567</a></td><td>Specification of logical operator traits uses BaseCharacteristic, which is defined only for UnaryTypeTraits and BinaryTypeTraits</td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2568">2568</a></td><td>[fund.ts.v2] Specification of logical operator traits uses BaseCharacteristic, which is defined only for UnaryTypeTraits and BinaryTypeTraits</td><td>Issaquah</td><td></td></tr>
   <tr><td><a href="https://wg21.link/LWG2569">2569</a></td><td>conjunction and disjunction requirements are too strict</td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2570">2570</a></td><td>[fund.ts.v2] conjunction and disjunction requirements are too strict</td><td>Issaquah</td><td></td></tr>
   <tr><td><a href="https://wg21.link/LWG2578">2578</a></td><td>Iterator requirements should reference iterator traits</td><td>Issaquah</td><td>Complete</td></tr>
-  <tr><td><a href="https://wg21.link/LWG2584">2584</a></td><td><regex> ECMAScript IdentityEscape is ambiguous</td><td>Issaquah</td><td></td></tr>
+  <tr><td><a href="https://wg21.link/LWG2584">2584</a></td><td><regex> ECMAScript IdentityEscape is ambiguous</td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2587">2587</a></td><td>"Convertible to bool" requirement in conjunction and disjunction</td><td>Issaquah</td><td>Resolved by 2567</td></tr>
   <tr><td><a href="https://wg21.link/LWG2588">2588</a></td><td>[fund.ts.v2] "Convertible to bool" requirement in conjunction and disjunction</td><td>Issaquah</td><td></td></tr>
   <tr><td><a href="https://wg21.link/LWG2589">2589</a></td><td>match_results can't satisfy the requirements of a container</td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2591">2591</a></td><td>std::function's member template target() should not lead to undefined behaviour</td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2598">2598</a></td><td>addressof works on temporaries</td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2664">2664</a></td><td>operator/ (and other append) semantics not useful if argument has root</td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2665">2665</a></td><td>remove_filename() post condition is incorrect</td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2672">2672</a></td><td>Should is_empty use error_code in its specification?</td><td>Issaquah</td><td>Complete</td></tr>