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

Add support for aarch64 #26476

Merged
merged 1 commit into from
Mar 14, 2025
Merged

Conversation

troibe
Copy link

@troibe troibe commented Feb 28, 2025

This allows both compiling opentitantool and images for the sival chips on hosts with ARM architecture. Before merging this we should adjust the release paths for crt, lowrisc toolchains and safe ftdi.

@troibe
Copy link
Author

troibe commented Feb 28, 2025

Depends on lowRISC/crt#42 and lowRISC/safe-ftdi#3.

Currently HOST_ARCHS is duplicated between 3 Bazel files.
I'm not quite sure where the best location would be to store this information.

@troibe troibe marked this pull request as ready for review February 28, 2025 17:28
@troibe troibe requested review from cfrantz and a team as code owners February 28, 2025 17:28
@troibe troibe force-pushed the earlgrey_es_sival_arm branch 4 times, most recently from 3ea5afa to 2b5db07 Compare March 3, 2025 16:50
@troibe troibe removed request for a team and cfrantz March 4, 2025 10:59
@troibe troibe marked this pull request as draft March 4, 2025 10:59
@troibe troibe force-pushed the earlgrey_es_sival_arm branch 4 times, most recently from c0cb7a3 to 6683457 Compare March 6, 2025 19:16
@cfrantz
Copy link
Contributor

cfrantz commented Mar 6, 2025

Depends on lowRISC/crt#42 and lowRISC/safe-ftdi#3.

Currently HOST_ARCHS is duplicated between 3 Bazel files. I'm not quite sure where the best location would be to store this information.

I'd put common host definitions (like HOST_ARCHS and get_host_cpu) into //rules:host.bzl and import them elsewhere as needed (e.g. load("//rules:host.bzl", "HOST_ARCHS")).

@troibe
Copy link
Author

troibe commented Mar 7, 2025

Thanks for the advice. Let me do that. 👍

@troibe troibe force-pushed the earlgrey_es_sival_arm branch from 6683457 to 15b69fb Compare March 7, 2025 11:14
@troibe
Copy link
Author

troibe commented Mar 7, 2025

I had to put get_host_cpu() into it's own bazel rule file because host.bzl gets loaded too early leading to the following cycle:

ERROR: Failed to load Starlark extension '@local_config_platform//:constraints.bzl'.
Cycle in the workspace file detected. This indicates that a repository is used prior to being defined.
The following chain of repository dependencies lead to the missing definition.
 - @local_config_platform
This could either mean you have to add the '@local_config_platform' repository with a statement like `http_archive` in your WORKSPACE file (note that transitive dependencies are not added automatically), or move an existing definition earlier in your WORKSPACE file.
ERROR: Error computing the main repository mapping: cycles detected during computation of main repo mapping

Other than that this worked well so thanks again.
I had to make one more change to crt so I'll ask @jwnrt to do another release (PR coming soon) and change the dependency URL.

@troibe troibe force-pushed the earlgrey_es_sival_arm branch from 15b69fb to 2bad8a4 Compare March 7, 2025 11:27
@troibe troibe marked this pull request as ready for review March 7, 2025 15:33
@troibe troibe requested a review from a team as a code owner March 7, 2025 15:33
@troibe troibe requested review from rswarbrick, cfrantz and a team and removed request for a team March 7, 2025 15:33
@troibe
Copy link
Author

troibe commented Mar 7, 2025

Currently only depends on lowRISC/crt#43.

@troibe troibe force-pushed the earlgrey_es_sival_arm branch from 2bad8a4 to 3c11610 Compare March 10, 2025 16:34
@troibe
Copy link
Author

troibe commented Mar 10, 2025

Changes to all dependencies (crt, lowrisc toolchains and safe ftdi) are now merged and release paths adjusted.
This should be good to go from my side.

Copy link
Contributor

@jwnrt jwnrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks okay to me

@@ -57,7 +57,7 @@ fn status_create_safe(code: StatusCode, mod_id: u32, file: String, arg: i32) ->

// Convert an array of i8 to a string. This function will stop at first 0 (or at the
// end of the array if it contains no zero).
fn c_string_to_string(array: &[i8]) -> String {
fn c_string_to_string(array: &[c_char]) -> String {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't your error, but would it be better to convert the array to a CStr instead of this manual iterator to construct the string:

let cstr = std::ffi::CStr::from_bytes_until_nul(array);
cstr.to_string_lossy().to_string()

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least from a quick look at the documentation it seems like from_bytes_until_nul would error if there is no null character in array.
Now I'm not sure if the callers of c_string_to_string would actually pass in a non null terminated string but I would rather not introduce that slight change in behavior unless I'm sure about that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is one instance where it can be called with a non-null termined string:

module_id: c_string_to_string(&mod_id)

However this ia a very special case for a 3-character string where none of them is expected to be a NUL anyway. So you could potentially rewrite a bit the code to eliminate c_string_to_string

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for explaining. 😇
Would it be fine for you if I put that in a small follow up PR to this one?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure :)

This allows both compiling opentitantool and images for the sival chips on hosts with ARM architecture.

Signed-off-by: Martin Troiber <[email protected]>
@jwnrt jwnrt merged commit dbfb6b9 into lowRISC:earlgrey_es_sival Mar 14, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants