Page MenuHomePhabricator

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

Authored by srhines on Sep 17 2018, 1:55 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

srhines created this revision.Sep 17 2018, 1:55 PM
dblaikie accepted this revision.Sep 18 2018, 11:21 AM

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.

This revision is now accepted and ready to land.Sep 18 2018, 11:21 AM

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.

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.

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?

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/

It seems that following tests are broken with this change:

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

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.

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

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

Thanks for the fix.