[patch][libcxx] fix C++98 binders when binding a ref-typed argument

Jesse_Zhang · August 3, 2020, 5:03am

Hi LLVM hackers,

PFA a tiny patch that fixes a bug I found in libcxx.

Here's a minimal repro (obligatory godbolt:
Compiler Explorer):

====>8====
#include <functional>

namespace jesse {
struct R {
  int foo() const { return 1; }
  int bar() const { return 2; }
  int foo() { return 42; }
  int bar() { return 13; }
};

struct S : R {
int i_;
};

int f(int, R& r) { return r.foo(); }
int g(R& r, int) { return r.bar(); }

int h(bool b) {
  S s;
  if (b)
    return bind2nd(std::ptr_fun(f), s)(0); // 42
  else
    return bind1st(std::ptr_fun(g), s)(0); // 13
}
} // namespace jesse
====8<====

When I compile with `-Wall -O2 -std=gnu++98 -stdlib=libc++` using Clang
11, it fails with "no matching constructor" for both call sites to the
binder helper functions. The attached patch fixes this by converting the
arguments before constructing the binder objects.

A couple quick questions:

1. I'd like to include a failing test in the patch, but am not
sufficiently familiar with the code base to know where to add one. Also,
a "failing" test here would mean it fails compilation, which is
different from a failed assertion in what I'm more accustomed to in my
day job, what's the best way to go about adding a regression /
integration test?

2. I tried running clang-format over my patch but noticed it results in
a massive diff so I held off formatting, is that acceptable?

3. I tried doing some archaeology to see whether this was a regression,
but I could only find a "genesis commit" where Howard imported libcxx
wholesale on May 11 2010 ("libcxx initial import", SVN r103490, Git
monorepo commit 3e519524c1186). Is the version control history before
that published somewhere?

P.S. Probably should have asked this question first: I'm new here (this
is literally my first email to any LLVM mailing list), so if this is the
wrong list to send patches please gently point me to the right one.

Cheers,
Jesse

0001-Fix-C-98-binders-when-binding-a-ref-typed-argument.patch (1.36 KB)

Jesse_Zhang · August 11, 2020, 2:06am

Hi LLVM hackers,

PFA a tiny patch that fixes a bug I found in libcxx.

Here's a minimal repro (obligatory godbolt:
Compiler Explorer):

====>8====
#include <functional>

namespace jesse {
struct R {
  int foo() const { return 1; }
  int bar() const { return 2; }
  int foo() { return 42; }
  int bar() { return 13; }
};

struct S : R {
int i_;
};

int f(int, R& r) { return r.foo(); }
int g(R& r, int) { return r.bar(); }

int h(bool b) {
  S s;
  if (b)
    return bind2nd(std::ptr_fun(f), s)(0); // 42
  else
    return bind1st(std::ptr_fun(g), s)(0); // 13
}
} // namespace jesse
====8<====

When I compile with `-Wall -O2 -std=gnu++98 -stdlib=libc++` using Clang
11, it fails with "no matching constructor" for both call sites to the
binder helper functions. The attached patch fixes this by converting the
arguments before constructing the binder objects.

A couple quick questions:

1. I'd like to include a failing test in the patch, but am not
sufficiently familiar with the code base to know where to add one. Also,
a "failing" test here would mean it fails compilation, which is
different from a failed assertion in what I'm more accustomed to in my
day job, what's the best way to go about adding a regression /
integration test?

2. I tried running clang-format over my patch but noticed it results in
a massive diff so I held off formatting, is that acceptable?

3. I tried doing some archaeology to see whether this was a regression,
but I could only find a "genesis commit" where Howard imported libcxx
wholesale on May 11 2010 ("libcxx initial import", SVN r103490, Git
monorepo commit 3e519524c1186). Is the version control history before
that published somewhere?

P.S. Probably should have asked this question first: I'm new here (this
is literally my first email to any LLVM mailing list), so if this is the
wrong list to send patches please gently point me to the right one.

Cheers,
Jesse

0001-Fix-C-98-binders-when-binding-a-ref-typed-argument.patch (1.36 KB)

Jesse_Zhang · March 4, 2021, 1:02am

Hi LLVM hackers,

PFA a tiny patch that fixes a bug I found in libcxx.

Here's a minimal repro (obligatory godbolt:
Compiler Explorer):

====>8====
#include <functional>

namespace jesse {
struct R {
  int foo() const { return 1; }
  int bar() const { return 2; }
  int foo() { return 42; }
  int bar() { return 13; }
};

struct S : R {
int i_;
};

int f(int, R& r) { return r.foo(); }
int g(R& r, int) { return r.bar(); }

int h(bool b) {
  S s;
  if (b)
    return bind2nd(std::ptr_fun(f), s)(0); // 42
  else
    return bind1st(std::ptr_fun(g), s)(0); // 13
}
} // namespace jesse
====8<====

When I compile with `-Wall -O2 -std=gnu++98 -stdlib=libc++` using Clang
11, it fails with "no matching constructor" for both call sites to the
binder helper functions. The attached patch fixes this by converting the
arguments before constructing the binder objects.

A couple quick questions:

1. I'd like to include a failing test in the patch, but am not
sufficiently familiar with the code base to know where to add one. Also,
a "failing" test here would mean it fails compilation, which is
different from a failed assertion in what I'm more accustomed to in my
day job, what's the best way to go about adding a regression /
integration test?

2. I tried running clang-format over my patch but noticed it results in
a massive diff so I held off formatting, is that acceptable?

3. I tried doing some archaeology to see whether this was a regression,
but I could only find a "genesis commit" where Howard imported libcxx
wholesale on May 11 2010 ("libcxx initial import", SVN r103490, Git
monorepo commit 3e519524c1186). Is the version control history before
that published somewhere?

P.S. Probably should have asked this question first: I'm new here (this
is literally my first email to any LLVM mailing list), so if this is the
wrong list to send patches please gently point me to the right one.

Cheers,
Jesse

0001-Fix-C-98-binders-when-binding-a-ref-typed-argument.patch (1.36 KB)

Quuxplusone · March 4, 2021, 1:20am

Hi LLVM hackers,

PFA a tiny patch that fixes a bug I found in libcxx.

FYI (and in answer to your last question), there is a libc+±specific mailing list at <libcxx-dev@lists.llvm.org>.

The usual workflow is to post patches-for-review to Phabricator at
https://reviews.llvm.org/differential/

; do you have the capability to do that? (Otherwise I dunno, maybe Louis Dionne will take it from here?)

Here’s a minimal repro (obligatory godbolt:
https://godbolt.org/z/3zra7s):
[…]

The attached patch fixes this by converting the
arguments before constructing the binder objects.

Looks reasonable to me. Your patch seems to be simply improving libc++'s adherence to to the actual C++11 specification of bind1st/bind2nd in
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf

Notice that bind1st/bind2nd did not exist in C++98, so libc++'s support of them in C++98 mode is a pure extension.
Notice that bind1st/bind2nd were “deprecated on arrival” in C++11, and are no longer standard C++ as of C++17.
So you’re asking to “fix” an extension, to C++98, which has never existed non-deprecated in the paper standard, which hasn’t existed (even in deprecated form) for two whole standard revisions at this point.

BUT… by the same tokens, the stakes of fixing this must be pretty low, and your patch seems obviously “correct” to me.

I’d like to include a failing test in the patch, but am not
sufficiently familiar with the code base to know where to add one.

You should add new passing tests, to libcxx/test/std/depr/depr.lib.binders/.
The bug is that you have some code that SHOULD compile but doesn’t (because of the bug), right? So the new test case should test that your code DOES compile.

I tried running clang-format over my patch but noticed it results in
a massive diff so I held off formatting, is that acceptable?

Yes.

I tried doing some archaeology to see whether this was a regression,
but I could only find a “genesis commit” where Howard imported libcxx
wholesale on May 11 2010 (“libcxx initial import”, SVN r103490, Git
monorepo commit 3e519524c1186). Is the version control history before
that published somewhere?

I don’t know, and am curious; but in this particular case it certainly doesn’t matter to the fate of your patch.

HTH,
Arthur

Tim_Song · March 4, 2021, 2:10am

Looks reasonable to me. Your patch seems to be simply improving libc++'s adherence to to the actual C++11 specification of bind1st/bind2nd in
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf

- Notice that bind1st/bind2nd did not exist in C++98, so libc++'s support of them in C++98 mode is a pure extension.

[citation needed]

- Notice that bind1st/bind2nd were "deprecated on arrival" in C++11, and are no longer standard C++ as of C++17.
So you're asking to "fix" an extension, to C++98, which has never existed non-deprecated in the paper standard, which hasn't existed (even in deprecated form) for two whole standard revisions at this point.
BUT... by the same tokens, the stakes of fixing this must be pretty low, and your patch seems obviously "correct" to me.

The specification, which will happily perform anything up to a
reinterpret_cast, seems obviously defective.

Jesse_Zhang · March 16, 2021, 3:28am

Hi Arthur,

FYI (and in answer to your last question), there is a libc++-specific mailing list at <libcxx-dev@lists.llvm.org>.

Thanks for your prompt response! I *did* try asking on libcxx-dev half a
year ago [1], but there doesn't seem to be a big audience there. I wish
I'd tried cfe-dev sooner.

The usual workflow is to post patches-for-review to Phabricator at
https://reviews.llvm.org/differential/
; do you have the capability to do that? (Otherwise I dunno, maybe Louis Dionne will take it from here?)

Embarrased to say I haven't tried that (will do). I come from more
old-fashioned communities (Linux / Postgres) where contributions are
patches, and discussions are primarily on IRC / listserv.

Here's a minimal repro (obligatory godbolt:
Compiler Explorer):
[...]

The attached patch fixes this by converting the
arguments before constructing the binder objects.

Looks reasonable to me. Your patch seems to be simply improving libc++'s adherence to to the actual C++11 specification of bind1st/bind2nd in
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf

- Notice that bind1st/bind2nd did not exist in C++98, so libc++'s support of them in C++98 mode is a pure extension.
- Notice that bind1st/bind2nd were "deprecated on arrival" in C++11, and are no longer standard C++ as of C++17.
So you're asking to "fix" an extension, to C++98, which has never existed non-deprecated in the paper standard, which hasn't existed (even in deprecated form) for two whole standard revisions at this point.
BUT... by the same tokens, the stakes of fixing this must be pretty low, and your patch seems obviously "correct" to me.

I did some due diligence to get my hands on a copy of C++98 (and made
some new friends), and the binders are specified in the standard [2].

1. I'd like to include a failing test in the patch, but am not
sufficiently familiar with the code base to know where to add one.

You should add new *passing* tests, to libcxx/test/std/depr/depr.lib.binders/.
The bug is that you have some code that SHOULD compile but doesn't (because of the bug), right? So the new test case should test that your code DOES compile.

/me nods incessantly.

Sorry for not using clearer words. I meant a test that would fail
*before* the code addition -- it goes without saying it passes after the
fix -- hence the term "failing test".

I was holding onto this email thinking I would send my response with a
new patch, but I'm still struggling a bit with adding a test, mostly
because the standard text actually requires a functional cast, which is
equivalent to a C-style cast, so I'm seeing a fairly simple test now
needing to cover 5*2 = 10 cases (const_cast, static_cast, static_cast +
const_cast, reinterpret_cast, reinterpret_cast + const_cast, double that
for 2nd arg), so I decide to respond with what I have first.

Cheers,
Jesse

[1] [libcxx-dev] [patch][libcxx] fix C++98 binders when binding a ref-typed argument
[2] ISO/IEC 14882:1998(E), subsection "20.3.6 Binders [lib.binders]"