Don't barf() on failures in loadArchive()
ClosedPublic

Authored by DemiMarie on Oct 27 2016, 6:16 PM.

Details

Summary

This patch replaces calls to barf() in loadArchive() with proper
error handling.

Test Plan

GHC CI

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.
DemiMarie updated this revision to Diff 9198.Oct 27 2016, 6:16 PM
DemiMarie retitled this revision from to Don't barf() on failures in loadArchive().
DemiMarie updated this object.
DemiMarie edited the test plan for this revision. (Show Details)
DemiMarie updated the Trac tickets for this revision.

Note that this patch requires D2642 in order to work.

DemiMarie edited edge metadata.Oct 27 2016, 11:03 PM
DemiMarie added a project: GHC.
DemiMarie updated this revision to Diff 9208.Oct 27 2016, 11:11 PM

Fix lint warnings

DemiMarie updated this revision to Diff 9209.Oct 27 2016, 11:12 PM
DemiMarie edited edge metadata.

Fix overly-large patch

DemiMarie updated this revision to Diff 9210.Oct 27 2016, 11:18 PM
DemiMarie edited edge metadata.

Fix more lint warnings

DemiMarie updated this revision to Diff 9211.Oct 27 2016, 11:22 PM
DemiMarie edited edge metadata.

Fix more lint warnings

bgamari edited edge metadata.EditedNov 3 2016, 7:26 AM

Hi @DemiMarie!

I just landed a rather large split-up of the linker (D2643 through D2650). Unfortunately this means that this patch may be a bit hard to rebase; I would be happy to do this on your behalf if you would prefer. Regardless, I'll try to rebase D2642 today. Sorry for the inconvenience!

Cheers,

~ Ben

@DemiMarie, can I help in any way with this?

Hi @DemiMarie!

I just landed a rather large split-up of the linker (D2643 through D2650). Unfortunately this means that this patch may be a bit hard to rebase; I would be happy to do this on your behalf if you would prefer. Regardless, I'll try to rebase D2642 today. Sorry for the inconvenience!

Cheers,

~ Ben

@DemiMarie, can I help in any way with this?

This is really my first time in the guts of the runtime; I am also very new to Phabricator. Does this patch need to be rebased? What needs to be done before it can be merged?

erikd edited edge metadata.Nov 21 2016, 1:44 AM

Does this patch need to be rebased?

Yes. I've you've been doing this work on the master branch, then git pullall --rebase should do the trick. If you are on a branch, checkout master and do 'git pullall` then checkout your branch and do git rebase master.

Once you've done that you can run arc diff again to refresh the patch on phabricator (ie the rebased version).

bgamari requested changes to this revision.Nov 21 2016, 11:07 AM
bgamari edited edge metadata.

This is really my first time in the guts of the runtime; I am also very new to Phabricator. Does this patch need to be rebased? What needs to be done before it can be merged?

Indeed it needs to be rebased; I have done a fair amount of clean-up in this area since you opened this and I apologize for the effort that it will take to rebase this. I generally try to avoid shifting the sands underneath contributors like this but I think that the changes I made should make it much easier to review the change you are making here. If you want help with rebasing let me know.

This revision now requires changes to proceed.Nov 21 2016, 11:07 AM
DemiMarie updated this revision to Diff 9556.Nov 22 2016, 9:59 AM
DemiMarie edited edge metadata.

Rebased to current master

DemiMarie updated this revision to Diff 9557.Nov 22 2016, 11:34 AM
DemiMarie edited edge metadata.
  • Allow user to override stage 0 programs
DemiMarie updated this revision to Diff 9558.Nov 22 2016, 11:35 AM
DemiMarie edited edge metadata.
  • Revert "Allow user to override stage 0 programs"
DemiMarie updated this revision to Diff 9596.Nov 23 2016, 5:27 PM
DemiMarie edited edge metadata.

Fix a bogus GCC warning

The warning was bogus (the code paths on which the value would be
used uninitialized are unreachable), but GCC can't determine this
automatically. Shut it up by initializing the variable with
SIZE_MAX – if this is ever used as a length, it will try to clobber
all memory and segfault almost immediately.

DemiMarie updated this revision to Diff 9608.Nov 24 2016, 6:18 PM
DemiMarie edited edge metadata.

Fix checks for symbol tables

DemiMarie updated this revision to Diff 9610.Nov 24 2016, 7:35 PM
DemiMarie edited edge metadata.

Make the checks for symbol table files more strict

bgamari requested changes to this revision.Dec 2 2016, 9:57 AM
bgamari edited edge metadata.

Getting there. It looks like there are some validation problems on OS X: https://phabricator.haskell.org/harbormaster/build/15957/.

rts/linker/LoadArchive.c
34

Today I learned about static array size checks. Thanks!

However, isn't sizeof(uint32_t) a rather verbose way of writing 4?

37

Why is this memcpy needed? Isn't ntohl(*(uint32_t*) target) sufficient?

77

This message is a bit unclear. Perhaps, "Fat archive contains %d architectures but none of them are compatible with the host".

This revision now requires changes to proceed.Dec 2 2016, 9:57 AM
DemiMarie updated this revision to Diff 9772.Dec 2 2016, 2:16 PM
DemiMarie edited edge metadata.

Fix compile errors and bad error message on Darwin

DemiMarie marked 3 inline comments as done.Dec 2 2016, 2:17 PM
DemiMarie added inline comments.
rts/linker/LoadArchive.c
37

The usual reason for the memcpy is to avoid invoking undefined behavior by violating the strict aliasing rule. However, the RTS is compiled with -fno-strict-aliasing (which I forgot when I wrote this code), so this is okay.

DemiMarie marked an inline comment as done.Dec 2 2016, 2:17 PM
DemiMarie added inline comments.
rts/linker/LoadArchive.c
34

It is; now fixed

bgamari updated this revision to Diff 9823.Dec 6 2016, 10:46 PM
bgamari edited edge metadata.
  • Fix bool uses
bgamari accepted this revision.Dec 6 2016, 10:47 PM
bgamari edited edge metadata.

There were a few build issues due to another RTS refactoring that I merged last week (428e152be6bb0fd3867e41cee82a6d5968a11a26). I've updated this diff for you to account for this change; I hope you don't mind. Otherwise if this Harbormaster build succeeds then I'll go ahead and merge this.

Thanks @DemiMarie!

This revision is now accepted and ready to land.Dec 6 2016, 10:47 PM
This revision was automatically updated to reflect the committed changes.