pcc added inline comments.
================
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()) {
----------------
pcc wrote:
> 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");
> ```
>
Sorry, this was meant to be `OPT_fsplit_lto_unit` and `OPT_fno_split_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
