Allow compilation of C/C++/ObjC/ObjC++ files with module from TH
ClosedPublic

Authored by bitonic on Mar 5 2017, 8:47 AM.

Details

Summary

The main goal is to easily allow the inline-c project (and
similar projects such as inline-java) to emit C/C++ files to
be compiled and linked with the current module.

Moreover, addCStub is removed, since it's quite fragile. Most
notably, the C stubs end up in the file generated by
CodeOutput.outputForeignStubs, which is tuned towards
generating a file for stubs coming from capi and Haskell-to-C
exports.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
bitonic created this revision.Mar 5 2017, 8:47 AM
bitonic updated this revision to Diff 11575.Mar 5 2017, 8:50 AM
  • Remove useless option from test
bitonic added inline comments.Mar 5 2017, 9:01 AM
testsuite/tests/th/T13366.stdout
2

@bgamari I'm puzzled on why the ordering is as it is here. When outputting to stdout directly I get the ordering we'd expect, that is

calling f(2)
3
calling fcxx(5)
6

But when redirecting to a file I get this. I'll take a closer look later.

facundominguez edited edge metadata.Mar 6 2017, 8:26 AM

https://ghc.haskell.org/trac/ghc/ticket/13366 talks about controlling compilation flags, but it doesn't look like this patch is addressing that. Perhaps update the ticket to explain the plan?

testsuite/tests/th/T13366.stdout
2

Perhaps buffering plays a role here. I don't know if Haskell and C code would both use the same buffer.

bitonic updated this revision to Diff 11590.Mar 6 2017, 9:23 AM
  • Remove useless option from test
bgamari requested changes to this revision.Mar 6 2017, 12:34 PM

I think this could still use a bit of honing.

compiler/deSugar/Desugar.hs
103

Hmm, I'm not sure I like this repetition. Why not instead just have,

data ForeignSrcLang = LangC | LangCxx | LangObjC | Lang ObjCxx

and tcg_th_foreign_srcs :: IORef [(ForeignSrcLang, String)]. This would also allow us to avoid passing around Phases everywhere, eliminating a number of partial matches.

compiler/main/DriverPipeline.hs
1549

I wonder if this should instead just be an identity.

This revision now requires changes to proceed.Mar 6 2017, 12:34 PM
dfeuer requested changes to this revision.Mar 6 2017, 12:46 PM
dfeuer added a subscriber: dfeuer.

There seems to be quite a bit of code duplicated among the different C-line languages. Could you factor that out in some fashion?

compiler/deSugar/Desugar.hs
191

I think this should probably be written more like cc_files = concat [map (con,) files | (con, files) <- (Cc, cfiles), (Ccxx, cxxfiles), ...].

compiler/main/CodeOutput.hs
93

You could drop this tiny do block in favor of (phase,) <$> outputCcFile.... if you like.

278

This constructor-to-extension logic should be in a separate function, IMO.

compiler/main/DriverPipeline.hs
89

Would it make sense to upgrade these maybes to custom types with self-describing constructors?

1551

Why is this null a panic? Can you restructure the code to avoid being concerned if that's empty? Is there some important optimization in the "no foreign" case?

bitonic updated this revision to Diff 11594.Mar 6 2017, 4:17 PM
bitonic edited edge metadata.
  • Address PR comments

I've replied to the comments, I think most of them are styling comments or refactors, and I'd be wary of starting refactoring existing code because of my changes. I'd like to stick to what we need for this improvements and reserve refactors for later.

Regarding style, how does it work on the GHC codebase? Is there a styling guide that everybody abides to, or some "styling arbiter"? It's quite tedious to fix styling on a PR, and if it can be avoided by just knowing what people expect, I'd be happier.

compiler/deSugar/Desugar.hs
103

Done

191

I find your code less clear, but irrelevant now since I've removed the separate variables.

compiler/main/CodeOutput.hs
93

I think this is a matter of style, and I find the do easier on the reader. Is there a GHC style guide or something like that?

278

This is the only place where it would be used, and again I think that forcing the reader to follow the indirection needlessly would lead to a worse code coding experience.

compiler/main/DriverPipeline.hs
89

Maybe, but I wouldn't start refactoring code in a PR addressing a very specific concern.

1549

I'm preserving the previous behavior here. It could have been the identity before, too.

1551

Again, throughout the code I preferred keeping things as close as possible to the old code.

bitonic marked an inline comment as done.Mar 6 2017, 4:19 PM
bgamari added inline comments.Mar 6 2017, 7:22 PM
compiler/deSugar/Desugar.hs
191

There's no reason to project the ForeignLang to a Phase so quickly. I would do it much later (perhaps in DriverPipeline).

compiler/main/CodeOutput.hs
54

Surely this should take a ForeignLang instead of a Phase

bitonic added inline comments.Mar 6 2017, 7:26 PM
compiler/deSugar/Desugar.hs
191

The reason why I did not do that is that if I define ForeignSrcLang in TcRnTypes and I import it from Desugar I have a loop. I could define it in HscTypes, but at the same time by this stage I don't think it's detrimental to make it clear that these files will be compiled by the CC pipeline. In any case I don't really care, if you prefer it to be defined in HscTypes, that's fine.

compiler/main/CodeOutput.hs
54

This hinges on the resolution for https://phabricator.haskell.org/D3280#inline-27076 , let's wait for it to be resolved.

bitonic updated this revision to Diff 11602.Mar 6 2017, 7:54 PM
  • Carry ForeignSrcLang around a bit longer
bitonic updated this revision to Diff 11603.Mar 6 2017, 7:55 PM
  • Remove useless imports
bitonic marked 2 inline comments as done.Mar 6 2017, 7:56 PM
rwbarton added inline comments.
libraries/ghci/GHCi/Message.hs
248–249

Could we have a single constructor parameterized on ForeignSrcLang instead? It's easy to imagine wanting to add more languages at a future point and that would give rise to some awkwardness in the Binary instance.

testsuite/tests/th/T13366.stdout
2

Right, they definitely don't use the same buffer. It might be a good idea to add explicit flushes to both the Haskell and C++ code to prevent the order from changing.

bitonic marked an inline comment as done.Mar 7 2017, 9:03 AM
bitonic updated this revision to Diff 11612.Mar 7 2017, 9:04 AM
  • Flush buffers in test for predictable output
bitonic updated this revision to Diff 11613.Mar 7 2017, 10:56 AM
  • Make ForeignSrcLang part of the TH API
bitonic marked an inline comment as done.Mar 7 2017, 10:56 AM
facundominguez added inline comments.Mar 7 2017, 11:03 AM
libraries/template-haskell/Language/Haskell/TH/Syntax.hs
466

-optc

bitonic updated this revision to Diff 11614.Mar 7 2017, 11:04 AM
  • Make ForeignSrcLang part of the TH API
bitonic marked an inline comment as done.Mar 7 2017, 11:05 AM
bgamari accepted this revision.Mar 7 2017, 11:32 AM

Thanks for sticking with this, @bitonic. This looks great!

dfeuer accepted this revision.Mar 7 2017, 3:05 PM

Definitely cleaner now. I still think it's a bit odd that the rather boring tasks of mapping source language to phase and to file extension are buried inside functions that do other things, rather than extracted out into just a couple functions/records/whatever that can be stuck next to each other at the top level, but I guess it doesn't matter much.

compiler/main/DriverPipeline.hs
1070

maybe [] return stub_o ++ ... looks kind of weird. One lousy option is maybe id (:) stub_o foreign_os. Another lousy option is foldMap pure stub_o ++ foreign_os. Yet another option is to just use case.

1405

Why not

unless (null foreign_os) $ do ...
This revision is now accepted and ready to land.Mar 7 2017, 3:05 PM
bitonic updated this revision to Diff 11625.Mar 7 2017, 4:57 PM
  • Add ForeignSrcLang, fix .in file
bitonic updated this revision to Diff 11626.Mar 7 2017, 5:12 PM
  • Remove binary dependency from TH
bitonic updated this revision to Diff 11627.Mar 7 2017, 6:06 PM
  • Forgot to add another file
This revision was automatically updated to reflect the committed changes.