[PATCH] [MC] Use the non-EH register mapping in the debug_frame section.

[Jim Grosbach]

Fair enough. Works for me.

-Jim

> On Feb 12, 2015, at 1:52 PM, Frédéric Riss <friss at apple.com> wrote:
> 
> 
>> On Feb 12, 2015, at 1:22 PM, Jim Grosbach <grosbach at apple.com> wrote:
>> 
>> Oof. The patch itself seems reasonable. I tend to agree with your thought that it may not be a root cause. Do you have any ideas about what a deeper solution would look like?
> 
> I guess there is no deeper solution. Storing the LLVM regnum in the CFI instruction would be marginally cleaner as it’s supposed to be more ‘canonical’, and it would need to be converted only once. You’d still have to make an arbitrary choice when emitting .cfi directives in AsmStreamer. They are used for both EH and standard frame info, but they take only one argument. This usecase really only works when both mapping are identical.
> 
> As Rafael put it in another reply, we have to choose one number to represent the register anyway. The EH number is the most used one, thus it’s not a bad choice.
> 
> Fred
> 
>> -Jim
>> 
>>> On Feb 12, 2015, at 8:32 AM, Frederic Riss <friss at apple.com> wrote:
>>> 
>>> Hi grosbach,
>>> 
>>> On 32bits x86 Darwin, the register mappings for the eh_frane and
>>> debug_frame sections are different. Thus the same CFI instructions
>>> should result in different registers in the object file. The
>>> problem isn't target specific though, but it requires that the
>>> mappings for EH register numbers be different from the standard
>>> Dwarf one.
>>> 
>>> The patch is really ugly. The CFI instructions are emitted with
>>> register operands that are already Dwarf EH register numbers,
>>> allthough they will be used to emit both EH and non-EH frame
>>> descriptions. I first thought the right solution was to encode
>>> LLVM register numbers in the CFI instructions and do the
>>> conversion at object emission time. This however breaks a lot
>>> of tests that where the .cfi_* asm directives are tested (which
>>> means it would break targets not using the integrated assembler).
>>> 
>>> What this patch does instead is to do a double conversion when
>>> emitting the debug_frame section. Knowing the CFI instruction
>>> references EH reg numbers, it converts them back to LLVM reg num
>>> and again back to the correct Dwarf mapping.
>>> 
>>> I'll add a test, but I first wanted to reach out for opinions
>>> as this looks like a bandaid rahter than a fix.
>>> 
>>> Fixes PR22363.
>>> 
>>> http://reviews.llvm.org/D7593
>>> 
>>> Files:
>>> lib/MC/MCDwarf.cpp
>>> 
>>> Index: lib/MC/MCDwarf.cpp
>>> ===================================================================
>>> --- lib/MC/MCDwarf.cpp
>>> +++ lib/MC/MCDwarf.cpp
>>> @@ -1045,11 +1045,16 @@
>>> void FrameEmitterImpl::EmitCFIInstruction(MCObjectStreamer &Streamer,
>>>                                         const MCCFIInstruction &Instr) {
>>> int dataAlignmentFactor = getDataAlignmentFactor(Streamer);
>>> +  auto *MRI = Streamer.getContext().getRegisterInfo();
>>> 
>>> switch (Instr.getOperation()) {
>>> case MCCFIInstruction::OpRegister: {
>>>   unsigned Reg1 = Instr.getRegister();
>>>   unsigned Reg2 = Instr.getRegister2();
>>> +    if (!IsEH) {
>>> +      Reg1 = MRI->getDwarfRegNum(MRI->getLLVMRegNum(Reg1, true), false);
>>> +      Reg2 = MRI->getDwarfRegNum(MRI->getLLVMRegNum(Reg2, true), false);
>>> +    }
>>>   Streamer.EmitIntValue(dwarf::DW_CFA_register, 1);
>>>   Streamer.EmitULEB128IntValue(Reg1);
>>>   Streamer.EmitULEB128IntValue(Reg2);
>>> @@ -1082,17 +1087,23 @@
>>>   return;
>>> }
>>> case MCCFIInstruction::OpDefCfa: {
>>> +    unsigned Reg = Instr.getRegister();
>>> +    if (!IsEH)
>>> +      Reg = MRI->getDwarfRegNum(MRI->getLLVMRegNum(Reg, true), false);
>>>   Streamer.EmitIntValue(dwarf::DW_CFA_def_cfa, 1);
>>> -    Streamer.EmitULEB128IntValue(Instr.getRegister());
>>> +    Streamer.EmitULEB128IntValue(Reg);
>>>   CFAOffset = -Instr.getOffset();
>>>   Streamer.EmitULEB128IntValue(CFAOffset);
>>> 
>>>   return;
>>> }
>>> 
>>> case MCCFIInstruction::OpDefCfaRegister: {
>>> +    unsigned Reg = Instr.getRegister();
>>> +    if (!IsEH)
>>> +      Reg = MRI->getDwarfRegNum(MRI->getLLVMRegNum(Reg, true), false);
>>>   Streamer.EmitIntValue(dwarf::DW_CFA_def_cfa_register, 1);
>>> -    Streamer.EmitULEB128IntValue(Instr.getRegister());
>>> +    Streamer.EmitULEB128IntValue(Reg);
>>> 
>>>   return;
>>> }
>>> @@ -1103,6 +1114,9 @@
>>>     Instr.getOperation() == MCCFIInstruction::OpRelOffset;
>>> 
>>>   unsigned Reg = Instr.getRegister();
>>> +    if (!IsEH)
>>> +      Reg = MRI->getDwarfRegNum(MRI->getLLVMRegNum(Reg, true), false);
>>> +
>>>   int Offset = Instr.getOffset();
>>>   if (IsRelative)
>>>     Offset -= CFAOffset;
>>> @@ -1136,6 +1150,8 @@
>>> }
>>> case MCCFIInstruction::OpRestore: {
>>>   unsigned Reg = Instr.getRegister();
>>> +    if (!IsEH)
>>> +      Reg = MRI->getDwarfRegNum(MRI->getLLVMRegNum(Reg, true), false);
>>>   Streamer.EmitIntValue(dwarf::DW_CFA_restore | Reg, 1);
>>>   return;
>>> }
>>> @@ -1290,10 +1306,10 @@
>>> if (CIEVersion == 1) {
>>>   assert(MRI->getRARegister() <= 255 &&
>>>          "DWARF 2 encodes return_address_register in one byte");
>>> -    streamer.EmitIntValue(MRI->getDwarfRegNum(MRI->getRARegister(), true), 1);
>>> +    streamer.EmitIntValue(MRI->getDwarfRegNum(MRI->getRARegister(), IsEH), 1);
>>> } else {
>>>   streamer.EmitULEB128IntValue(
>>> -        MRI->getDwarfRegNum(MRI->getRARegister(), true));
>>> +        MRI->getDwarfRegNum(MRI->getRARegister(), IsEH));
>>> }
>>> 
>>> // Augmentation Data Length (optional)
>>> 
>>> EMAIL PREFERENCES
>>> http://reviews.llvm.org/settings/panel/emailpreferences/
>>> <D7593.19836.patch>
>> 
>