aaaaaa123456789 / libplum

Image-handling library that allows reading and writing common image file formats
The Unlicense
11 stars 6 forks source link

Use Awk instead of Bash for merging input files #17

Open ISSOtm opened 2 weeks ago

ISSOtm commented 2 weeks ago

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!

Rangi42 commented 2 weeks ago

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
ISSOtm commented 2 weeks ago

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

Rangi42 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 1 week ago

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