pcc accepted this revision. pcc added a comment. This revision is now accepted and ready to land.
LGTM ================ Comment at: include/clang/Basic/CodeGenOptions.def:120 +CODEGENOPT(EnableSplitLTOUnit, 1, 0) ///< Enable LTO unit splitting to support + /// CFI and tranditional whole program + /// devirtulization that require whole ---------------- traditional ================ Comment at: include/clang/Basic/CodeGenOptions.def:121 + /// CFI and tranditional whole program + /// devirtulization that require whole + /// program IR support. ---------------- devirtualization ================ Comment at: lib/Driver/ToolChains/Clang.cpp:5112 + bool EnableSplitLTOUnit = Args.hasFlag( + options::OPT_fsplit_lto_unit, options::OPT_fno_split_lto_unit, false); + if (EnableSplitLTOUnit || WholeProgramVTables || Sanitize.needsLTO()) { ---------------- tejohnson wrote: > pcc wrote: > > Should this default to `WholeProgramVTables || Sanitize.needsLTO()` and > > emit an error if the user passes the (for now) unsupported combinations > > `-fno-split-lto-unit -fwhole-program-vtables` or `-fno-split-lto-unit > > -fsanitize=cfi`? > I think the code below needs to stay as is to allow -fsplit-lto-unit to also > enable the splitting even when the other options aren't on (not sure when > that would be used outside of testing, but I'm assuming we want a way to > force that on). > > But yes I think it makes sense to emit an error on those combinations (when > my follow on patches go in then we would remove the error with > -fno-split-lto-unit -fwhole-program-vtables). `-fsplit-lto-unit` is required when linking translation units compiled with `-fsanitize=cfi` with translation units compiled without (the latter would need the flag). May I suggest simplifying this code as follows: ``` bool RequiresSplitLTOUnit = WholeProgramVTables || Sanitize.needsLTO(); bool SplitLTOUnit = Args.hasFlag(options::OPT_fwhole_program_vtables, options::OPT_fno_whole_program_vtables, RequiresSplitLTOUnit); if (RequiresSplitLTOUnit && !SplitLTOUnit) error; if (SplitLTOUnit) CmdArgs.push_back("-fsplit-lto-unit"); ``` Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53891/new/ https://reviews.llvm.org/D53891 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits