Fix logic around determining use of frame pointer with -pg.

[srhines]

Summary

As part of r342165, I rewrote the logic to check whether
-fno-omit-frame-pointer was passed after a -fomit-frame-pointer
argument. This CL switches that logic to use the consolidated
shouldUseFramePointer() function. This fixes a potential issue where -pg
gets used with -fomit-frame-pointer on a platform that must always retain
frame pointers.

[dblaikie]

Sure, looks good. Though my other/vague concern is why does this case error about fomit-frame-pointer having no effect, but other things (like using -fomit-frame-pointer on a target that requires frame pointers) that ignore -fomit-frame-pointer don't? Weird. But it probably makes sense somehow.

[srhines]

In D52191#1238628, @dblaikie wrote:

Sure, looks good. Though my other/vague concern is why does this case error about fomit-frame-pointer having no effect, but other things (like using -fomit-frame-pointer on a target that requires frame pointers) that ignore -fomit-frame-pointer don't? Weird. But it probably makes sense somehow.

I think the original issue is that one should not explicitly say "omit frame pointers" and "instrument for profiling with mcount". It's possible that there should be other errors for using "omit frame pointers" on targets that require them, but that should be checked independently elsewhere. Do we have many (any) of these kinds of targets? I'm going to submit this patch, but am happy to add a diagnostic later on for the other case, if you think that is worthwhile.

[dblaikie]

In D52191#1238648, @srhines wrote:

In D52191#1238628, @dblaikie wrote:

Sure, looks good. Though my other/vague concern is why does this case error about fomit-frame-pointer having no effect, but other things (like using -fomit-frame-pointer on a target that requires frame pointers) that ignore -fomit-frame-pointer don't? Weird. But it probably makes sense somehow.

I think the original issue is that one should not explicitly say "omit frame pointers" and "instrument for profiling with mcount". It's possible that there should be other errors for using "omit frame pointers" on targets that require them, but that should be checked independently elsewhere. Do we have many (any) of these kinds of targets? I'm going to submit this patch, but am happy to add a diagnostic later on for the other case, if you think that is worthwhile.

Yeah, looks like the other cases (targets, specifically - and yeah, I didn't look at the implementation of shouldUseFramePointer far enough to see which targets, etc - OK, I just looked now, and it's Darwin ARM & Thumb - so arguably on those targets, since -fomit-frame-pointer has no effect, maybe it's not important to error on -fomit-frame-pointer -pg)

Also I just realized you probably want/need "!shouldUseFramePointer" on your error. (missed the inversion) - sorry I didn't catch that in review. Should catch that with your test from the previous change?

[vsapsai]

Seems like this change causes 2 test failures:

Clang :: Driver/clang_f_opts.c
Clang :: Frontend/gnu-mcount.c

Some of the failing bots are

http://lab.llvm.org:8011/builders/clang-cmake-aarch64-quick/builds/15363/
http://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-incremental/53328/

[rdhindsa]

It seems that following tests are broken with this change:

clang/test/Driver/clang_f_opts.c
clang/test/Frontend/gnu-mcount.c

[vsapsai]

Confirm that reverting the change locally fixes the tests. If nobody beats me to it, I plan to revert the change in 30-60 minutes. @srhines, if you want to fix it in another way and need more time, please let me know.

[dblaikie]

Fixed in r342510 with the solution I mentioned up-thread.

[srhines]

Thanks @dblaikie for the quick fixup. I must have accidentally dropped the '!', because I did run check-all to test the change.

[vsapsai]

Thanks for the fix.

lib/Driver/ToolChains/Clang.cpp

if (Args.hasArg(options::OPT__SLASH_fallback) &&
     // fails, so that the main compilation's fallback to cl.exe runs.
     C.addCommand(llvm::make_unique<ForceSuccessCommand>(JA, *this, Exec,
                                                         CmdArgs, Inputs));
   } else {
     C.addCommand(llvm::make_unique<Command>(JA, *this, Exec, CmdArgs, Inputs));
   }
 
   if (Arg *A = Args.getLastArg(options::OPT_pg))
-    if (Args.hasFlag(options::OPT_fomit_frame_pointer,
-                     options::OPT_fno_omit_frame_pointer, /*default=*/false))
+    if (shouldUseFramePointer(Args, Triple))
       D.Diag(diag::err_drv_argument_not_allowed_with) << "-fomit-frame-pointer"
                                                       << A->getAsString(Args);
 
   // Claim some arguments which clang supports automatically.
 
   // -fpch-preprocess is used with gcc to add a special marker in the output to
   // include the PCH file. Clang's PTH solution is completely transparent, so we
   // do not need to deal with it at all.