The isMemWithSimmOffset predicate rejects relocations which is incorrect
behavior. Linkers and other tools should handle|warn|error when the
field overflows.
Details
Diff Detail
Event Timeline
LGTM with one nit about a comment.
lib/Target/Mips/AsmParser/MipsAsmParser.cpp | ||
---|---|---|
1077 | I don't think we need this comment but if we do want to keep it then please put it in front of the 'template' | |
1080 | Hmm... This isn't quite correct but it's no less correct than the other predicates. I'm happy for this to be committed as-is but we need to fix this and a couple other predicates afterwards. My concern is that checks for specific expression nodes will be confused by any variety. For example isConstantMemOff() is false for '1+1' because the top-level node is a MCBinaryExpr instead of an MCConstantExpr. Similarly, isMemWithSimmOffset() will unnecessarily reject things like '%lo(foo) + 2' when 'foo' is known. When 'foo' is unknown it will correctly reject it because %lo(foo)+2 is not a relocatable expression (it should be %lo(foo+2)). I think we should do this kind of test with something along the lines of: int64_t Offset; if (getMemOff()->evaluateAsAbsolute(Offset) && isShiftedInt<Bits, ShiftAmount>(Offset)) return true; return getMemOff()->evaluateAsRelocatable(Value, ...); Possibly with some checks that MCValue::RefKind agrees with the size of the field in the instruction |
I don't think we need this comment but if we do want to keep it then please put it in front of the 'template'