CurlHandle: fix error message handling #522
Conversation
madsbk
added
bug
|
Can you add a description of what you observed is the problematic thing this is fixing please? |
cpp/include/kvikio/shim/libcurl.hpp
Outdated
| * `std::string` from a `char*`, the compiler might optimize the code such that the `std::string` | ||
| * is created from the full size of `__FILE__` including the trailing `\0` chars. | ||
| */ | ||
| __attribute__((optnone)) inline std::string fix_conda_file_path_hack(std::string filename) |
There was a problem hiding this comment.
issue: I think we should explicitly request noinline rather than hoping that optnone will ensure the function is not inlined.
| __attribute__((optnone)) inline std::string fix_conda_file_path_hack(std::string filename) | |
| __attribute__((noinline)) inline std::string fix_conda_file_path_hack(std::string filename) |
There was a problem hiding this comment.
I think we can do both, no?
__attribute__((optnone)) __attribute__((noinline)) std::string fix_conda_file_path_hack(std::string filename)There was a problem hiding this comment.
I couldn't see optnone as a valid attribute in the GCC documentation: https://gcc.gnu.org/onlinedocs/gcc-14.2.0/gcc/Common-Function-Attributes.html
There was a problem hiding this comment.
Interesting, somehow it's not in GCC's but it's in Clang's saying it's available in GNU (I assume GCC): https://clang.llvm.org/docs/AttributeReference.html#optnone
There was a problem hiding this comment.
Maybe it should be __attribute__((optimize(0))) instead?
There was a problem hiding this comment.
Maybe it should be
__attribute__((optimize(0)))instead?
Does clang support optimize(0) ?
There was a problem hiding this comment.
optimize is undocumented in Clang. 🤣
I guess you'll either have to risk leaving optnone or do some compiler checking.
There was a problem hiding this comment.
Let's see if __attribute__((noinline)) is enough
There was a problem hiding this comment.
If a68b9aa works then this is probably good as is. However, as @wence- pointed out optnone is not documented in GCC and perhaps might require something different such as optimize(0).
|
/merge |
|
@wence- @pentschev, thanks for the help debugging this! |
When building with conda, the
__FILE__macro will contain the (long) conda build path. At install time, this is replaced by the actual path, which may be shorter. Any extra characters are replaced by\0.The extra
\0is problematic ifCurlHandlelater throws an exception to Cython since while converting the exception to Python, Cython might truncate the error message.