New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Location.none is broken with the LLVM assembler in 4.03.0+beta2 #7226
Comments
Comment author: @lefessan Is it a problem of OCaml, or a problem of the ppx that emits Location.none in a place where OCaml is expecting a true location ? |
Comment author: @gasche I see several ways to work around the problem: (1) change the column-printing code of #212 to not emit the column when it is negative Options (1) seems best to me. If you want to submit a PR, that would be very nice indeed. If you want it to be considered for inclusion into 4.03, it should be as non-invasive and "obviously non-regressing" as possible. (We could also consider reverting #212 for 4.03 to be in the clear, but I'd rather not do that.) |
Comment author: antron It's a problem of OCaml. If you are writing or maintaining a flawed PPX that emits Location.none, and you accidentally only use Location.none in constructs that don't cause emission of .loc directives, you won't notice that you have a problem. And, if you use Location.none in constructs that do cause emission of .loc, but your OCaml is configured to assemble using the GNU toolchain, you still won't notice it. If somebody else then tries to use your PPX on OCaml configured with the Clang toolchain, they will get a number of opaque error messages coming from the assembler. The points are:
If it is genuinely important for OCaml that locations of expressions or other constructs are not Location.none, then I think the compiler should be modified to either check for Location.none, or to prevent or strongly discourage construction of ASTs with Location.none. |
Comment author: @gasche It's not even clear to me that using Location.none if ppx rewriters is a bad choice in the first place. For code that comes from user code, using user code locations is better, but there may be situations where it's reasonable to say that a particular code has "no location". Let's get over this debate and fix the bug. |
Comment author: antron Ok, I'll do (1). Thanks. |
Comment author: @lefessan Except that, when debugging a program, faulty ppx programs have the same impact on debuggers with "gas" (where there is a location, but it is incorrect) as with "clang" (if the location is removed). I think the best solution would be, at least, to issue a warning ("your ppx generates code with incorrect locations"), so that users can send bug reports upstream to ppx developers. |
Comment author: @damiendoligez @lefessan how hard would it be to emit such a warning? I say let's release 4.03 with the simple patch, and add the warning in trunk. |
Comment author: @mmottl I have just run into apparently the exact same issue with OCaml 4.03+flambda. Not sure why this happens with Flambda turned on. The issue does seem to be fixed with the plain vanilla OCaml 4.03 OPAM switch. |
Comment author: @gasche I cannot reproduce under either 4.03-flambda or trunk-flambda. |
Comment author: @mmottl I've created a small test case, reproducible with OPAM 4.03.0+flambda on Mac OS X. File "foo.ml":type 'a t = 'a [@@deriving bin_io]Compile it with: ocamlfind ocamlopt -c -g -package ppx_bin_prot -o foo.cmx foo.ml The important bit above is "-g". Without it the problem does not happen, neither does it without Flambda. |
Original bug ID: 7226
Reporter: antron
Assigned to: @gasche
Status: closed (set by @xavierleroy on 2017-09-24T15:32:57Z)
Resolution: fixed
Priority: normal
Severity: major
Target version: 4.03.0+dev / +beta1
Fixed in version: 4.03.0+dev / +beta1
Category: back end (clambda to assembly)
Monitored by: @gasche @mmottl
Bug description
LLVM's assembler does not (apparently) accept negative column numbers, e.g.
.loc 2 1 -1
The result is that it is not possible to compile on systems that use Clang for assembly, with the -g flag, with a PPX that sometimes uses Location.none, because Location.none has a column number of -1:
/var/folders/wr/52wphbkn0zx5nvbx61v640nm0000gn/T/camlasmef96b3.s:2804:11: error: unexpected token in '.loc' directive
.loc 2 1 -1
^
Steps to reproduce
foo.s:
.file 1 "none"
.loc 1 1 -1
Then:
clang foo.s
as foo.s # If this is gas, it should accept the -1.
ppx_loc_bug.ml:
(* Changes location of all application expressions to Location.none. *)
open Asttypes
open Parsetree
open Ast_mapper
let mapper _ =
{default_mapper with expr = fun mapper e ->
match e with
| {pexp_desc = Pexp_apply _} -> {e with pexp_loc = Location.none}
| _ -> default_mapper.expr mapper e}
let () =
register "ppx_loc_bug" mapper
test.ml:
let () =
print_endline "Hello world!"
Makefile:
PPX = ppx_loc_bug
TEST = test
.PHONY : build$(PPX) $ (PPX).ml
build : clean
ocamlfind opt -linkpkg -package compiler-libs.common -o
.PHONY : clean$(PPX) $ (TEST) *.cm[iox] *.o
clean :
rm -f
Additional information
This was apparently introduced by #212
I propose to solve it by not emitting the column number if it is negative, i.e.
.loc 2 1
If this is reasonable, I can make a PR.
The text was updated successfully, but these errors were encountered: