[lld][ELF] Check length of subsection in .ARM.attributes

[evgeny777]

Summary

At the moment ld.lld hangs if .ARM.attributes section contains all zeroes

[ruiu]

tools/lld/test/ELF/bad-arm-attributes2.test

1 ↗

(On Diff #204242)

We don't use yaml2obj unless code is not representable in assembly. Is there any way to rewrite this in assembly?

[evgeny777]

Addressed

[ruiu]

LGTM with the following changes.

tools/lld/test/ELF/bad-arm-attributes.s

1 ↗

(On Diff #204265)

Please move this to tools/lld/test/ELF/invalid directory.

tools/lld/test/ELF/bad-arm-attributes2.s

1 ↗

(On Diff #204265)

Ditto

[grimar]

LGTM with one nit and Rui's comments addressed.

tools/lld/test/ELF/bad-arm-attributes.s

1 ↗

(On Diff #204265)

I think you also need to add # REQUIRES: arm

[MaskRay]

tools/lld/test/ELF/bad-arm-attributes2.s

2 ↗

(On Diff #204265)

%tout -> /dev/null

[MaskRay]

tools/lld/test/ELF/bad-arm-attributes2.s

2 ↗

(On Diff #204265)

Oh, wait.

// ELF/InputFiles.cpp

    Attributes.Parse(Contents, /*isLittle*/ Config->EKind == ELF32LEKind);

Parse doesn't return an error? That may be something that should be improved later..

[peter.smith]

Ironically bad-arm-attributes2.s test is failing on an Arm buildbot http://lab.llvm.org:8011/builders/clang-cmake-armv8-lld/builds/4/steps/ninja%20check%201/logs/FAIL%3A%20lld%3A%3Abad-arm-attributes2.s

invalid subsection length 4294967040 at offset 0

The expected is:

# CHECK: invalid subsection length 4294967295 at offset 1

I've not worked out why yet, it could be a 32/64-bit host issue.

[peter.smith]

I've not worked out why yet, it could be a 32/64-bit host issue.

Yes it appears to be the case. The offset starts at 1 and when 0xffffffff is added on a 32-bit machine the offset overflows back to 0

[evgeny777]

Strange, I haven't got notification. Are buildbot notifications working?
Anyway I think making Offset uint64_t instead of size_t will fix the issue.

[peter.smith]

In D63191#1543464, @evgeny777 wrote:

Strange, I haven't got notification. Are buildbot notifications working?
Anyway I think making Offset uint64_t instead of size_t will fix the issue.

I think this landed, when the lab.llvm.org was down, we're just catching up. One possible drawback of making Offset uint64_t is that the .ARM.attributes should only be seen in a 32-bit ELF file so there is never a legal attributes section size that large, but I don't think it matters that much. An alternative would be to drop the size field to 0xFE, 0xFF, 0xFF, 0xFF.

[evgeny777]

would be to drop the size field to 0xFE, 0xFF, 0xFF, 0xFF.

This would cause problems on arm-be, no?

[peter.smith]

In D63191#1543468, @evgeny777 wrote:

would be to drop the size field to 0xFE, 0xFF, 0xFF, 0xFF.

This would cause problems on arm-be, no?

That would give a different (smaller I think) size on BE yes, although LLD doesn't support big-endian at the moment. I kind of picked the first value that came into my head anyway, so by all means pick something else if you want to change the test.

lld/trunk/test/ELF/invalid/bad-arm-attributes.s

+# REQUIRES: arm
+# RUN: llvm-mc -filetype=obj -triple=arm-unknown-linux %s -o %t
+# RUN: ld.lld %t -o /dev/null 2>&1 | FileCheck %s
+
+# CHECK: invalid subsection length 0 at offset 1
+
+.section .ARM.attributes,"a",%0x70000003
+  .quad 0

lld/trunk/test/ELF/invalid/bad-arm-attributes2.s

+# REQUIRES: arm
+# RUN: llvm-mc -filetype=obj -triple=arm-unknown-linux %s -o %t
+# RUN: ld.lld %t -o /dev/null 2>&1 | FileCheck %s
+
+# CHECK: invalid subsection length 4294967295 at offset 1
+
+.section .ARM.attributes,"a",%0x70000003
+  .byte 0, 0xFF, 0xFF, 0xFF, 0xFF