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 eabihf ABI support #2745

Merged
merged 2 commits into from
Nov 13, 2023
Merged

Add eabihf ABI support #2745

merged 2 commits into from
Nov 13, 2023

Conversation

no1wudi
Copy link
Collaborator

@no1wudi no1wudi commented Nov 10, 2023

No description provided.


if (is_baremetal_target(comp_ctx->target_arch, comp_ctx->target_cpu,
abi)) {
vendor_sys = "-none-unknown-";
Copy link
Contributor

Choose a reason for hiding this comment

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

Why need to set vendor sys of thumb to "-none-unknown-"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No functional change, just let target triple more close to baremetal platform.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the vendor sys is changed from "-pc-linux-" to "-none-unknown-", not sure whether it affects functionality. The ABI may be changed?

Copy link
Collaborator Author

@no1wudi no1wudi Nov 10, 2023

Choose a reason for hiding this comment

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

I had a test by binary diff with "-pc-linux-" and "-none-unknown-" for a thumb based device, nothing changed in the AOT file.

Copy link
Contributor

Choose a reason for hiding this comment

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

It may affect the below "msvc" check, for abi == "msvc" and arch == "thumb", the original vendor_sys is "-pc-windows-", but now it is changed to "-none-unknown-". How about adding the check in the else branch:

    if (!strcmp(abi, "msvc")) {
        if (!strcmp(arch1, "i386"))
            vendor_sys = "-pc-win32-";
        else
            vendor_sys = "-pc-windows-";
    }
    else {
        if (is_baremetal_target(arch, cpu, abi))
            vendor_sys = "-unknown-none-";
        else
            vendor_sys = "-pc-linux-";
    }

BTW, it seems that the vendor_sys for bare metal should be "-unknown-none-" but not "-none-unknown-"? Refer to:
https://clang.llvm.org/docs/CrossCompilation.html#target-triple

The vendor needs to be specified only if there’s a relevant change, for instance between PC and Apple. Most of the time it can be omitted (and Unknown) will be assumed, which sets the defaults for the specified architecture. The system name is generally the OS (linux, darwin), but could be special like the bare-metal “none”.

Copy link
Collaborator Author

@no1wudi no1wudi Nov 13, 2023

Choose a reason for hiding this comment

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

Does the couple of msvc and thumb is really exists?

Sorry, the triple order is a typo.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the order is ok ? "unknown" for vendor and "none" for system type.

I am not sure, but the document mentions:

The vendor needs to be specified only if there’s a relevant change, for instance between PC and Apple.
Most of the time it can be omitted (and Unknown) will be assumed, which sets the defaults for the specified architecture.

The system name is generally the OS (linux, darwin), but could be special like the bare-metal “none”.

And BTW, does the couple of msvc and thumb is really exists?

msvc + thumb may not exist, but we may add more bare metal target in the future? Such as msvc + aarch64?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

Signed-off-by: Huang Qi <huangqi3@xiaomi.com>
Signed-off-by: Huang Qi <huangqi3@xiaomi.com>
Copy link
Contributor

@wenyongh wenyongh left a comment

Choose a reason for hiding this comment

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

LGTM

@wenyongh wenyongh merged commit e4353b4 into bytecodealliance:main Nov 13, 2023
383 checks passed
victoryang00 pushed a commit to victoryang00/wamr-aot-gc-checkpoint-restore that referenced this pull request May 27, 2024
…codealliance#2745)

Set the vendor-sys of bare-metal targets to "-unknown-none-",
and currently only add "thumbxxx" to the bare-metal target list.

Signed-off-by: Huang Qi <huangqi3@xiaomi.com>
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.

None yet

2 participants