This is an archive of the discontinued LLVM Phabricator instance.

Enable frame pointer for all non-leaf functions on riscv64 Android
ClosedPublic

Authored by hiraditya on May 12 2023, 3:14 PM.

Diff Detail

Event Timeline

hiraditya created this revision.May 12 2023, 3:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 12 2023, 3:14 PM
hiraditya requested review of this revision.May 12 2023, 3:14 PM
hiraditya edited the summary of this revision. (Show Details)May 12 2023, 3:15 PM
enh added inline comments.May 12 2023, 3:19 PM
clang/lib/Driver/ToolChains/Clang.cpp
423

should this...

439

(ah, this is why you need Android early. but, yeah, probably worth moving all the Android stuff together, like with the other OSes?)

459

...be down here instead?

465

(take care of all this android/arm stuff where you take care of android/riscv64? arm64 has frame pointers by "default default", so we don't need to mention it?)

hiraditya updated this revision to Diff 521826.May 12 2023, 3:47 PM

Addressed comments.

enh added inline comments.May 12 2023, 3:52 PM
clang/lib/Driver/ToolChains/Clang.cpp
424

(where? is it simpler to just add arm64 to the switch?)

522

how does this work for Android/arm64?

hiraditya added inline comments.May 12 2023, 4:06 PM
clang/lib/Driver/ToolChains/Clang.cpp
424

yeah we can add it for better readability but it would be redundant.

522

becaues of Triple.isAArch64(), AArch64 always has non-leaf frame pointers for all platforms.

hiraditya updated this revision to Diff 521833.May 12 2023, 4:13 PM
hiraditya added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
424

seems like we don't even have a test for aarch64.*android target. I'll add a testcase.

hiraditya updated this revision to Diff 521834.May 12 2023, 4:19 PM
hiraditya updated this revision to Diff 521835.May 12 2023, 4:23 PM

Is there more context on why Android enables the frame pointer?

craig.topper added inline comments.May 12 2023, 4:45 PM
clang/lib/Driver/ToolChains/Clang.cpp
522

You can use Triple.isRISCV64()

enh accepted this revision.May 12 2023, 5:36 PM

(lgtm with craig.topper's suggested simplification.)

clang/lib/Driver/ToolChains/Clang.cpp
522

lol. i could seem so much smarter if only i'd learn to read :-)

This revision is now accepted and ready to land.May 12 2023, 5:36 PM
MaskRay added inline comments.May 12 2023, 7:02 PM
clang/lib/Driver/ToolChains/Clang.cpp
522

&& has a lower precedence than ==. == doesn't need parens.

clang/test/Driver/frame-pointer-elim.c
109

Use --target= for new tests. -target has been deprecated since clang 3.x, albeit without a warning.

clang/test/Driver/frame-pointer.c
61

I don't think we need so many RUN lines. -O0 and -O1 suffice.

Addressed comments.

hiraditya marked 4 inline comments as done.May 15 2023, 10:31 AM

Is there more context on why Android enables the frame pointer?

From what i gathered, this is more of an effort to have parity such that existing build flag overrides continue to be consistent.

enh added a comment.May 15 2023, 10:59 AM

Is there more context on why Android enables the frame pointer?

From what i gathered, this is more of an effort to have parity such that existing build flag overrides continue to be consistent.

well, when i said that on the internal chat, i thought you were asking "why do we say what clang already says?" :-)

if the question was actually "is there more context on why Android enables the frame pointer?" i'd have said something like "because Android developers [OS and app developers alike] do so much debugging from the field, where all we get is a crash report for something we probably can't repro locally, that having good _and_ cheap unwinds is super important to us".

Is there more context on why Android enables the frame pointer?

From what i gathered, this is more of an effort to have parity such that existing build flag overrides continue to be consistent.

well, when i said that on the internal chat, i thought you were asking "why do we say what clang already says?" :-)

if the question was actually "is there more context on why Android enables the frame pointer?" i'd have said something like "because Android developers [OS and app developers alike] do so much debugging from the field, where all we get is a crash report for something we probably can't repro locally, that having good _and_ cheap unwinds is super important to us".

Thanks. I suspected that was the answer, but I wanted to check that it made sense to copy AArch64.

Is there more context on why Android enables the frame pointer?

From what i gathered, this is more of an effort to have parity such that existing build flag overrides continue to be consistent.

well, when i said that on the internal chat, i thought you were asking "why do we say what clang already says?" :-)

if the question was actually "is there more context on why Android enables the frame pointer?" i'd have said something like "because Android developers [OS and app developers alike] do so much debugging from the field, where all we get is a crash report for something we probably can't repro locally, that having good _and_ cheap unwinds is super important to us".

Thanks. I suspected that was the answer, but I wanted to check that it made sense to copy AArch64.

Enabling the frame pointer is fine. We probably should revert ancient conventions by using -fasynchronous-unwind-tables...