Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

archutil: refactor generation and validation #4607

Merged
merged 7 commits into from
Feb 9, 2024

Conversation

crazy-max
Copy link
Member

Moves archutil generation and validation to main bake definition and makefile. This will trigger validate-archutil on ci as well through bake validate targets group:

echo "matrix=$(docker buildx bake validate --print | jq -cr '.target | keys')" >> $GITHUB_OUTPUT

// Do not edit manually.

const Binaryamd64 = "\x1f\x8b\x08\x00\x00\x00\x00\x00\x02\xff\xec\x98\x3f\x8b\x13\x41\x18\xc6\x9f\xd9\x6c\xee\x0f\x08\xb9\xe0\x81\xc2\x59\xa8\x58\xd8\xb8\xe1\x54\x38\x05\x95\x55\x50\x07\xf1\xec\xce\xd2\x65\x93\x5d\xce\x80\x97\x5b\xb2\xb3\xe7\x0a\x07\x77\x47\xbe\x83\x58\x07\x14\x0b\x4b\xc1\x94\xa9\x5c\x2c\x6d\xac\x4d\x21\x04\xad\xec\x0c\x28\xca\x6e\xde\x49\xb2\x63\xa2\x82\xdd\x31\x3f\x08\xcf\xbc\xcf\xbe\x33\xef\x90\xa4\xd9\x67\xef\xe6\xdd\x5b\x06\x63\x90\x18\xb8\x86\xac\x5a\xb2\xb3\xda\x26\xff\x63\x79\xd4\x02\x1b\x97\x50\x80\x8d\x39\x14\xb3\x5e\x13\x93\xd8\x39\x3d\x42\x47\x4b\xc5\xd2\x50\xd2\xb2\x38\x51\xcb\x79\x52\x03\xb2\xa5\xca\x3e\x93\x3e\x7d\xb2\xfb\x34\x47\xea\x19\xf2\xa5\x9a\x13\xba\x0c\xa0\x00\xe0\xf6\xbd\x0d\xf8\xcf\xdf\x5c\xfe\xf0\xf2\xd3\xfd\xf5\x1f\x4f\x8f\x7e\x7f\xf1\x79\xef\xed\x85\xf7\xaf\xa1\xd1\x68\x34\x1a\x8d\x46\xa3\xd1\x68\x34\x1a\xcd\x21\x85\xaf\x76\x4b\x6d\xde\xfa\x36\xbf\x7b\x87\x77\x00\xec\xa7\x66\xa9\x7d\x95\x01\xfb\xbb\x6b\x3c\xe9\xb2\xac\xe9\x5d\xa9\xcd\x0f\x7a\xec\xf4\x33\xf0\x83\x41\x2a\xd1\x0a\xef\x30\x6a\x1f\x3e\x6e\xf5\x18\x6f\x0d\x58\x64\x7c\x59\xe4\x49\x72\x1d\x40\xb6\xb8\x91\x2e\xc0\x93\xee\x95\xf4\xe0\xe2\x9f\xee\x52\x00\x1b\xbd\xc7\xe7\x7d\x63\x9c\x0f\x60\x9c\x1f\x98\xf8\xfa\x53\xed\x2d\x53\x8a\xb1\xa1\xf4\x2f\x93\xff\x40\xf1\x8f\x91\xbf\xa9\xf8\xa7\xb2\xc8\xe1\xf7\xb9\x27\xa4\x7f\x32\xef\x9f\x9d\xe1\x57\x66\xf8\xa8\x88\xad\xa0\x52\xab\xc5\xb5\xb5\xc7\xcd\x75\x6b\x1b\x3b\xab\xd8\x39\x0f\x3f\xae\x0b\x38\x4e\x35\x0c\x9d\x50\xb8\x4d\x01\xc7\xf7\x5c\xe1\xc2\xf1\x1b\x1e\x60\x85\x4f\xb6\x84\x5b\x85\x15\x8a\xe6\x50\x1f\xca\x55\x63\x5b\xf8\xd6\x66\x23\xb2\xaa\x51\xfd\x91\x77\xae\xee\xc1\x12\x7e\x2c\xfe\xfb\xff\xb1\x02\x60\x3e\xfb\x86\xd4\xbc\x25\x9f\xb3\x40\xc9\x5b\x24\x16\xfd\x56\x73\xa3\x1c\xc7\xce\xe5\x39\x81\xd2\xcf\xa6\xd4\xc6\x94\x7b\x05\xb4\x7f\x81\x8d\xe7\xa6\xf7\x5c\xa0\xe7\xc7\x49\x17\x29\xf3\x51\x89\x29\xcf\xba\xf8\x97\xf9\xe5\x19\xfb\x5f\xfd\xe3\xfe\x5f\x01\x00\x00\xff\xff\xa5\x58\xb1\x16\x60\x13\x00\x00"
const Binaryamd64 = "\x1f\x8b\x08\x00\x00\x00\x00\x00\x02\xff\xec\x98\x3f\x8b\x13\x41\x18\xc6\x9f\xd9\x6c\xee\x0f\x08\xb9\xe0\x81\xc2\x59\xa8\x58\xd8\xb8\xe1\x4e\x41\x05\x95\x55\x38\x1d\x45\x6d\xe4\x2c\x5d\x36\xd9\xe5\x5c\xf0\x72\xcb\xed\xec\x11\xe1\xe0\x2e\xe4\x3b\x88\x75\x40\xb1\xb0\x14\x4c\x99\xca\xc5\xd2\xc6\xda\x14\x42\xd0\xca\xce\x80\xa2\xec\xe6\x9d\x24\x3b\x26\x2a\xd8\xc9\xfc\x20\x3c\xf3\x3e\xfb\xce\xbc\x43\x92\x66\x9f\xfd\xf5\xdb\xd7\x0d\xc6\x20\x31\x70\x05\x59\xb5\x64\x67\xb5\x4d\xfe\x87\xf2\xa8\x05\x36\x2e\xa0\x00\x1b\x73\x28\x66\xbd\x26\x26\xb1\x73\x7a\x88\x8e\x96\x8a\xa5\xa1\xa4\x65\x71\xa2\x96\xf3\xa4\x86\x64\x4b\x95\x7d\x26\x7d\xfa\x64\xf7\x69\x8e\xd4\x53\xe4\x4b\x35\x27\x74\x19\x40\x01\xc0\x8d\xbb\x1b\xf0\x9f\xbd\xbe\xf8\xfe\xc5\xc7\xfb\x77\xbe\x3f\x39\xfc\xed\xf9\xa7\xfd\x37\x67\xdf\xbd\x82\x46\xa3\xd1\x68\x34\x1a\x8d\x46\xa3\xd1\x68\x34\xff\x29\x7c\xb5\x5b\x6a\xf3\xd6\xd7\xf9\xbd\x5b\xbc\x03\xe0\x20\x35\x4b\xed\xcb\x0c\x38\xd8\x3b\xcf\x93\x2e\xcb\x9a\xde\x96\xda\xbc\xd9\x63\x27\x9f\x82\x37\x07\xa9\xc4\x2b\xbc\xc3\xa8\x7d\xf8\xb8\xd5\x63\xbc\x35\x60\xb1\xf1\x79\x91\x27\xc9\x55\x00\xd9\xe2\x5a\xba\x00\x4f\xba\x97\xd2\x83\x8b\xbf\xbb\x4b\x01\x6c\xf4\x1e\x9f\xf7\x8d\x71\x3e\x80\x71\x7e\x60\xe2\xcb\x0f\xb5\xb7\x4c\x29\xc6\x86\xd2\xbf\x4c\xfe\x03\xc5\x3f\x42\xfe\xa6\xe2\x9f\xc8\x22\x87\x5f\xe7\x1e\x93\xfe\xf1\xbc\x7f\x7a\x86\x5f\x99\xe1\xa3\x22\xb6\xc2\x4a\xad\xb6\xb6\x1e\xd4\xee\xdd\xb4\xb6\xb1\xbb\x8a\xdd\x35\xf8\x8d\x40\xc0\x71\xaa\x51\xe4\x44\xc2\xdd\x11\x70\x7c\xcf\x15\x2e\x1c\xbf\xee\x01\x56\xf4\x78\x4b\xb8\x55\x58\x91\xd8\x19\xea\x43\xb9\xaa\x6f\x0b\xdf\xda\xac\xc7\x56\x35\x0e\x1e\x79\x67\x02\x0f\x96\xf0\x1b\xe2\x9f\xff\x1f\x2b\x00\xe6\xb3\x6f\x48\xcd\x5b\xf2\x39\x0b\x94\xbc\x45\x62\xd1\x6f\x35\x37\xca\x71\xec\x5c\x9e\x13\x2a\xfd\x6c\x4a\x6d\x4c\xb9\x57\x48\xfb\x17\xd8\x78\x6e\x7a\xcf\x05\x7a\x7e\x94\x74\x91\x32\x1f\x95\x06\xe5\x59\xe7\xfe\x30\xbf\x3c\x63\xff\xcb\xbf\xdc\xff\x33\x00\x00\xff\xff\xad\x50\x19\xd5\x60\x13\x00\x00"
Copy link
Member Author

@crazy-max crazy-max Feb 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This diff looks odd when generating bins locally: https://github.com/moby/buildkit/actions/runs/7740278596/job/21104904747#step:4:682

Might be linked to current microarchitecture level on my pc

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok when reverting this change I still got a diff on ci: https://github.com/moby/buildkit/actions/runs/7740278596/job/21104904747#step:4:682

Something is flaky, I will add an exception in the meantime.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added an exception in validation: aa14573

@crazy-max

This comment was marked as resolved.

@crazy-max crazy-max marked this pull request as ready for review February 1, 2024 11:28
FROM base AS exit-amd64
COPY fixtures/exit.amd64.S .
COPY util/archutil/fixtures/exit.amd64.S .
RUN x86_64-linux-gnu-gcc -static -nostdlib -o exit exit.amd64.S
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it missing --noexecstack that is causing the validation issue?

Seems to contain a temp filename.

» diff d1.dis d2.dis                                                                                                                                                !4185
2c2
< f1.bin:	file format elf64-x86-64
---
> f2.bin:	file format elf64-x86-64
25c25
<   400107: 7f c6                        	jg	0x4000cf </tmp/cc2EicSI.o+0x4000cf>
---
>   400107: 7f c6                        	jg	0x4000cf </tmp/ccxc7wrM.o+0x4000cf>
146,148c146,151
<        6: 63 63 32                     	movslq	50(%rbx), %esp
<        9: 45 69 63 53 49 2e 6f 00      	imull	$7286345, 83(%r11), %r12d # imm = 0x6F2E49
<       11: 76 31                        	jbe	0x44 <.symtab+0x44>
---
>        6: 63 63 78                     	movslq	120(%rbx), %esp
>        9: 63 37                        	movslq	(%rdi), %esi
>        b: 77 72                        	ja	0x7f <.symtab+0x7f>
>        d: 4d 2e                        	cs
>        f: 6f                           	outsl	(%rsi), %dx
>       10: 00 76 31                     	addb	%dh, 49(%rsi)


» diff f1.xxd f2.xxd                                                                                                                                                !4186
141c141
< 00001180: 3245 6963 5349 2e6f 0076 3100 7632 0065 7869 7400 5f5f 6273 735f 7374 6172 7400  2EicSI.o.v1.v2.exit.__bss_start.
---
> 00001180: 7863 3777 724d 2e6f 0076 3100 7632 0065 7869 7400 5f5f 6273 735f 7374 6172 7400  xc7wrM.o.v1.v2.exit.__bss_start.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it missing --noexecstack that is causing the validation issue?

Doesn't look like it: https://github.com/moby/buildkit/actions/runs/7752978105/job/21143440422#step:4:724

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't see how the compiler has interpreted and potentially modified the original assembly instructions with temp files like jg 0x4000cf </tmp/cc2EicSI.o+0x4000cf>, even with debug output -S -v and -fverbose-asm

And striping debug info and ident directives doesn't seem to solve this. I think optimization is already disabled with gcc by default so should not be this.

Copy link
Member

@tonistiigi tonistiigi Feb 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the the difference now in binary? There is still /tmp/_.o in the binary, even with no execstack and strip?

@crazy-max crazy-max force-pushed the ci-archutil branch 4 times, most recently from 231d91a to 4a12942 Compare February 2, 2024 08:39
@crazy-max crazy-max force-pushed the ci-archutil branch 3 times, most recently from 47a7a0d to cae021a Compare February 9, 2024 10:14
@crazy-max
Copy link
Member Author

crazy-max commented Feb 9, 2024

No relevant diff between local and ci:

local objdump


amd64:     file format elf64-x86-64


Disassembly of section .text:

0000000000401000 <_start>:
  401000:       48 31 c0                xor    %rax,%rax
  401003:       0f a2                   cpuid
  401005:       48 83 f8 07             cmp    $0x7,%rax
  401009:       7c 4a                   jl     401055 <v1>
  40100b:       48 b8 00 00 00 80 00    movabs $0x80000000,%rax
  401012:       00 00 00
  401015:       0f a2                   cpuid
  401017:       3d 01 00 00 80          cmp    $0x80000001,%eax
  40101c:       7c 37                   jl     401055 <v1>
  40101e:       48 c7 c0 01 00 00 00    mov    $0x1,%rax
  401025:       48 31 c9                xor    %rcx,%rcx
  401028:       0f a2                   cpuid
  40102a:       48 81 e1 01 22 98 00    and    $0x982201,%rcx
  401031:       48 81 f9 01 22 98 00    cmp    $0x982201,%rcx
  401038:       75 1b                   jne    401055 <v1>
  40103a:       48 b8 01 00 00 80 00    movabs $0x80000001,%rax
  401041:       00 00 00
  401044:       48 31 c9                xor    %rcx,%rcx
  401047:       0f a2                   cpuid
  401049:       48 83 e1 01             and    $0x1,%rcx
  40104d:       48 83 f9 01             cmp    $0x1,%rcx
  401051:       75 02                   jne    401055 <v1>
  401053:       eb 09                   jmp    40105e <v2>

0000000000401055 <v1>:
  401055:       48 c7 c7 41 00 00 00    mov    $0x41,%rdi
  40105c:       eb 09                   jmp    401067 <exit>

000000000040105e <v2>:
  40105e:       48 c7 c7 42 00 00 00    mov    $0x42,%rdi
  401065:       eb 00                   jmp    401067 <exit>

0000000000401067 <exit>:
  401067:       48 c7 c0 3c 00 00 00    mov    $0x3c,%rax
  40106e:       0f 05                   syscall

ci objdump


archutil-exits/amd64:     file format elf64-x86-64


Disassembly of section .text:

0000000000401000 <_start>:
  401000:       48 31 c0                xor    %rax,%rax
  401003:       0f a2                   cpuid
  401005:       48 83 f8 07             cmp    $0x7,%rax
  401009:       7c 4a                   jl     401055 <v1>
  40100b:       48 b8 00 00 00 80 00    movabs $0x80000000,%rax
  401012:       00 00 00
  401015:       0f a2                   cpuid
  401017:       3d 01 00 00 80          cmp    $0x80000001,%eax
  40101c:       7c 37                   jl     401055 <v1>
  40101e:       48 c7 c0 01 00 00 00    mov    $0x1,%rax
  401025:       48 31 c9                xor    %rcx,%rcx
  401028:       0f a2                   cpuid
  40102a:       48 81 e1 01 22 98 00    and    $0x982201,%rcx
  401031:       48 81 f9 01 22 98 00    cmp    $0x982201,%rcx
  401038:       75 1b                   jne    401055 <v1>
  40103a:       48 b8 01 00 00 80 00    movabs $0x80000001,%rax
  401041:       00 00 00
  401044:       48 31 c9                xor    %rcx,%rcx
  401047:       0f a2                   cpuid
  401049:       48 83 e1 01             and    $0x1,%rcx
  40104d:       48 83 f9 01             cmp    $0x1,%rcx
  401051:       75 02                   jne    401055 <v1>
  401053:       eb 09                   jmp    40105e <v2>

0000000000401055 <v1>:
  401055:       48 c7 c7 41 00 00 00    mov    $0x41,%rdi
  40105c:       eb 09                   jmp    401067 <exit>

000000000040105e <v2>:
  40105e:       48 c7 c7 42 00 00 00    mov    $0x42,%rdi
  401065:       eb 00                   jmp    401067 <exit>

0000000000401067 <exit>:
  401067:       48 c7 c0 3c 00 00 00    mov    $0x3c,%rax
  40106e:       0f 05                   syscall

Diff

$ diff local.dis ci.di
2c2
< amd64:     file format elf64-x86-64
---
> archutil-exits/amd64:     file format elf64-x86-64

With an hex dump:

$ diff <(xxd amd64) <(xxd ci/amd64)
277,278c277,278
< 00001140: 0000 0000 0000 0000 0063 6344 6d32 6230  .........ccDm2b0
< 00001150: 4d2e 6f00 7631 0076 3200 6578 6974 005f  M.o.v1.v2.exit._
---
> 00001140: 0000 0000 0000 0000 0063 6365 7444 4452  .........ccetDDR
> 00001150: 4a2e 6f00 7631 0076 3200 6578 6974 005f  J.o.v1.v2.exit._

objdump with all headers

$ diff <(objdump -x amd64) <(objdump -x ci/amd64)
2,3c2,3
< amd64:     file format elf64-x86-64
< amd64
---
> ci/amd64:     file format elf64-x86-64
> ci/amd64
25c25
< 0000000000000000 l    df *ABS*        0000000000000000 ccDm2b0M.o
---
> 0000000000000000 l    df *ABS*        0000000000000000 ccetDDRJ.o

@crazy-max crazy-max force-pushed the ci-archutil branch 3 times, most recently from e85224d to 13de276 Compare February 9, 2024 15:03
@crazy-max crazy-max marked this pull request as draft February 9, 2024 15:03
@crazy-max
Copy link
Member Author

crazy-max commented Feb 9, 2024

Was comparing builds on ci ubuntu 20.04 vs ubuntu 22.04: https://github.com/moby/buildkit/actions/runs/7845795579?pr=4607

diff <(objdump -d ci-ubuntu-2004/amd64) <(objdump -d ci-ubuntu-2204/amd64)
2c2
< ci-ubuntu-2004/amd64:     file format elf64-x86-64
---
> ci-ubuntu-2204/amd64:     file format elf64-x86-64
diff <(objdump -x ci-ubuntu-2004/amd64) <(objdump -x ci-ubuntu-2204/amd64)
2,3c2,3
< ci-ubuntu-2004/amd64:     file format elf64-x86-64
< ci-ubuntu-2004/amd64
---
> ci-ubuntu-2204/amd64:     file format elf64-x86-64
> ci-ubuntu-2204/amd64
25c25
< 0000000000000000 l    df *ABS*        0000000000000000 cce259MI.o
---
> 0000000000000000 l    df *ABS*        0000000000000000 ccopjZOn.o
diff <(xxd ci-ubuntu-2004/amd64) <(xxd ci-ubuntu-2204/amd64)
277,278c277,278
< 00001140: 0000 0000 0000 0000 0063 6365 3235 394d  .........cce259M
< 00001150: 492e 6f00 7631 0076 3200 6578 6974 005f  I.o.v1.v2.exit._
---
> 00001140: 0000 0000 0000 0000 0063 636f 706a 5a4f  .........ccopjZO
> 00001150: 6e2e 6f00 7631 0076 3200 6578 6974 005f  n.o.v1.v2.exit._

@crazy-max crazy-max force-pushed the ci-archutil branch 7 times, most recently from fc60834 to b49628b Compare February 9, 2024 16:06
@crazy-max
Copy link
Member Author

Ok using strip seems to work, not sure why I can't strip this header directly with gcc. Also tried with clang and same issue.

@crazy-max crazy-max marked this pull request as ready for review February 9, 2024 16:08
Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand why the binary size only changed for amd64 when adding strip.

@crazy-max crazy-max merged commit 0131359 into moby:master Feb 9, 2024
65 checks passed
@crazy-max crazy-max deleted the ci-archutil branch February 9, 2024 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants