smithp35 wrote: > thanks for review @smithp35 > > > Not knowing the hexagon toolchain, I can't give you any useful comments on > > that part of the review. Based on our experience of Picolibc in the > > baremetal driver the question I have for you is whether there is enough > > picolibc specific details here to justify a specific picolibc environment. > > picolibc enviroment is right now used to set -I and -L paths, and we add > crt0-semihost and -lsemihost that we get from Picolibc, I don't know if this > is enough to justify a new environment. But what else is Musl environment > used for apart from what I listed? >
There are some restrictions on what MUSL supports for example ifunc support https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/TargetInfo.h#L1578 I agree that there isn't any defined rules for when an environment is the right answer or not. There was some concern raised at the dev meeting about the number of possible bare-metal C-libraries that exist and could have environments added for them. I think it would be reasonable to say picolibc, newlib and llvmlibc. > > It looks like the majority of the extra configuration that you are adding > > is not specific to picolibc, but could also apply to other bare-metal > > C-libraries like llvm libc and newlib. About the only thing that I can see > > that is Picolibc specific is adding crt-semihost.o for the startfiles. > > Would it be better to add this as a bare-metal configuration, not specific > > to picolibc, then you wouldn't need a new environment when changing > > C-library? > > sry, I don't get what you mean my "add this as bare-metal configuration", the > idea for adding this triple was so that C libraries can co-exist and can be > switch by just switching the triple from say : hexagon-none-picolibc to > hexagon-none-llvmlibc we would point -I and -L flags to the different dirs > based on the environment, and add crt and other files > Instead of switching include and library directories for a specific environment then you have a single -I and -L directory structure (shared between C-libraries) and then change sysroot to switch between C libraries. We support newlib and LLVM libc as an overlay package by installing a sysroot. We then have a config file that sets the sysroot and the same driver works for all 3. That wouldn't require a new environment per C-library, just a change of sysroot. > > FWIW. We require our users to use `-nostartfiles -lcrt-semihost` if they > > prefer the semihosting startup code (with semihosted argc, argv and return) > > and `lsemihost` for the semihost implementation. > > I did check > https://github.com/ARM-software/LLVM-embedded-toolchain-for-Arm/blob/main/docs/newlib.md, > I like the idea of pointing to correct sysroot using .cfg file, but isn't > adding crt/semihost libs too much work for users? wouldn't it be better if > the user can just specify triple or/and --libc flag and clang driver takes > care of everything else? > Yes, I think there is scope to do this better, although for most Drivers the choice of semihosting or not is independent of the environment. Production = no semihosting (no argc/argv and debug IO interface), development = semihosting. Picolibc's default is production with crt0.o no semihosting. It looks like you want to default to semihosting. There's always two environments picolibc and picolibc-semihosting but it seems like we're just putting too much into the environment. > > It may also be useful to respond to > > https://discourse.llvm.org/t/rfc-add-command-line-option-for-selecting-c-library/87335 > > as the environment part is acting that way. > > --libc flag would have been perfect for our usecase, but I don't know if this > is going to go in. if we could simply use `libc.isPicolibc()` instead of > `triple.isPicolibc()`, that would have been great > > > While I don't have any strong objections to adding picolibc as an > > evironment, there was some pushback to the RFC as there were so many > > bare-metal C-libraries, do we end up with an environment for each of them? > > With that in mind it may be better to split the generic triple parts out > > into a separate patch, to get more opinions on whether it is the right > > thing to do. > > I have split it into 2 commits, do you prefer it to be a separate PR instead? Main reason to split it out into two PRs is that there's two separate decisions here: * Should we add an environment for Picolibc that can be used by any Driver? * What should the Hexagon driver do with the Picolibc environment? I can much more easily contribute to reviews that could affect an Arm target. Decisions that are specific to the Hexagon driver are much harder for me to contribute to (as an Arm employee). It may be easier to get approval for the two independently as a smaller number of people are likely to be concerned about Hexagon specific changes. I don't think you need to split it, if I'm the only one bothered about it. I just won't be able to comment on anything beyond the environment parts. To summarise: * The main downside of adding a picolibc environment is that we'll likely end up with several more environments, I think newlib, newlib-nano and LLVM-libc are likely. * I think that there are ways to add support for picolibc without using an environment. That would avoid the concern above, for now at least, but I acknowledge that there are design trade-offs that may prefer an environment. * I don't/can't have an opinion on the Hexagon specific parts of the PR. * If there is a consensus is that adding an environment for bare-metal C-library is fine, then I don't have any objections, it is not much extra code and there are already examples in the hosted area. https://github.com/llvm/llvm-project/pull/169613 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
