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

Reply via email to