Page MenuHomePhabricator

Improve error message for bad SHF_MERGE sections
ClosedPublic

Authored by ruiu on Oct 9 2019, 9:21 PM.

Diff Detail

Event Timeline

ruiu created this revision.Oct 9 2019, 9:21 PM
Herald added a project: Restricted Project. · View Herald Transcript
MaskRay added inline comments.Oct 9 2019, 11:52 PM
lld/ELF/InputFiles.cpp
520

Make it a bit more detailed, e.g.

section size (1337) must be a multiple of sh_entsize (4)

ruiu updated this revision to Diff 224268.Oct 10 2019, 12:17 AM
  • address review comments
  • merged with a existing test file
MaskRay accepted this revision.Oct 10 2019, 12:35 AM

LGTM.

lld/test/ELF/invalid/merge-writable.s
1

git mv writable-merge.s merge-writable.s

This revision is now accepted and ready to land.Oct 10 2019, 12:35 AM
ruiu marked an inline comment as done.Oct 10 2019, 12:40 AM
ruiu added inline comments.
lld/test/ELF/invalid/merge-writable.s
1

This file is actually already merge-writable.s. Am I missing something?

MaskRay added inline comments.Oct 10 2019, 1:11 AM
lld/test/ELF/invalid/merge-writable.s
1

There is an existing file writable-merge.s

I think we can reuse the test, though probably rename it to merge-writable.s

This revision was automatically updated to reflect the committed changes.
grimar added inline comments.Oct 10 2019, 1:56 AM
lld/test/ELF/invalid/merge-invalid-size.s
4

You should probably be able to do something like the following I think:

// ... | FileCheck %s - DFILE=%t.o
// CHECK: [[FILE]]:(.foo): SHF_MERGE section size (2) must be a multiple of sh_entsize (4)

This patch adds a section name to error messages.

The summary needs an update.

ruiu marked an inline comment as done.Oct 10 2019, 3:01 AM
ruiu added inline comments.
lld/test/ELF/invalid/merge-invalid-size.s
4

I'm not sure if that use of -D makes things easier, so I'd like to stick with this.

grimar added inline comments.Oct 10 2019, 3:05 AM
lld/test/ELF/invalid/merge-invalid-size.s
4

Hardcoding something is indeed easier. Problem is that here you depend on the naming rules.
I.e. what if ".tmp." suffix changes to something else? Such tests will fail.
FWIW in llvm tools test cases -D is used heavily.

ruiu marked an inline comment as done.Oct 10 2019, 3:12 AM
ruiu added inline comments.
lld/test/ELF/invalid/merge-invalid-size.s
4

Yeah, you are right, though I'm not too worried about that. If writing a full filename here is too much, I'd change this to .o:(.foo) so that it is not too picky about filenames.

grimar added inline comments.Oct 10 2019, 3:21 AM
lld/test/ELF/invalid/merge-invalid-size.s
4

That is in line with what we usually did in LLD before and sounds good to me.