[PATCH] D52191: Fix logic around determining use of frame pointer with -pg.
vsapsai added a comment. Thanks for the fix. Repository: rL LLVM https://reviews.llvm.org/D52191 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52191: Fix logic around determining use of frame pointer with -pg.
srhines added a comment. Thanks @dblaikie for the quick fixup. I must have accidentally dropped the '!', because I did run check-all to test the change. Repository: rL LLVM https://reviews.llvm.org/D52191 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52191: Fix logic around determining use of frame pointer with -pg.
dblaikie added a subscriber: srhines. dblaikie added a comment. Fixed in r342510 with the solution I mentioned up-thread. Repository: rL LLVM https://reviews.llvm.org/D52191 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D52191: Fix logic around determining use of frame pointer with -pg.
Fixed in r342510 with the solution I mentioned up-thread. On Tue, Sep 18, 2018 at 1:10 PM Volodymyr Sapsai via Phabricator < revi...@reviews.llvm.org> wrote: > vsapsai added a comment. > > 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. > > > Repository: > rL LLVM > > https://reviews.llvm.org/D52191 > > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52191: Fix logic around determining use of frame pointer with -pg.
vsapsai added a comment. 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. Repository: rL LLVM https://reviews.llvm.org/D52191 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52191: Fix logic around determining use of frame pointer with -pg.
rdhindsa added a comment. It seems that following tests are broken with this change: clang/test/Driver/clang_f_opts.c clang/test/Frontend/gnu-mcount.c Repository: rL LLVM https://reviews.llvm.org/D52191 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52191: Fix logic around determining use of frame pointer with -pg.
vsapsai added a comment. 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/ Repository: rL LLVM https://reviews.llvm.org/D52191 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52191: Fix logic around determining use of frame pointer with -pg.
dblaikie added a comment. In https://reviews.llvm.org/D52191#1238648, @srhines wrote: > In https://reviews.llvm.org/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? Repository: rL LLVM https://reviews.llvm.org/D52191 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52191: Fix logic around determining use of frame pointer with -pg.
This revision was automatically updated to reflect the committed changes. Closed by commit rC342501: Fix logic around determining use of frame pointer with -pg. (authored by srhines, committed by ). Changed prior to commit: https://reviews.llvm.org/D52191?vs=165826&id=166007#toc Repository: rL LLVM https://reviews.llvm.org/D52191 Files: lib/Driver/ToolChains/Clang.cpp Index: lib/Driver/ToolChains/Clang.cpp === --- lib/Driver/ToolChains/Clang.cpp +++ lib/Driver/ToolChains/Clang.cpp @@ -4956,8 +4956,7 @@ } 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); Index: lib/Driver/ToolChains/Clang.cpp === --- lib/Driver/ToolChains/Clang.cpp +++ lib/Driver/ToolChains/Clang.cpp @@ -4956,8 +4956,7 @@ } 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); ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52191: Fix logic around determining use of frame pointer with -pg.
This revision was automatically updated to reflect the committed changes. Closed by commit rL342501: Fix logic around determining use of frame pointer with -pg. (authored by srhines, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D52191 Files: cfe/trunk/lib/Driver/ToolChains/Clang.cpp Index: cfe/trunk/lib/Driver/ToolChains/Clang.cpp === --- cfe/trunk/lib/Driver/ToolChains/Clang.cpp +++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp @@ -4956,8 +4956,7 @@ } 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); Index: cfe/trunk/lib/Driver/ToolChains/Clang.cpp === --- cfe/trunk/lib/Driver/ToolChains/Clang.cpp +++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp @@ -4956,8 +4956,7 @@ } 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); ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52191: Fix logic around determining use of frame pointer with -pg.
srhines added a comment. In https://reviews.llvm.org/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. Repository: rC Clang https://reviews.llvm.org/D52191 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52191: Fix logic around determining use of frame pointer with -pg.
dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. 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. Repository: rC Clang https://reviews.llvm.org/D52191 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52191: Fix logic around determining use of frame pointer with -pg.
srhines created this revision. srhines added a reviewer: dblaikie. Herald added a subscriber: cfe-commits. 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. Repository: rC Clang https://reviews.llvm.org/D52191 Files: lib/Driver/ToolChains/Clang.cpp Index: lib/Driver/ToolChains/Clang.cpp === --- lib/Driver/ToolChains/Clang.cpp +++ lib/Driver/ToolChains/Clang.cpp @@ -4956,8 +4956,7 @@ } 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); Index: lib/Driver/ToolChains/Clang.cpp === --- lib/Driver/ToolChains/Clang.cpp +++ lib/Driver/ToolChains/Clang.cpp @@ -4956,8 +4956,7 @@ } 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); ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
