Remove old x86 Directives #3948
Conversation
spiessimon
force-pushed
the
new-directives-x86-remove-old-ones
branch
from
7bf1756 to
c73e03b
Compare
| (* TODO: Buffer.length = 0 => set section align *) | ||
| let pos = Buffer.length b.buf in | ||
| let current = pos mod n in | ||
| if current > 0 then | ||
| let n = n - current in | ||
| if data then | ||
| match data with | ||
| | Asm_targets.Asm_directives_new.Zero -> |
There was a problem hiding this comment.
I think you can just write "Zero" here (and "Nop" below), by virtue of constructor disambiguation
There was a problem hiding this comment.
I tried that earlier and it triggers warning 40 (i.e., the disambiguation will stop working if the type is no longer clear from context).
| | Indirect_symbol _ -> assert false | ||
| | Type (lbl, kind) -> (get_symbol b lbl).sy_type <- Some kind | ||
| | Size (lbl, cst) -> ( | ||
| | Directive (D.Comment _ )-> () |
There was a problem hiding this comment.
As below (re. constructor disambiguation), you can remove the "D." prefixes I think
backend/x86_binary_emitter.ml
Outdated
| | C.Unsigned_int n, Sixteen -> buf_int16L b (Numbers.Uint64.to_int64 n) | ||
| | C.Unsigned_int n, Thirty_two -> buf_int32L b (Numbers.Uint64.to_int64 n) | ||
| | C.Unsigned_int n, Sixty_four -> buf_int64L b (Numbers.Uint64.to_int64 n) | ||
| | _, Eight -> |
There was a problem hiding this comment.
I think you should enable warning 4 in this file (see warning directive at the top), then expand these underscores. If that is too much work, at least enable the warning locally for this function, and do the same expansion.
There was a problem hiding this comment.
I've enabled it on the function. Turning it on for the entire file currently leads to 94 warnings (after enabling and fixing it for this function). Do you see another function where you think it would be worth enabling the warning?
| && (not (Char.equal c '"' (* '"' *))) | ||
| && not (Char.equal c '\\') | ||
| && (not (Char.equal c '"')) | ||
| (* '"' *) && not (Char.equal c '\\') |
There was a problem hiding this comment.
What is the purpose of this comment?
There was a problem hiding this comment.
I have no idea. It's in the original code in x86_proc.ml and the code in emitaux.
| let last_was_escape = ref false in | ||
| for i = 0 to String.length s - 1 do | ||
| for i = k to k + n - 1 do |
There was a problem hiding this comment.
I think there should probably be a bounds check before this loop, so a proper error can be given
There was a problem hiding this comment.
I added a bounds check for going beyond the end of the string and for passing negative values.
| @@ -403,9 +449,12 @@ module Directive = struct | |||
| (* masm only *) | |||
| | External _ -> assert false | |||
| | Reloc { offset; name; expr } -> | |||
| bprintf buf "\t.reloc\t%a, %s, %a" Constant.print offset | |||
| bprintf buf "\t.reloc\t%a, %s, %a" | |||
| (Constant.print ~force_digits:false) | |||
There was a problem hiding this comment.
I think it's a shame these other call sites have become polluted with "force_digits", which is only used in two cases. Maybe have two functions?
There was a problem hiding this comment.
I defined Constant.print (which will set it to false) and Constant.print_using_decimals (for the special case that sets it to true).
This PR removes the old x86 directives in favor of the new ones. This notably includes using the new logic for printing directives instead of the existing x86 logic for printing directives. The changes that were observed in the emitted assembly code are summarized in PR #3951.