Add a few C APIs by benquike · Pull Request #2047 · google/xls

benquike · 2025-04-22T18:57:37Z

No description provided.

benquike · 2025-04-22T18:58:19Z

@cdleary
Can you please take a look?

cdleary

Looks good -- just the only thing I think should change is that the API should offer an xls_bytes_free to do the delete[] of the xls_bits_to_bytes() result under the hood.

With that in place all looks good!

I'm not sure why the CLA scan didn't happen automatically but I think to land the change I imagine you'll need to do https://cla.developers.google.com/about/google-individual

cdleary · 2025-04-23T03:05:29Z

+  const auto* cpp_bits = reinterpret_cast<const xls::Bits*>(bits);
+  auto bytes = cpp_bits->ToBytes();
+  *byte_count_out = bytes.size();
+  *bytes_out = (uint8_t*)malloc(bytes.size());


You'll need a xls_bytes_free() for this (because the libxls allocator can be different from the surrounding process so you can't assume the surrounding process can use the same free()), and generally I think we've been doing new[] inside the impl e.g. https://sourcegraph.com/github.com/google/xls@ef07f4bc7db8df70dc32e52a04d268653b58847a/-/blob/xls/public/c_api_impl_helpers.cc?L72

Done, can you take another look?

benquike · 2025-04-23T16:27:04Z

Looks good -- just the only thing I think should change is that the API should offer an xls_bytes_free to do the delete[] of the xls_bits_to_bytes() result under the hood.

With that in place all looks good!

I'm not sure why the CLA scan didn't happen automatically but I think to land the change I imagine you'll need to do https://cla.developers.google.com/about/google-individual

I am a Googler thus have signed CLA.

ingomueller-net · 2025-04-24T15:26:00Z

 bool xls_bits_get_bit(const struct xls_bits* bits, int64_t index);

+// Returns the bytes in the given bits value. The caller must
+// free the returned `bytes_out` pointer via `free`,  The caller


Shouldn't it be freed via xls_bytes_free?

ingomueller-net · 2025-04-24T15:27:20Z

+// Converts the given bits value to a signed or unsigned integer 64-bit integer
+// value. if the conversion is not possible, false is returned and `error_out`
+// contains the error message and the caller must free the `value_out` pointer
+// itself via `free`.


In the success case, no memory is returned, so there is nothing to free, right?

Doc in comment updated.

ingomueller-net

LGTM!

Disclaimer: I am not familiar with the coding conventions of this project. Some of the code does not respect the Google style guide (like the use of auto) but similar patterns are used in the rest of the file so I guess it's fine.

ingomueller-net · 2025-04-24T17:43:45Z

+                       uint8_t** bytes_out, size_t* byte_count_out);
+
+// Converts the given bits value to a signed or unsigned integer 64-bit integer
+// value. if the conversion is not possible, false is returned, `error_out`


Ultra-nit:

Suggested change

// value. if the conversion is not possible, false is returned, `error_out`

// value. If the conversion is not possible, false is returned, `error_out`

benquike · 2025-04-24T18:12:39Z

LGTM!

Disclaimer: I am not familiar with the coding conventions of this project. Some of the code does not respect the Google style guide (like the use of auto) but similar patterns are used in the rest of the file so I guess it's fine.

It would be nice to have a lint tool for style conformance.

benquike · 2025-04-24T18:15:38Z

@cdleary can you take another look and if it is OK, can we proceed with the merge?

Add xls_value_get_kind to allow inspecting the type of a Value object. Signed-off-by: Hui Peng <phui@google.com>

cdleary

LGTM! You may want to check that this has nothing flagged under bazel test -c opt --config=asan or it may hit a snag when folks attempt to import it to the repo.

cdleary · 2025-04-25T00:51:30Z

+  CHECK(error_out != nullptr);
+  CHECK(byte_count_out != nullptr);
+  const auto* cpp_bits = reinterpret_cast<const xls::Bits*>(bits);
+  auto bytes = cpp_bits->ToBytes();


Since the type is not obvious from the right hand side expression this auto should probably be elided. (That's the general C++ style guide guideline.)

Since the type is not obvious from the right hand side expression this auto should probably be elided. (That's the general C++ style guide guideline.)

Fixed.

cdleary · 2025-04-25T00:53:22Z

Disclaimer: I am not familiar with the coding conventions of this project. Some of the code does not respect the Google style guide (like the use of auto) but similar patterns are used in the rest of the file so I guess it's fine.

Yeah this is a good point, an auto for a routine called ToUint64() that returns a StatusOr<uint64_t> is a bit on the edge of the guideline in my experience, if you wanted to be sure you could elide auto for those too.

Thanks for helping build out this API!

cc @meheff @richmckeever and team to tag for import.

benquike · 2025-04-25T00:54:08Z

LGTM! You may want to check that this has nothing flagged under bazel test -c opt --config=asan or it may hit a snag when folks attempt to import it to the repo.

Can we add a check in the CI?

benquike · 2025-04-25T01:05:09Z

Yeah this is a good point, an auto for a routine called ToUint64() that returns a StatusOr<uint64_t> is a bit on the edge of the guideline in my experience, if you wanted to be sure you could elide auto for those too.

Fixed

benquike · 2025-04-25T01:06:12Z

LGTM! You may want to check that this has nothing flagged under bazel test -c opt --config=asan or it may hit a snag when folks attempt to import it to the repo.

I got the following odr violation, is it expected?

=================================================================
==3602574==ERROR: AddressSanitizer: odr-violation (0x7f8181685480):

These globals were registered at these points:
[1]:
#0 0x55f14168b7a6 in __asan_register_globals /tmp/final/llvm-project/compiler-rt/lib/asan/asan_globals.cpp:369:3
#1 0x55f14168c8c9 in __asan_register_elf_globals /tmp/final/llvm-project/compiler-rt/lib/asan/asan_globals.cpp:352:3
#2 0x7f81a6188f4d in call_init elf/dl-init.c:74:3
#3 0x7f81a6188f4d in call_init elf/dl-init.c:26:1

[2]:
#0 0x55f14168b7a6 in __asan_register_globals /tmp/final/llvm-project/compiler-rt/lib/asan/asan_globals.cpp:369:3
#1 0x55f14168c8c9 in __asan_register_elf_globals /tmp/final/llvm-project/compiler-rt/lib/asan/asan_globals.cpp:352:3
#2 0x7f81a6188f4d in call_init elf/dl-init.c:74:3
#3 0x7f81a6188f4d in call_init elf/dl-init.c:26:1

==3602574==HINT: if you don't care about these errors you may set ASAN_OPTIONS=detect_odr_violation=0
SUMMARY: AddressSanitizer: odr-violation: global 'deflate_copyright' at /proc/self/cwd/external/llvm_zlib/deflate.c:55 in

cdleary · 2025-04-25T01:27:17Z

I got the following odr violation, is it expected?

Yes I get that as well, use the env var to disable it as it suggests: ASAN_OPTIONS=detect_odr_violation=0

cdleary · 2025-04-25T01:28:00Z

LGTM! You may want to check that this has nothing flagged under bazel test -c opt --config=asan or it may hit a snag when folks attempt to import it to the repo.

Can we add a check in the CI?

#1447 is the issue that tracks this but I'm not sure there's active movement on it

benquike · 2025-04-25T01:30:44Z

I got the following odr violation, is it expected?

Yes I get that as well, use the env var to disable it as it suggests: ASAN_OPTIONS=detect_odr_violation=0

I ran the following command and all tests passed

ASAN_OPTIONS=detect_odr_violation=0 bazel run -c opt --config=asan //xls/public:c_api_test

cdleary · 2025-04-25T02:37:28Z

I ran the following command and all tests passed
ASAN_OPTIONS=detect_odr_violation=0 bazel run -c opt --config=asan //xls/public:c_api_test

Awesome! I expect the google XLS team build gardener should be able to merge successfully then.

@benquike Just out of curiosity are you building out the C APIs for use in the Rust crate? (https://github.com/xlsynth/xlsynth-crate) Or a different use case?

erinzmoore

LG overall. Just the comment on auto from cdleary@ to address and then can merge.

- xls_bits_to_bytes - xls_bits_to_uint64 - xls_bits_to_int64 Signed-off-by: Hui Peng <phui@google.com>

benquike · 2025-04-25T15:39:56Z

@benquike Just out of curiosity are you building out the C APIs for use in the Rust crate? (https://github.com/xlsynth/xlsynth-crate) Or a different use case?

No, We are using these APIs to create python bindings.
do you want me to add it in rust binding as well?

cdleary · 2025-04-26T02:57:17Z

@benquike Just out of curiosity are you building out the C APIs for use in the Rust crate? (https://github.com/xlsynth/xlsynth-crate) Or a different use case?

No, We are using these APIs to create python bindings. do you want me to add it in rust binding as well?

Nah it's ok was just curious, I can add them there no problem, look forward to the Python bindings if you'll be doing them in open source!

cdleary requested changes Apr 23, 2025

View reviewed changes

benquike force-pushed the main branch 3 times, most recently from 19089f8 to 03d519c Compare April 23, 2025 16:21

benquike requested a review from cdleary April 23, 2025 16:26

ingomueller-net reviewed Apr 24, 2025

View reviewed changes

benquike force-pushed the main branch 2 times, most recently from e205c20 to 2eadc0f Compare April 24, 2025 16:03

benquike requested a review from ingomueller-net April 24, 2025 17:32

ingomueller-net approved these changes Apr 24, 2025

View reviewed changes

benquike force-pushed the main branch from 2eadc0f to 289f052 Compare April 24, 2025 17:49

[XLS][c_api] add xls_value_get_kind C API.

0de695d

Add xls_value_get_kind to allow inspecting the type of a Value object. Signed-off-by: Hui Peng <phui@google.com>

cdleary approved these changes Apr 25, 2025

View reviewed changes

cdleary reviewed Apr 25, 2025

View reviewed changes

benquike force-pushed the main branch from 289f052 to 0a4f717 Compare April 25, 2025 01:03

erinzmoore requested changes Apr 25, 2025

View reviewed changes

[XLS][c_api] add the following c-apis

0b24db1

- xls_bits_to_bytes - xls_bits_to_uint64 - xls_bits_to_int64 Signed-off-by: Hui Peng <phui@google.com>

benquike force-pushed the main branch from 0a4f717 to 0b24db1 Compare April 25, 2025 15:36

benquike requested a review from erinzmoore April 25, 2025 15:40

erinzmoore approved these changes Apr 25, 2025

View reviewed changes

erinzmoore added the Reviewing Internally label Apr 25, 2025

copybara-service Bot merged commit b0d5ab7 into google:main Apr 25, 2025

	// value. if the conversion is not possible, false is returned, `error_out`
	// value. If the conversion is not possible, false is returned, `error_out`

Uh oh!

Conversation

benquike commented Apr 22, 2025

Uh oh!

benquike commented Apr 22, 2025

Uh oh!

cdleary left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benquike commented Apr 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benquike Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ingomueller-net left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benquike commented Apr 24, 2025

Uh oh!

benquike commented Apr 24, 2025

Uh oh!

cdleary left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdleary commented Apr 25, 2025

Uh oh!

benquike commented Apr 25, 2025

Uh oh!

benquike commented Apr 25, 2025

Uh oh!

benquike commented Apr 25, 2025

I got the following odr violation, is it expected?

Uh oh!

cdleary commented Apr 25, 2025

I got the following odr violation, is it expected?

Uh oh!

cdleary commented Apr 25, 2025

Uh oh!

benquike commented Apr 25, 2025

I got the following odr violation, is it expected?

Uh oh!

cdleary commented Apr 25, 2025

Uh oh!

erinzmoore left a comment

Choose a reason for hiding this comment

Uh oh!

benquike commented Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cdleary commented Apr 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

benquike Apr 24, 2025 •

edited

Loading

benquike commented Apr 25, 2025 •

edited

Loading

cdleary commented Apr 26, 2025 •

edited

Loading