Mantis Bug Tracker

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0007226OCamlback end (clambda to assembly)public2016-04-13 17:332017-09-24 17:32
Reporterantron 
Assigned Togasche 
PrioritynormalSeveritymajorReproducibilityalways
StatusclosedResolutionfixed 
PlatformOSOS Version
Product Version 
Target Version4.03.0+dev / +beta1Fixed in Version4.03.0+dev / +beta1 
Summary0007226: Location.none is broken with the LLVM assembler in 4.03.0+beta2
DescriptionLLVM'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 Reproduce1. To check for such an assembler:

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.


2. To trigger this failure through OCaml:

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
build : clean
    ocamlfind opt -linkpkg -package compiler-libs.common -o $(PPX) $(PPX).ml

    @echo " No Location.none: expected to succeed"
    ocamlfind opt -g -o $(TEST) $(TEST).ml

    @echo " No -g: expected to succeed"
    ocamlfind opt -ppx ./$(PPX) -o $(TEST) $(TEST).ml

    @echo " Location.none + -g: expected to fail on 4.03.0+beta2"
    ocamlfind opt -ppx ./$(PPX) -g -o $(TEST) $(TEST).ml

.PHONY : clean
clean :
    rm -f $(PPX) $(TEST) *.cm[iox] *.o
Additional InformationThis was apparently introduced by https://github.com/ocaml/ocaml/pull/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.
TagsNo tags attached.
Attached Files

- Relationships

-  Notes
(0015731)
lefessan (developer)
2016-04-13 17:46

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 ?
(0015733)
gasche (administrator)
2016-04-13 18:10
edited on: 2016-04-13 18:14

I see several ways to work around the problem:

(1) change the column-printing code of GPR#212 to not emit the column when it is negative
(2) change the default ghost location value to use a non-negative column number (but this has the risk of breaking people assumptions, so I wouldn't recommend it)
(3) disable column printing when using clang/llvm (I think this is not necessary, and not necessarily easy to do)

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* GPR#212 for 4.03 to be in the clear, but I'd rather not do that.)

(0015734)
antron (reporter)
2016-04-13 18:11

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:

1. There isn't uniform behavior across toolchains, and I am guessing GNU is the more common one. In practice, this means developers on Linux won't easily catch such a problem with their PPX, and will be in danger of unknowingly distributing buggy code.
2. The error message seen by the user really makes no sense until non-trivial investigation. In particular, there is no reference to a PPX. It can arise in some preprocessed files but not others, depending on which constructs each file contains.
3. Ast_helper.default_loc is set to Location.none. Combined with a tendency to skip ?loc arguments when using Ast_helper, the interface invites authors to make this mistake. Similarly, Ast_convenience in ppx_tools doesn't (yet) offer an obvious way to pass locations, and it takes some discipline to use [@metaloc] everywhere when using ppx_metaquot.
4. 4.02 happily accepted Location.none, whether that was supposed to be correct or not. Breaking this now may require a review of existing PPXs.
5. Sometimes there isn't really a sensible location to pick for a generated construct, and if a PPX author is reasonably certain that an error won't be reported on it, and the user can deal with the stack trace, the author may be tempted to use Location.none.

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.
(0015735)
gasche (administrator)
2016-04-13 18:13

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.
(0015736)
antron (reporter)
2016-04-13 18:17

Ok, I'll do (1). Thanks.
(0015737)
lefessan (developer)
2016-04-13 18:29

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.
(0015741)
doligez (administrator)
2016-04-14 16:24

@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.
(0015749)
gasche (administrator)
2016-04-14 19:31

Fixed in 4.03 ( 9c0558114a40a57ad159da0bc7a393f10208102b ) and trunk ( 0746566e79de16f125ddd4d413a393ff6d799adf ), by the patch proposed by antron. Thanks!
(0015970)
mottl (reporter)
2016-06-03 16:20

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.
(0015971)
gasche (administrator)
2016-06-04 13:18

I cannot reproduce under either 4.03-flambda or trunk-flambda.
(0015973)
mottl (reporter)
2016-06-04 15:03

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.

- Issue History
Date Modified Username Field Change
2016-04-13 17:33 antron New Issue
2016-04-13 17:46 lefessan Note Added: 0015731
2016-04-13 18:10 gasche Note Added: 0015733
2016-04-13 18:10 gasche Status new => confirmed
2016-04-13 18:11 gasche Target Version => 4.03.0+dev / +beta1
2016-04-13 18:11 antron Note Added: 0015734
2016-04-13 18:13 gasche Note Added: 0015735
2016-04-13 18:14 gasche Note Edited: 0015733 View Revisions
2016-04-13 18:17 antron Note Added: 0015736
2016-04-13 18:29 lefessan Note Added: 0015737
2016-04-14 16:24 doligez Note Added: 0015741
2016-04-14 19:31 gasche Note Added: 0015749
2016-04-14 19:31 gasche Status confirmed => resolved
2016-04-14 19:31 gasche Fixed in Version => 4.03.0+dev / +beta1
2016-04-14 19:31 gasche Resolution open => fixed
2016-04-14 19:31 gasche Assigned To => gasche
2016-06-03 16:20 mottl Note Added: 0015970
2016-06-04 13:18 gasche Note Added: 0015971
2016-06-04 15:03 mottl Note Added: 0015973
2017-02-23 16:35 doligez Category OCaml backend (code generation) => Back end (clambda to assembly)
2017-02-23 16:44 doligez Category Back end (clambda to assembly) => back end (clambda to assembly)
2017-09-24 17:32 xleroy Status resolved => closed


Copyright © 2000 - 2011 MantisBT Group
Powered by Mantis Bugtracker