tejohnson marked 2 inline comments as done. tejohnson added inline comments.
================ Comment at: lib/CodeGen/BackendUtil.cpp:831 + *OS, CodeGenOpts.EmitLLVMUseLists, EmitLTOSummary, + /*EmitModuleHash=*/false)); } ---------------- pcc wrote: > Why add this argument here (and below)? I think this was leftover from an earlier version where I had added a new parameter. Will revert this change (here and below). ================ 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: > 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). 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