[PATCH] D58320: [Darwin] Introduce a new flag, -fapple-link-rtlib that forces linking of the builtins library.

[Ahmed Bougacha via Phabricator via cfe-commits]

ab added a comment.

Please check the embedded thing (the other comments are minor).  Otherwise, LGTM; if you have found this flag to be necessary, this looks like a reasonable way to implement it



================
Comment at: clang/include/clang/Driver/Options.td:1252
+def fapple_link_rtlib : Flag<["-"], "fapple-link-rtlib">, Group<f_Group>,
+  HelpText<"Apple specific option to force linking the clang builtins runtime library">;
 def flto_EQ : Joined<["-"], "flto=">, Flags<[CoreOption, CC1Option]>, Group<f_Group>,
----------------
Hmm, maybe move "Apple specific" to a parenthesis at the end?

Also, I'm guessing this belonged here when it was "-flink-rtlib", it doesn't anymore.


================
Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:573
+  bool NoStdOrDefaultLibs =
+      Args.hasArg(options::OPT_nostdlib, options::OPT_nodefaultlibs);
+  bool ForceLinkBuiltins = Args.hasArg(options::OPT_fapple_link_rtlib);
----------------
Maybe sink this to the AddLinkRuntimeLibArgs implementations (as an extra parameter or directly checking the args)?  The flags don't bypass the whole thing anymore, might as well treat it like -mkernel and whatnot.


================
Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:2084
 
   AddLinkRuntimeLib(Args, CmdArgs, CompilerRT, RLO_IsEmbedded);
 }
----------------
This is different from 'builtins'.  Are you OK with the difference?  Otherwise maybe this should be an error for now;  I wouldn't be surprised if we never hit this path (this part, I'm not familiar with)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58320/new/

https://reviews.llvm.org/D58320