Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[C++] Span implementation includes non-conformant SFINAE #40252

Closed
vyasr opened this issue Feb 26, 2024 · 2 comments · Fixed by #40253
Closed

[C++] Span implementation includes non-conformant SFINAE #40252

vyasr opened this issue Feb 26, 2024 · 2 comments · Fixed by #40253
Assignees
Milestone

Comments

@vyasr
Copy link
Contributor

vyasr commented Feb 26, 2024

Describe the bug, including details regarding any error messages, version, and platform.

The span polyfill includes a SFINAE overload for selecting which types a span should be constructible from. Currently this implementation includes code that is technically in violation of the standard because it requires querying another constructor of span<T> at a point when the type is still incomplete. Other span implementations avoid this kind of templating: here are 1) Microsoft's GSL implementation and 2) cudf's implementation in RAPIDS.

It seems like both clang and gcc allow this particular code to compile from my experiments, but nvcc does not. Here is an example with the compiler explorer demonstrating the expected failure mode with an even simpler construct (also showing that the current code compiles, albeit unexpectedly). Prior to Arrow 15 this was not a major issue unless external code tried to use Arrow's span directly. However, after the merge of #38027 span.h is transitively included by arrow/api.h, which means that essentially any code including Arrow headers will no longer compile on device. Here's a minimal example:

#include <arrow/api.h>
int main() { return 0; }

And attempting to compile it:

(rapids) coder _ ~/local/arrow/span_test $ nvcc test.cu  -I ../cpp/install/include/
../cpp/install/include/arrow/result.h(263): warning #2810-D: ignoring return value type with "nodiscard" attribute

Remark: The warnings can be suppressed with "-diag-suppress <warning-number>"

../cpp/install/include/arrow/util/span.h(62): error: incomplete type is not allowed

../cpp/install/include/arrow/record_batch.h(335): warning #2810-D: ignoring return value type with "nodiscard" attribute

../cpp/install/include/arrow/record_batch.h(338): warning #2810-D: ignoring return value type with "nodiscard" attribute

../cpp/install/include/arrow/table.h(244): warning #611-D: overloaded virtual function "arrow::RecordBatchReader::ReadNext" is only partially overridden in class "arrow::TableBatchReader"

1 error detected in the compilation of "test.cu".

This issue may be fixed using a different expression analogous to those shown in the other span implementations linked above.

Component(s)

C++

@vyasr
Copy link
Contributor Author

vyasr commented Feb 26, 2024

CC @galipremsagar

@kou kou changed the title Span implementation includes non-conformant SFINAE [C++] Span implementation includes non-conformant SFINAE Feb 26, 2024
@jorisvandenbossche jorisvandenbossche added this to the 15.0.1 milestone Feb 27, 2024
@raulcd raulcd modified the milestones: 15.0.1, 16.0.0 Mar 1, 2024
pitrou pushed a commit that referenced this issue Mar 13, 2024
…lation with nvcc (#40253)

### Rationale for this change

The current code uses an incomplete type in a SFINAE construct.

Closes #40252 

### What changes are included in this PR?

This PR changes the way that the type is specified to avoid any UB.

### Are these changes tested?

I tested locally that code that originally failed to compile with nvcc now compiles successfully. The same code also compiles under clang and gcc. This is a minimal reproducer:
```
#include <arrow/api.h>
#include <vector>

int main() {
  arrow::util::span<int> x{std::vector<int>{1, 2, 3}};
}
```

I did not include it here because it is a compile-time rather than runtime issue, and because at present it only manifests on nvcc.

### Are there any user-facing changes?

No.

* GitHub Issue: #40252

Authored-by: Vyas Ramasubramani <vyasr@nvidia.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@pitrou
Copy link
Member

pitrou commented Mar 13, 2024

Issue resolved by pull request 40253
#40253

@raulcd raulcd modified the milestones: 16.0.0, 15.0.2 Mar 13, 2024
raulcd pushed a commit that referenced this issue Mar 13, 2024
…lation with nvcc (#40253)

### Rationale for this change

The current code uses an incomplete type in a SFINAE construct.

Closes #40252 

### What changes are included in this PR?

This PR changes the way that the type is specified to avoid any UB.

### Are these changes tested?

I tested locally that code that originally failed to compile with nvcc now compiles successfully. The same code also compiles under clang and gcc. This is a minimal reproducer:
```
#include <arrow/api.h>
#include <vector>

int main() {
  arrow::util::span<int> x{std::vector<int>{1, 2, 3}};
}
```

I did not include it here because it is a compile-time rather than runtime issue, and because at present it only manifests on nvcc.

### Are there any user-facing changes?

No.

* GitHub Issue: #40252

Authored-by: Vyas Ramasubramani <vyasr@nvidia.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants