chapel-lang / chapel

a Productive Parallel Programming Language
https://chapel-lang.org
Other
1.77k stars 416 forks source link

Issues with Chapel-generated C headers for exported procs #24668

Open twesterhout opened 5 months ago

twesterhout commented 5 months ago

I've been using a pattern where I first hand-write a C header file such as

// header.h
#pragma once
void foo(void);

and then use it for export procs instead of relying on the Chapel-generated header:

require "header.h";
export proc foo() { ... }

This has been causing me some trouble for a few reasons:

@bradcray pointed out that I really shouldn't be doing this, and that I should instead share what's wrong with the Chapel-generated headers that's causing me to write my own.

What makes automatically generated headers unusable for me

Here's what Chapel currently generates for my library (I've removed some of the functions to limit the size of the code block):

#include "stdchpl.h"
#include "wctype.h"
#include "ctype.h"
#include "/nix/store/d63pi2kqlyp0wy6zncnip089qfjadz4g-lattice-symmetries-haskell-2.2.0-lib/include/lattice_symmetries_functions.h"
#include "Timing.h"
#include "ConcurrentQueue.h"
void chpl__init_BatchedOperator(int64_t _ln,
                                int32_t _fn);
void ls_chpl_matrix_vector_product_csr_i32_c128(int64_t numberRows,
                                                int64_t numberCols,
                                                int64_t numberNonZero,
                                                _complex128 * matrixElements,
                                                int32_t * rowOffsets,
                                                int32_t * colIndices,
                                                _complex128 * x,
                                                _complex128 * y,
                                                int64_t numTasks);
void chpl__init_CommonParameters(int64_t _ln,
                                 int32_t _fn);
void ls_chpl_matrix_vector_product_f64(ls_hs_operator * matrixPtr,
                                       int32_t numVectors,
                                       _real64 * xPtr,
                                       _real64 * yPtr);
void ls_chpl_extract_diag_c128(ls_hs_operator * matrixPtr,
                               chpl_external_array * diag);
void ls_chpl_display_timings(void);
void ls_chpl_experimental_axpy_c128(int64_t size,
                                    _real64 alpha_re,
                                    _real64 alpha_im,
                                    _complex128 * xs,
                                    _complex128 * ys,
                                    _complex128 * zs);
void chpl__init_Vector(int64_t _ln,
                       int32_t _fn);

So which parts cause trouble?

  1. The biggest issue is that all const annotations are gone. E.g. in Chapel the ls_chpl_extract_diag_c128 function is declared as
    export proc ls_chpl_extract_diag_c128(matrixPtr : c_ptrConst(ls_hs_operator),
                                         diag : c_ptr(chpl_external_array)) { ... }

    but the header declares the matrixPtr parameter as ls_hs_operator *.

  2. An include with an absolute path to the header makes it very difficult to distribute the library. It's possible to work around the problem with replacing all absolute paths in a script, so it's not as big a deal as 1.
  3. The header contains a few includes of the internal Chapel files. This means that the code using the library needs Chapel as a dependency even if the Chapel runtime is linked statically into the shared library. Again, I could probably remove these includes with sed, but that's an extra step.
  4. The _real64 type could probably just be a double, right? Or are there systems where double and _real64 are different?
  5. The _complex128 type is a bit trickier since _Complex double wouldn't be accepted in C++ and std::complex<double> wouldn't be accepted in C. So having a custom typedef makes sense (even though I'd probably prefer a compiler flag letting me specify my own), but I'm not sure how good a practice it is in C to make type names starting with an underscore and without an additional namespace prefix, visible to external code.
  6. The generated header #includes some internal headers which are not needed for the public interface. E.g. I don't think the "Timing.h" should appear in the header.

Feature requests :smiley:

1.

Could we teach the Chapel compiler how to handle c_ptrConst in the generated C code? The issue here is that if one naively includes const qualifiers in the generated signatures, the C backend might break, because it might try to convert const-qualified parameters to non const-qualified local variables. But that's just my guess... perhaps it's much easier in practice.

Motivation

In C it's quite tricky to stay "safe" when const qualifiers are not part of the declaration. Say, I'm writing my own C function that received double const* xs as an argument, and I'd like to call a Chapel function that expects a c_ptrConst(real(64)). I'll have to do (double *)xs when passing the variable to Chapel. Suppose that I now replace double const* xs with float const* xs in my C code. The compiler won't complain and will happily coerce my xs into a double *, but surely that's undesirable. If the Chapel function had expected a double const* instead, I would have gotten a compiler error. Illustration with code:

// Library.chpl
export proc sum(count : int(64), xs : c_ptrConst(real(64))) : real(64) { ... }

Generated header:

// Library.h
// what's generated right now:
_real64 sum(int64_t count, _real64 * xs);
// what I wish was generated:
_real64 sum(int64_t count, _real64 const * xs);

Using the library:

#include "Library.h"

void my_simulation(int64_t const count, double const* xs) {
  // ...
  const double total = sum(count, (double *)xs); // what we have to write right now
  const double total = sum(count, xs); // what I wish we could write
  // ...
}

void my_simulation_f32(int64_t const count, float const* xs) {
  // ...
  const double total = sum(count, (double *)xs); // Oops a crash at runtime
  const double total = sum(count, xs); // Good, compiler error!
  // ...
}

2.

Could the Chapel compiler only include headers that are strictly necessary for the declarations?

Motivation

This would open up a possibility for libraries to have Chapel as an implementation detail. E.g., if I'd like to expose some functions to Python, I could just give a header to CFFI. Unfortunately, right now the Python code will need access to the internal Chapel headers which might not even be available on the system. If we were instead to include "stdint.h" and such for int64_t instead of "stdchpl.h", the Python code would not need to have access to a Chapel installation.


Sorry if that's not the most structured proposal/request, it's mainly meant to initiate a discussion, and I'd be happy to clarify my wishes and struggles in the comments.

bradcray commented 5 months ago

Thanks for taking the time to capture this, and for the extensive write-up, @twesterhout!

bradcray commented 4 months ago

@twesterhout : I wanted to make sure you were aware that https://github.com/chapel-lang/chapel/pull/24746 should have addressed your first of two requests here. If you get a chance to try it out and verify that it provides the benefit you were hoping for, that'd be good to know.

I'm leaving this open due to the second request (reducing the number of header files used to something more minimal)