Page MenuHomePhabricator

Restore correct x86_64 EH encodings in kernel code model

Authored by lliu0 on Aug 8 2018, 10:15 PM.



Fixes PR37524.

The exception handling encodings for x86_64 in kernel code model has been changed with r309884. Restore it to correct ones. These encodings include PersonalityEncoding, LSDAEncoding and TTypeEncoding.

Diff Detail


Event Timeline

lliu0 created this revision.Aug 8 2018, 10:15 PM
JDevlieghere accepted this revision.Aug 9 2018, 3:19 AM
  • I guess a possible alternative would have been to set LargeCodeModel to true for kernel case? Regardless, I'd prefer being explicit to prevent confusion.
  • I'm slightly worried about the default value being CodeModel::Small in case other parts in MCObjectFileInfo start relying on this, but it's definitely the most sensible.

So all things considered this LGTM.

This revision is now accepted and ready to land.Aug 9 2018, 3:19 AM
davide added a reviewer: rnk.Aug 9 2018, 8:21 AM
rnk requested changes to this revision.Aug 9 2018, 10:51 AM

Rafael's commit message was still correct, this shouldn't be part of MCObjectFileInfo, it should be in TargetLoweringObjectFile, which is only used by CodeGen. Give me a minute to try to untangle that.

This revision now requires changes to proceed.Aug 9 2018, 10:51 AM
rnk added a comment.Aug 9 2018, 1:53 PM

If we move some of this logic back up into CodeGen, that removes the need to pass the full code model to the assembler. I implemented this in D50533. Please take a look. You should be able to rebase your change to the x86_64 EH encodings on that, removing the need to thread the code model to MC.

lliu0 retitled this revision from Pass code model parameter to MC from CodeGen to Restore correct x86_64 EH encodings in kernel code model.Aug 9 2018, 7:04 PM
lliu0 edited the summary of this revision. (Show Details)
lliu0 updated this revision to Diff 160049.Aug 9 2018, 7:09 PM

Rebased after rL339397.

rnk added a comment.Aug 10 2018, 3:34 PM

Code looks good, but we should strengthen the test case.

3 ↗(On Diff #160049)

Please check all the relevant .cfi directives and the .byte that comes before this comment. For example, the number after .cfi_personalty should change as a result of this patch.

lliu0 updated this revision to Diff 160280.Aug 12 2018, 5:32 PM

Update test case.

lliu0 marked an inline comment as done.Aug 12 2018, 5:33 PM
rnk accepted this revision.Aug 12 2018, 10:37 PM

Looks good, thanks!

This revision is now accepted and ready to land.Aug 12 2018, 10:37 PM
This revision was automatically updated to reflect the committed changes.
hvdijk added a subscriber: hvdijk.Apr 27 2021, 3:42 PM
hvdijk added inline comments.

This looks like it's the wrong way around compared to the rest. I'm also not sure why the medium code model is treated like small in the PIC case, but like large in the non-PIC case, shouldn't it be the same for both?

Herald added a project: Restricted Project. · View Herald TranscriptApr 27 2021, 3:42 PM
Herald added a subscriber: pengfei. · View Herald Transcript
hvdijk added inline comments.May 9 2021, 7:08 AM

Proposed to swap sdata4 and sdata8 in D102132. The different handling of the medium model between the PIC and non-PIC cases actually looks like it might be correct after all: in the PIC case, we do not store the typeinfo address directly.