This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Fix ARM/AArch64/Mips/Sparc/SystemZ/X86 prologue_end position. Related to D11268.
ClosedPublic

Authored by iid_iunknown on Nov 4 2015, 10:53 AM.

Details

Summary

This review is related to another review request http://reviews.llvm.org/D11268, does the same and merely fixes a couple of issues with it.

D11268 is quite old and has merge conflicts against the current trunk.
This request

  • rebases D11268 onto the new trunk;
  • resolves the merge conflicts;
  • fixes the prologue_end tests, which do not pass due to the subprogram definitions not marked as distinct.

Diff Detail

Repository
rL LLVM

Event Timeline

iid_iunknown retitled this revision from to [DebugInfo] Fix ARM/AArch64 prologue_end position. Related to D11268..
iid_iunknown updated this object.
iid_iunknown added a reviewer: kubamracek.
iid_iunknown set the repository for this revision to rL LLVM.
iid_iunknown added subscribers: asl, llvm-commits.
rengolin accepted this revision.Nov 5 2015, 6:31 AM
rengolin edited edge metadata.
This revision is now accepted and ready to land.Nov 5 2015, 6:31 AM
kubamracek edited edge metadata.Nov 5 2015, 6:33 AM

Thanks for doing this! Please go ahead and commit (this patch was already LGTM'd so I think it's okay). Please note that when you commit, you might see a buildbot failure in ARM/AArch64 tests in compiler-rt/sanitizers/ASan/TSan -- this might actually be okay, IIRC there are a few XFAILs because of this bug. Let me know if you see such a test failure.

iid_iunknown closed this revision.Nov 5 2015, 9:52 AM
tyomitch added inline comments.
test/DebugInfo/ARM/prologue_end.ll
2

I don't think "thumbv1" is a valid thing. Is this supposed to be thumbv7 perhaps?

This patch seems to be adding Windows line endings again. Please fix that before committing.

Oh, sorry, it's the same patch as before. Never mind. Artyom's right that thumbv1 is probably not what you want though. "thumbv6m" is a reasonable alternative (a modern triple defaulting to a Thumb-1-only CPU).

I agree with the Mips16 changes but could you be a bit clearer with the subject line when the patch isn't ARM/AArch64 specific? I skipped over the D11268 thread because the subject line said it was ARM-only :-). Herald now CC's me on patches that touch the Mips directories so it doesn't matter as much anymore but a clearer subject line would still be useful.

Oh, sorry, it's the same patch as before. Never mind. Artyom's right that thumbv1 is probably not what you want though. "thumbv6m" is a reasonable alternative (a modern triple defaulting to a Thumb-1-only CPU).

Hello Tim.

Thank you for you comments. Artyom has fixed the test, so it should be ok now.

This patch seems to be adding Windows line endings again. Please fix that before committing.

I am not sure but probably I somehow got those line endings while applying the original patch from D11268. Anyway, I'll pay extra attention to this in my future commits.
Thank you!

I agree with the Mips16 changes but could you be a bit clearer with the subject line when the patch isn't ARM/AArch64 specific? I skipped over the D11268 thread because the subject line said it was ARM-only :-). Herald now CC's me on patches that touch the Mips directories so it doesn't matter as much anymore but a clearer subject line would still be useful.

Hi Daniel,

My apologies for that. I will be more specific in future review requests.
Thanks for pointing this out!

iid_iunknown retitled this revision from [DebugInfo] Fix ARM/AArch64 prologue_end position. Related to D11268. to [DebugInfo] Fix ARM/AArch64/Mips/Sparc/SystemZ/X86 prologue_end position. Related to D11268..Nov 11 2015, 11:13 AM
iid_iunknown edited edge metadata.

I agree with the Mips16 changes but could you be a bit clearer with the subject line when the patch isn't ARM/AArch64 specific? I skipped over the D11268 thread because the subject line said it was ARM-only :-). Herald now CC's me on patches that touch the Mips directories so it doesn't matter as much anymore but a clearer subject line would still be useful.

Hi Daniel,

My apologies for that. I will be more specific in future review requests.
Thanks for pointing this out!

No worries. Thanks for updating the title.