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

Emit position dependent code for embedded targets #15174

Merged
merged 3 commits into from
Nov 14, 2024

Conversation

RX14
Copy link
Contributor

@RX14 RX14 commented Nov 7, 2024

Embedded targets (detected by target triples constaining "eabi") would prefer non-PIC builds, for smaller binary sizes and easier runtime support (avoiding having to handle the GOT).

@ysbaddaden
Copy link
Contributor

Do you have some references we could look up on this topic?

@RX14 RX14 removed the request for review from ysbaddaden November 8, 2024 09:30
@RX14
Copy link
Contributor Author

RX14 commented Nov 8, 2024

Position independent code is code which can be loaded and ran at any address, this is accomplished by making the code and data refer to each other using relative address offsets, instead of absolute addresses. This is used for dynamic libraries, and more recently to allow the dynamic loader (ld.so) to randomize the code location which has security benefits. However, this does introduce additional complexity, as some offset tables that contain locations of code and data have to be loaded and maintained.

There's some more info here: https://mcuoneclipse.com/2021/06/05/position-independent-code-with-gcc-for-arm-cortex-m/

I think in the future, PIC should be configurable for crystal, but in any case I believe the default for embedded targets should be non-PIC. I personally couldn't get the linker to emit .plt/.got sections with the funky double-linking elf-patching toolchain rp2040 uses.

@RX14
Copy link
Contributor Author

RX14 commented Nov 8, 2024

To give some better reasons other than "i couldn't get it to work":

  • PIC code is significantly larger than non-PIC, and embedded cares a lot about binary size.
  • Other toolchains for other boards likely doesn't support PIC as well, as it's really uncommon in the MCU world for the above reason.
  • The performance downside of PIC vs. non-PIC is likely larger with simpler in-order cores because there's more indirection and more instructions.

@RX14 RX14 requested a review from HertzDevil November 11, 2024 13:15
@ysbaddaden
Copy link
Contributor

I understand and agree for real embedded CPU with limited power/memory, such as the Cortex M series, but the eabi targets in crystal target all of ARM32, which aren't necessarily embedded. For example the debian arm/armhf are arm-linux-gnueabi and arm-linux-gnueabihf 🤷

Or do I read this wrong and you mean to disable PIC by default for the generic arm-unknown-eabi and -eabihf targets (no stdlib)? That would make perfect sense.

I'd love a -Dnopic flag (target independent)? And maybe a -Dpic flag if we disable it by default on some targets.

@RX14
Copy link
Contributor Author

RX14 commented Nov 13, 2024

Oh dear, I forgot that gnueabi was used on linux. Yes, the intention is to disable this on bare-metal targets like arm-none-eabi.

This stackoverflow answer seems to imply that gnueabi is used with arm32 linux, and just eabi is used for embedded targets. So, it seems this PR should be reverted to it's original state where it only activates for eabi exactly. I'm quite hesitant to disable PIC for all none targets, since it seems a wider scope change, but I could be convinced.

I'm fine to add a pic flag, but I'm not sure what it could ever be used for?

@straight-shoota
Copy link
Member

I'm fine to add a pic flag, but I'm not sure what it could ever be used for?

Possibly to correct an inconvenient default configuration that disables it for a particular target, but you still want PIC... Like when targerting gnueabi in the current state of this PR 😏
Not sure if there's much relevance for this as I figure the default should typically be accurate.

For comparison, rustc has a relocation-model option with values static, pic, pie., Golang has a build-mode option which serves a number of different purposes, but also has a pie option.

@RX14
Copy link
Contributor Author

RX14 commented Nov 13, 2024

Oh, you mean use -Dpic -Dnopic to override the PIC default? Is there precedent for -D affecting compiler configuration, and not the other way around?

@straight-shoota
Copy link
Member

@straight-shoota
Copy link
Member

straight-shoota commented Nov 13, 2024

Though I suppose we could talk about using a different method to pass these options to the compiler: They could be ordinary CLI options. Or we could a separate CLI option that works like -D but is explicitly for compiler options (for example Rust uses -C for codegen flags).

@RX14
Copy link
Contributor Author

RX14 commented Nov 13, 2024

-Drelocations=static and -Drelocations=pic makes sense for me. Would the correct behaviour be that they're always set as flags for the program, but setting them at the command line overrides the default choice?

@straight-shoota
Copy link
Member

I'm not sure if the program actually needs to know these flags. It's not really relevant, is it?
I suppose that would be a reason to go for a different mechanism than standard compiler flags 🤷

@RX14
Copy link
Contributor Author

RX14 commented Nov 13, 2024

I'm not sure if the program actually needs to know these flags

This is actually the question I meant to ask with "but I'm not sure what it could ever be used for". The only answer I can come up with is if the program calls out to link a C library at runtime it might want to pass -fPIC or not using that flag.

@ysbaddaden
Copy link
Contributor

Let's disable PIC for both targets with exactly eabi and eabihf in the environment parts (some Cortex-M series come with hardware floats) and keep it enabled otherwise (i.e. -gnueabi[hf], -musleabi[hf], -androideabi[hf], ...).

Using compile-time flags for setting compiler options is probably a bit clunky. It's dead simple to use, but explicit CLI options are probably more fit, yes.

@RX14
Copy link
Contributor Author

RX14 commented Nov 14, 2024

This should be the right logic for enabling now. Not sure what to think about how to expose override the default, unless we get consensus quickly I'd rather leave it for a later PR.

@RX14 RX14 merged commit 5e0bbaa into crystal-lang:master Nov 14, 2024
68 of 69 checks passed
@ysbaddaden ysbaddaden added this to the 1.15.0 milestone Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants