Page MenuHomePhabricator

[Mips] Use addAliasForDirective rather than custom parsing logic for data directives
AbandonedPublic

Authored by asb on May 17 2018, 12:23 AM.

Details

Reviewers
sdardis
Summary

The target independent AsmParser doesn't recognise .hword, .word, .dword which are required for Mips. Currently MipsAsmParser recognises these through dispatch to MipsAsmParser::parseDataDirective. This contains equivalent logic to AsmParser::parseDirectiveValue. This patch allows reuse of AsmParser::parseDirectiveValue by making use of addAliasForDirective to support .hword, .word and .dword.

Diff Detail

Event Timeline

asb created this revision.May 17 2018, 12:23 AM
asb retitled this revision from {Mips] Use addAliasForDirective rather than custom parsing logic for data directives to [Mips] Use addAliasForDirective rather than custom parsing logic for data directives.May 17 2018, 12:23 AM
asb added a comment.May 17 2018, 12:34 AM

Apologies, I missed that there is a test failure/change with this patch. Symbol h in micromips-label-test.s now looks like:

Symbol {
  Name: h (40)
  Value: 0x8
  Size: 0
  Binding: Local (0x0)
  Type: None (0x0)
  Other [ (0x80)
    STO_MIPS_MICROMIPS (0x80)
  ]
  Section: .text (0x2)
}

Rather than:

Symbol {
  Name: h (40)
  Value: 0x8
  Size: 0
  Binding: Local (0x0)
  Type: None (0x0)
  Other: 0
  Section: .text (0x2)
}

I'm not familiar with STO_MIPS_MICROMIPS so I'm not sure if this is a breaking change or not.

sdardis requested changes to this revision.May 17 2018, 5:33 AM

This change isn't quite correct, as microMIPS traditionally doesn't mark inline data (.word and friends) which have a symbol associated with them as being microMIPS. The linker needs to know what symbols are microMIPS / MIPS to support interlinking the two. Inline data isn't code, so marking the symbol associated with it as microMIPS could result in buggy linkers seeing a microMIPS symbol in an address calculation, setting the lowest bit (as required for jumps to microMIPS symbols) leading to an unaligned load.

I believe this patch also needs to override EmitIntValue() as well so that the pending set of microMIPS symbols can be cleared.

See D6039 / rL221355.

This revision now requires changes to proceed.May 17 2018, 5:33 AM
asb added a comment.May 17 2018, 6:00 AM

Thanks for the explanation. In that case, the wrong marker is going to be produced for any of the target independent data directives that aren't handled specifically by the Mips asm parser (e.g. .4byte, .long etc) so this is definitely something that's worth fixing. That's probably a separate, parent patch to this one.

I suggest to abandon / close this review. Any objections?

asb abandoned this revision.Jul 4 2018, 7:45 AM

Sure, but it does sound like the handling of .4byte, .long may incorrectly set STO_MIPS_MICROMIPS in the current implementation so perhaps there's a bug to be filed.