Include cmm-sources when linking shared objects

[mpickering]

Fixes #7182

Patch is by @bgamari, I have just rebased it on master as requested by @phadej in #7183

---

Please include the following checklist in your PR:

• Patches conform to the coding conventions.
• Any changes that could be relevant to users have been recorded in the changelog (add file to changelog.d directory).
• The documentation has been updated, if necessary.

Please also shortly describe how you tested your change. Bonus points for added tests!

[mpickering]

Anyone fancy merging this patch?

[bgamari]

This should also end up in the 3.6 branch.

[fgaz]

echoing @phadej's question:

This change is something else? We don't need cmmObjs nor asmObjs however?

[mpickering]

I don't really understand the question. Before the files weren't even passed the the linker, now they are. It is possible that these other files are also needed but this patch fixes the issue for cxxObjs, as claimed, and other files types are out-of-scope.

[fgaz]

I don't really understand the question.

Rephrasing as I understand it: Is this part of the "Include cmm-sources and asm-sources when linking shared objects" goal? If yes, why aren't cmmObjs and asmObjs included here too (as in cProfObjs/cSharedObjs)?

this patch fixes the issue for cxxObjs

wasn't it cmm and asm?

[mpickering]

cObjs contains all the objs you list as it is constructed from cLikeFiles (which contains asm, cmm, c etc).

cabal/Cabal/src/Distribution/Simple/GHC.hs

Line 554 in 5f69122

let cLikeFiles = fromNubListR $ mconcat

[phadej]

The confusion steps from having lines

      cObjs       = map (`replaceExtension` objExtension) cLikeFiles 

and

      cObjs               = map (`replaceExtension` objExtension) cSrcs

Rename the first to cLikeObjs (and all other related "just" C) and then confusion will be no more.

In particular this is confusing because it seems that there is split to "C, asm, cmm" and "C++" (cxx). However cxxSources are in cLikeFiles.

THere is code like

    let cProfObjs            = map (`replaceExtension` ("p_" ++ objExtension))
                               (cSources libBi ++ cxxSources libBi)
        cSharedObjs          = map (`replaceExtension` ("dyn_" ++ objExtension))
                               (cSources libBi ++ cxxSources libBi)

why that doesn't include cmm and asm files? They don't have profiling variants?

There are a lot of questions, and variable naming & code structure doesn't give answers.

[mpickering]

I am not sure if @phadej is requesting changes or just suggesting an improvement. We didn't modify the names in this confusing code.

This patch is critical to build ghc-debug with 9.2 so needs to be in the 3.6 branch.

[phadej]

@mpickering I'm not a maintainer of Cabal, so my comments can be ignored.

[emilypi]

Thanks @mpickering - this is clear and even tests!

[bgamari]

For what it's worth, I agree with @phadej that the current naming is confusing; I struggled to figure out its semantics when I first wrote the patch as well. It would be quite reasonable to carry out @phadej's proposed renaming in a follow-up patch.

[mpickering]

Anyone want to merge this?

[emilypi]

As long as we're agreed there will be a follow up for @phadej's commentary, yes @mpickering - consider it merged

[phadej]

Can we have an issue for

I struggled to figure out its semantics when I first wrote the patch as well. It would be quite reasonable to carry out @phadej's proposed renaming in a follow-up patch.

Just to remind us when this code is touched another time.

[bgamari]

@emilypi, can you also backport to the 3.6 branch?

[fgaz]

@bgamari There is no 3.6 branch yet

[fgaz]

* [x]  Any changes that could be relevant to users have been recorded in the changelog (add file to `changelog.d` directory).

The changelog entry isn't there. @mpickering or @bgamari could you add one in a follow-up pr?

[bgamari]

@bgamari There is no 3.6 branch yet

Ahh, I see. Cabal hasn't forked yet; support for GHC 9.2 has merely been added.

The changelog entry isn't there. @mpickering or @bgamari could you add one in a follow-up pr?

Sure.