rts/Linker.c: define the 'mmap_again' label only for x86_64
AbandonedPublic

Authored by alpmestan on Oct 17 2018, 4:31 AM.

Details

Summary

We only ever goto mmap_again when the host arch is x86_64, which
made the build fail on i386, complaining that mmap_again is defined but not
used anywhere. This patch simply moves the label definition inside the
#if defined(x86_64_HOST_ARCH) block.

Test Plan

Build the RTS on i386

alpmestan created this revision.Oct 17 2018, 4:31 AM
alpmestan edited the summary of this revision. (Show Details)Oct 17 2018, 4:31 AM
alpmestan added a comment.EditedOct 17 2018, 7:59 AM

The i686 build now fails, complaining that most of the variables we define are unused.

This made me realize that we run that mmap_again label the first time around, without jumping to it with goto, and therefore my patch is wrong so far. I guess I can just put a goto mmap_again; statement right before its definition.

It's a bit annoying that the C compiler doesn't figure out that it's not exactly useless, we do run that code but never jump to it explicitly. If my understanding is correct.

alpmestan updated this revision to Diff 18349.Oct 17 2018, 8:08 AM
  • previous fix was wrong, let's try an explicit goto to silence the error

There's an i686 (Circle CI) build running for this diff here: https://circleci.com/gh/ghc/ghc-diffs/479

Thank you for fixing this.

rts/Linker.c
1012–1013

Add a comment for why we need this dummy goto?

bgamari requested changes to this revision.Oct 17 2018, 9:39 AM

I'm a bit lost. Is the problem that mmap_again is considered to be unused since the compiler has shown all gotos are unreachable?

If so I think we might be better off just refactoring the function to avoid the goto entirely. This code is already tricky to grok.

This revision now requires changes to proceed.Oct 17 2018, 9:39 AM

I'm a bit lost. Is the problem that mmap_again is considered to be unused since the compiler has shown all gotos are unreachable?

Yes. That's what the error was about.

If so I think we might be better off just refactoring the function to avoid the goto entirely. This code is already tricky to grok.

I'm inclined to agree, given that my updated patch makes 300+ tests fail. :-)

alpmestan updated this revision to Diff 18358.Oct 17 2018, 4:13 PM
  • only define mmap_again label when on x86_64

https://circleci.com/gh/ghc/ghc-diffs/484 is the build for my latest update. So, let me sum it up: on i686, not having this mmap_again label there is enough to make 300+ tests fail, even though (still on i686) we _never_ jump to it. I'll have to try and reproduce this on a small isolated example to figure out what's going on at the assembly level, none of this makes sense to me.

I'm a bit lost. Is the problem that mmap_again is considered to be unused since the compiler has shown all gotos are unreachable?

All ‘goto‘s are inside the ‘#if defined x86_64...‘ so on other platforms none of the ‘goto‘s will be seen by the compiler because the preprocessor removed that code already.