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

Use Awk instead of Bash for merging input files #17

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ISSOtm
Copy link
Contributor

@ISSOtm ISSOtm commented Jul 7, 2024

This program is POSIX-conformant, so this should fix [EDIT: nope] #16—cc @Rangi42 :)

This was checked to yield precisely the same files as the current merge.sh, down to the byte!

This program is POSIX-conformant
Copy link
Contributor

@Rangi42 Rangi42 left a comment

Choose a reason for hiding this comment

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

Fails on macOS 11 Big Sur:

% git status
On branch awk
Your branch is up to date with 'issotm/awk'.

nothing to commit, working tree clean
% /usr/bin/awk --version
awk version 20200816
% make
mkdir -p build
./merge.awk header/libplum.h > build/libplum.h
/usr/bin/awk: extra ] at source line 27 in function include source file ./merge.awk
 context is
					if >>>  (!sub(/\/[^/] <<< *$/, "/", include_dir)) {
/usr/bin/awk: nonterminated character class \/[^
 source line number 27 source file ./merge.awk
make: *** [build/libplum.h] Error 2

@Rangi42
Copy link
Contributor

Rangi42 commented Jul 7, 2024

Escaping the [^/] to be [^\/] gets further, but still fails:

% make
mkdir -p build
./merge.awk src/allocator.c src/bmpread.c src/bmpwrite.c src/checksum.c src/color.c src/framebounds.c src/framebuffer.c src/frameduration.c src/gifcompress.c src/gifread.c src/gifwrite.c src/huffman.c src/jpegarithmetic.c src/jpegcomponents.c src/jpegcompress.c src/jpegdct.c src/jpegdecompress.c src/jpeghierarchical.c src/jpeghuffman.c src/jpegread.c src/jpegreadframe.c src/jpegtables.c src/jpegwrite.c src/load.c src/metadata.c src/misc.c src/newstruct.c src/palette.c src/pngcompress.c src/pngdecompress.c src/pngread.c src/pngreadframe.c src/pngwrite.c src/pnmread.c src/pnmwrite.c src/sort.c src/store.c > build/libplum.c
mkdir -p build
./merge.awk header/libplum.h > build/libplum.h
cc -shared -fPIC -std=c17 -Ofast -fomit-frame-pointer -fno-asynchronous-unwind-tables -fno-exceptions -Wl,-S -Wl,-x -Wl,--gc-sections -march=native -mtune=native build/libplum.c -o build/libplum.so
build/libplum.c:1:10: fatal error: 'proto.h' file not found
#include "proto.h"
         ^~~~~~~~~
1 error generated.
make: *** [all] Error 1

Somehow GAwk doesn't mind that, but others do
@ISSOtm
Copy link
Contributor Author

ISSOtm commented Jul 7, 2024

I pushed a fix for the escape, but it does compile fine on my machine. Does it work for you now?

@Rangi42
Copy link
Contributor

Rangi42 commented Jul 8, 2024

Pushing to issotm/awk isn't working, but here's what needs changing:

  1. Use POSIX [[:space:]] not GNU \s.
  2. Remove -Wl,--gc-sections from CFLAGS in the Makefile (Apple clang 13, at least, does not support it).
    • For that matter, most other CFLAGS look extraneous too: C doesn't have exceptions or unwind tables; omitting the frame pointer is a bad idea; and if the user wants their symbols stripped (-Wl,-S -Wl,-x) they'll do it themselves, but they should be available in the library. (Same goes for the dead code elimination that --gc-sections would do; that's the end user's business.)

@ISSOtm
Copy link
Contributor Author

ISSOtm commented Jul 8, 2024

I fixed the first bullet, but I expect @aaaaaa123456789 will want to have some input on the latter. (For what it's worth, too, that is a fix largely orthogonal to this PR, so maybe it can be tackled separately.)

@aaaaaa123456789
Copy link
Owner

Please don't add extraneous changes (like compiler flags) to PRs. As for omitting the frame pointer, if you want readable stack traces, use a debug build — that's what it's for! Same for all the other optimizations; I'm not going to deliver low-quality .so files just because someone could clean it up.
(On the other hand, how come a specific flavour of clang doesn't support a flag?)

I'll test the awk changes later. Thanks!

@Rangi42
Copy link
Contributor

Rangi42 commented Jul 10, 2024

If -Wl,--gc-sections stays, then #16 should remain open, because it will not yet have fixed the "doesn't build on macOS" issue.

My understanding is that .so files with symbols are not generally considered "low-quality" -- the opposite, if anything. If the .so file has symbols, they will be stripped when users' own libplum-using project strips symbols from their own .exe (likewise for LTO); but if there are no symbols, then users who need them won't have them.

Anyway, that's a separate issue, but it occurred to me because those are a lot of flags which don't actually matter for building or using the project, and which only increase the chances of some C compiler not supporting them all, as it turns out Apple clang doesn't.

@ISSOtm
Copy link
Contributor Author

ISSOtm commented Jul 10, 2024

Section GC

Since -Wl is for passing flags to the linker, it seems more likely that the breakage is in the underlying linker.

I thought Apple's toolchains used LLD by default, but ld.lld does support --gc-sections.

What is the exact compile error that you get from it, @Rangi42?

I do think it's a good idea to provide "good defaults", as long as they can be overridden. Though that's outside of this PR's scope, arguably.

Stripping

Since stripping can be done externally (using strip), I think it's preferable to provide the library with symbols, so that the packager has the option to keep them within reasonable convenience.

@Rangi42
Copy link
Contributor

Rangi42 commented Jul 11, 2024

Here's the exact output (minus repetitive warnings), with the merge.awk fix but without the Makefile fix:

% cc --version
Apple clang version 13.0.0 (clang-1300.0.29.30)
Target: x86_64-apple-darwin20.6.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin
% man ld
ld(1)                     BSD General Commands Manual                    ld(1)

NAME
     ld -- linker

SYNOPSIS
     ld files...  [options] [-o outputfile]

[...]

SEE ALSO
     as(1), ar(1), cc(1), nm(1), otool(1) lipo(1), arch(3), dyld(3), Mach-O(5), strip(1), rebase(1)

Darwin                        September 10, 2020                        Darwin
% make
mkdir -p build
./merge.awk src/allocator.c src/bmpread.c src/bmpwrite.c src/checksum.c src/color.c src/framebounds.c src/framebuffer.c src/frameduration.c src/gifcompress.c src/gifread.c src/gifwrite.c src/huffman.c src/jpegarithmetic.c src/jpegcomponents.c src/jpegcompress.c src/jpegdct.c src/jpegdecompress.c src/jpeghierarchical.c src/jpeghuffman.c src/jpegread.c src/jpegreadframe.c src/jpegtables.c src/jpegwrite.c src/load.c src/metadata.c src/misc.c src/newstruct.c src/palette.c src/pngcompress.c src/pngdecompress.c src/pngread.c src/pngreadframe.c src/pngwrite.c src/pnmread.c src/pnmwrite.c src/sort.c src/store.c > build/libplum.c
mkdir -p build
./merge.awk header/libplum.h > build/libplum.h
cc -shared -fPIC -std=c17 -Ofast -fomit-frame-pointer -fno-asynchronous-unwind-tables -fno-exceptions -Wl,-S -Wl,-x -Wl,--gc-sections -march=native -mtune=native build/libplum.c -o build/libplum.so
[build/libplum.c:2617:9: warning: add explicit braces to avoid dangling else [-Wdangling-else]
      } else
        ^
[...]
build/libplum.c:2925:16: warning: using the result of an assignment as a condition without parentheses [-Wparentheses]
  while (block = context -> data[p ++]) {
         ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
[...]
build/libplum.c:5734:34: warning: result of comparison of constant 1152921504606846975 with expression of type 'uint32_t' (aka 'unsigned int') is always false [-Wtautological-constant-out-of-range-compare]
  if (context -> image -> frames > SIZE_MAX / sizeof(struct plum_rectangle)) throw(context, PLUM_ERR_IMAGE_TOO_LARGE);
      ~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[...]
24 warnings generated.
ld: unknown option: --gc-sections
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [all] Error 1

@ISSOtm
Copy link
Contributor Author

ISSOtm commented Aug 8, 2024

Since this is at least a step in the right direction, could it be merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants