Allow assembling skein_block_asm.s with clang
ClosedPublic
Actions

Authored by arichardson on Jun 5 2020, 9:58 AM.

Details

Reviewers

cem
emaste
jmg

Commits

rS361833: Allow assembling skein_block_asm.s with clang

Summary

GNU as seems to allow macro arguments without the '\' but clang is more
strict in that regard.
This change makes the source code compatible with LLVM's but does not yet
change the build system or rename it to .S

Test Plan

Pasted the new source into godbold.org, successfully assembles with clang and GCC (after changing -D flags to -Wa,-defsym).

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

arichardson requested review of this revision.Jun 5 2020, 9:58 AM

arichardson created this revision.

Harbormaster completed remote builds in B31503: Diff 72709.Jun 5 2020, 9:58 AM

https://godbolt.org/z/W7h0Ij

val_packett.cool added a subscriber: val_packett.cool.Jun 5 2020, 12:23 PM

godbolt shows one inconsequential difference between clang & gcc:

--- clang.txt   2020-06-05 08:29:34.207545000 -0400
+++ gcc.txt     2020-06-05 08:29:58.341321000 -0400
@@ -1866,8 +1866,8 @@
  mov    QWORD PTR [rbp+0xc0],rcx
  mov    r9,QWORD PTR [rdi+0x18]
  jmp    1bc0 <Skein1024_block_loop>
- nop    WORD PTR cs:[rax+rax*1+0x0]
- nop    DWORD PTR [rax+0x0]
+ data16 nop WORD PTR cs:[rax+rax*1+0x0]
+ nop    DWORD PTR [rax]
 Skein1024_block_loop:
  mov    r8,QWORD PTR [rdi+0x10]
  add    r8,QWORD PTR [rbp+0xc0]

We should do the same comparison between Clang IAS and the current in-tree as 2.17.50.

I have the build infrastructure changes staged in a WIP tree somewhere.

GNU as 2.17.50 assembles this identically to the unpatched version; IMO you should commit this now, and you or I can follow up with the build infrastructure change.

sys/crypto/skein/amd64/skein_block_asm.s
243 ↗	(On Diff #72709)	I tried assembling both versions with GNU as 2.17.50 and comparing the result with diffoscope. The only difference I see is the absence of `_RCNT_` in the patched version.
771 ↗	(On Diff #72709)	the non-`\` change on this line is just extra white space

This revision is now accepted and ready to land.Jun 5 2020, 1:20 PM

arichardson added inline comments.Jun 5 2020, 1:42 PM

sys/crypto/skein/amd64/skein_block_asm.s
243 ↗	(On Diff #72709)	This change may not be necessary.
771 ↗	(On Diff #72709)	Ah my editor is configured to strip trailing whitespace in modified lines.

emaste added inline comments.Jun 5 2020, 1:46 PM

sys/crypto/skein/amd64/skein_block_asm.s
243 ↗	(On Diff #72709)	I'm happy enough with or without this change, but I think it's better with your change.
771 ↗	(On Diff #72709)	Indeed, that's fine. We could commit a whitespace cleanup first, stripping all trailing whitespace.

In D25143#554011, @emaste wrote:

GNU as 2.17.50 assembles this identically to the unpatched version; IMO you should commit this now, and you or I can follow up with the build infrastructure change.

I If you could do the build system changes that would be great.

sys/crypto/skein/amd64/skein_block_asm.s
243 ↗	(On Diff #72709)	The problem was the `&` isntead of using `\()` to join macro arguments: This: .macro RotL64 reg,BLK_SIZE,ROUND_NUM,MIX_NUM _RCNT_ = RC_\BLK_SIZE\()_\ROUND_NUM\()_\MIX_NUM .if _RCNT_ #is there anything to do? rolq $_RCNT_,%\reg .endif .endm is also accepted by clang. But I guess the symbol is not necessary so I'll go with the current version.