Page MenuHomePhabricator

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

Authored by zoecarver on Aug 22 2019, 11:29 AM.

Details

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));

Diff Detail

Event Timeline

zoecarver created this revision.Aug 22 2019, 11:29 AM
Herald added a project: Restricted Project. · View Herald Transcript
zoecarver retitled this revision from [libc++] Fix std::abs tests to [libc++] ECMAScript IdentityEscape is ambiguous (2584).Aug 22 2019, 11:32 AM
zoecarver edited the summary of this revision. (Show Details)
zoecarver added reviewers: ldionne, mclow.lists.

Friendly ping :)

ldionne requested changes to this revision.Mar 3 2020, 12:44 PM
ldionne added inline comments.
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?

This revision now requires changes to proceed.Mar 3 2020, 12:44 PM
zoecarver updated this revision to Diff 250380.Mar 14 2020, 1:11 PM
zoecarver marked an inline comment as done.
  • rebase off master
  • address review comments
Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 14 2020, 1:11 PM

@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 updated this revision to Diff 251761.Mar 20 2020, 2:16 PM
  • Remove now valid bad escape tests
zoecarver updated this revision to Diff 252743.Mar 25 2020, 9:39 PM
  • Diff from master (sorry for the bad diff)
ldionne accepted this revision.May 7 2020, 1:06 PM
ldionne added inline comments.
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).

This revision is now accepted and ready to land.May 7 2020, 1:06 PM
zoecarver marked an inline comment as done.May 7 2020, 2:22 PM
zoecarver added inline comments.
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...

This revision was automatically updated to reflect the committed changes.

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

@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.

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 added a comment.EditedMay 8 2020, 9:52 AM

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.

This revision is now accepted and ready to land.May 8 2020, 10:38 AM

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

@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 added a comment.EditedMay 9 2020, 5:12 PM

@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.

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.

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

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.