Adds support for xray instrumentation on mips for both 32-bit and 64-bit.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
Thanks for the patch!
I'm not an expert on the mips assembly, so I'll defer to someone else who is (or if you are confident this is "doing the right thing" already, then I'm happy to defer to your judgment here).
@rSerge ran into the same issue I comment on later, we probably need to think about refactoring that logic into a common function higher up in the base class just to consolidate the logic for emitting ELF tables. I'm happy to wait to do the consolidation later, but will ask that you make that change here as well.
lib/Target/Mips/MipsAsmPrinter.cpp | ||
---|---|---|
1101–1103 | You probably want to do something similar to what we do in x86 where we determine whether the function being instrumented is in a comdat group (and handle that case appropriately). I think we already do that for arm7 (no thumb) and aarch64 too. |
Added handling of the case where function being instrumented is in a comdat group, as suggested.
Some comments inlined. I've highlighted a potential pic bug regarding the clobbering of $gp.
This needs some more tests, such as functions calling functions local to the TU, and functions calling external functions, ensuring that gp is not clobbering by accident.
lib/Target/Mips/MipsAsmPrinter.cpp | ||
---|---|---|
1052 | This should be const unsigned NoopsInSledCount. | |
1065 | You also need to save t9 here for pic code. If we have something like: unsigned short g(unsigned b); unsigned short m (unsigned short a) { return g(a); } unsigned short g (unsigned b) { return b + b + 5; } The prologue of m will contain: lui $2, %hi(_gp_disp) addiu $2, $2, %lo(_gp_disp) addu $2, $2, $25 calculating the gp for this function group. Without saving t9 on entry, after Please add the example as a test. | |
1067 | You're adding the lower half of __xray_FunctionEntry/Exit to t9 here, not the upper half. Secondly, looking at the corresponding implementation in compiler rt, this should be ORI, as if bit 15 of the address of __xray_FunctionEntry is set, that half will be sign extended. |
I'm being hit with an llvm assertions when trying to use the integrated assembler about the function size when not being absolute.
Can you repost the patch with comments addressed, and take a quick look to see if basic sample programs can be instrumented?
Thanks,
Simon
lib/Target/Mips/MipsAsmPrinter.cpp | ||
---|---|---|
1068 | Ignore this, I'm being hit by phabricator bugs. | |
1076 | I am somewhat sure about this in the case of the produced being non-pic as the gp pointer doesn't (AFAIK) need be changed. Add a FIXME: Is this correct for the static relocation model. | |
1081 | corretly -> correctly. | |
1092–1095 | This is incorrect. GAS will build a pc relative relocation with the value <func> + 0x24, which will overflow. Instead use the temporary symbol, Target, and use that instead. Also, update the comment on lines 1089 to reflect that we're branching to a label, not a pc-relative offset. Likewise for the below branches, | |
1108 | After testing this, you need to emit the label Target before the addiu. Otherwise we get a SEGV fault. | |
test/CodeGen/Mips/xray-mips-attribute-instrumentation.ll | ||
8 | See my comment above about branching to labels. |
Addressed review comments. I've also added mips64 support for xray along with this change since the patch was ready.
I'm being hit with an llvm assertions when trying to use the integrated assembler about the >function size when not being absolute.
Can you repost the patch with comments addressed, and take a quick look to see if basic sample >programs can be instrumented?
Sure, Ill compile some more examples with the integrated assembler and check if I can reproduce the assertion.
lib/Target/Mips/MipsAsmPrinter.cpp | ||
---|---|---|
1100–1103 | This sequence needs to be: lui t9, // %highest(__xray_FunctionEntry/Exit) ori t9, // %higher(__xray_FunctionEntry/Exit) dsll t9, t9, 16 ori t9, t9, // %hi(__xray_FunctionEntry/Exit) dsll t9, t9, 16 ori t9, t9, // %lo(__xray_FunctionEntry/Exit) |
LGTM. Please see my inlined comment about the test case - it hits a bug in llvm-objdump.
test/CodeGen/Mips/xray-section-group.ll | ||
---|---|---|
2–13 | Running this with ToT shows a bug in llvm-objdump when using -disassemble-all. It attempts to disassemble the string table which is 173 bytes long as instructions, so when it attempts to disassemble the "instruction" starting at offset 172, it crashes on an invalid array access. For the moment, use llvm-readobj to determine the presence of sections and their corresponding flags. |
This should be const unsigned NoopsInSledCount.