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

Added support for newlib #3946

Merged
merged 4 commits into from
Apr 7, 2022
Merged

Added support for newlib #3946

merged 4 commits into from
Apr 7, 2022

Conversation

drahosj
Copy link
Contributor

@drahosj drahosj commented Mar 16, 2022

Fixes #3945 by handling newlib like uclibc.

Implemented similar to fixupUClibc. newlib will be removed from the
environment part of the triple, if present. The rest of the environment
is preserved (such as eabi). For example, the triple
arm-none-eabi-newlib will be converted to arm-none-eabi and
CRuntime_Newlib will be set.

Similar to fixupUClibcEnv, fixupNewlibEnv checks for "newlib" within
the environment of the triple, sets a global flag, then removes [-]newlib
from the triple. This preserves "eabi" if present in the triple, and allows
both arm-unknown-none-eabi-newlib and arm-unknown-none-newlib to
work.
@kinke
Copy link
Member

kinke commented Mar 16, 2022

On 32-bit ARM, triples looked like armv6-linux-gnueabihf (hard-float, glibc) and armv7a-unknown-linux-androideabi, so I'd have expected arm-none-newlibeabi. Are there any precedents for arm-none-eabi-newlib you could link? Btw, you can use ldc2 -v -mtriple=… to check the normalized triple (a quadruple actually) - the last value in the config line (arm-none-unknown-eabi-newlib - arm arch, none vendor, unknown OS, eabi-newline environment).

@drahosj
Copy link
Contributor Author

drahosj commented Mar 16, 2022

Are there any precedents for arm-none-eabi-newlib you could link?

I based the eabi-newlib environment format on my (admittedly unverified) understanding of how DMD was parsing the triple, but I agree that newlibeabi is the preferred format, for example. This patch does actually parse arm-unknown-none-newlibeabi correctly, with the resulting triple being arm-unknown-none-eabi being passed to the rest of the parsing. However, it doesn't handle arm-none-newlibeabi correctly - without fully specifying the full quadruple initially, the first-pass parse in fixUpNewlibEnv doesn't pick up newlibeabi as the environment component. Fixing that would require some creativity in how the triple is initially parsed.

Perhaps scanning the entire un-parsed triple for the string "newlib" and deleting it if present, before even attempting to parse it as an LLVM triple is the correct approach? It seems like a pretty blunt approach, but would make arm-none-newlibeabi work correctly - newlib would be detected and arm-unknown-none-eabi would end up being the normalized quad passed to the rest of the LLVM triple handling.

Avoids issues where an environment won't be recognized
as the environment component when the specified as a less-than-full
quadruple.
@JohanEngelen
Copy link
Member

To me it seems best to upstream to LLVM (and only have a temporary fix in LDC if needed). This prevents the D triple name diverging from the rest of the world. The changes needed are very simple: see e.g. https://gist.github.com/crabtw/4481604

kinke added 2 commits April 7, 2022 03:51
To make e.g. `-mtriple=armv7-none-newlibeabi` work, where
`newlibeabi` was previously apparently treated as OS component
before normalization.
@kinke
Copy link
Member

kinke commented Apr 7, 2022

I took the liberty of slightly refactoring the two env-patching functions to a single consistent one. Thanks for noticing the hassle wrt. having to specify a full quadruple (arm-none-newlibeabi not working, requiring sth. like arm-none--newlibeabi); string-matching the first -{uclibc,newlib} should be good enough indeed.

I've just checked the LLVM 14 source, apparently still no official uclibc support.

@drahosj
Copy link
Contributor Author

drahosj commented Apr 7, 2022

The refactor is definitely an improvement and keeps things relatively clean for what is ultimately a hack. While there may be some hassle with the triple parsing, having any command-line/build-environment way of getting the CRuntime_Newlib version identifier set without having to use a patched version of LDC is a massive convenience for any runtime development or porting effort.

@kinke
Copy link
Member

kinke commented Apr 7, 2022

having any command-line/build-environment way of getting the CRuntime_Newlib version identifier set without having to use a patched version of LDC is a massive convenience for any runtime development or porting effort.

This is the price to pay for predefined version identifiers; the according logic (and FWIW, tests) preventing the user from setting one is in the shared DMD frontend, so nothing LDC specific. Initial ports with an arbitrary custom version are surely simpler to perform, and adding the predefined version later. FWIW, I've been annoyed by those checks a few times myself. ;)

@kinke kinke merged commit 603e773 into ldc-developers:master Apr 7, 2022
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.

LDC can't set some reserved version identifiers (CRuntime_*)
3 participants