Repository navigation
Conversation
`parseMemWithPercents` (`Utils.hs:78`) reads its argument with `parseNumber
memstr 'm'`, and `parseNumber` APPENDS the default specifier before reading,
so `"512"` is read as `"512m"`. `Utils.hs:71` says it outright: `"24"
означает 24mb`. README's option table agrees -- `-lc N` is "Limit memory for
compression to N MB".
The port delegated every non-percentage spelling to `method::parse_mem`,
whose default is the `-m` grammar's: no suffix is a POWER OF TWO, because
that is how `-md22` spells a 4 MB dictionary. The two tables agree on
b/k/m/g and disagree on exactly the spelling the option documents, so the one
delegation produced three wrong answers at once:
-lc24 meant 16 MB (`1 << 24`), not 24 MB, and said nothing about it.
-lc512 was refused with "not a memory size": `1 << 512` has no answer and
the method table returns None from 32 up, which is most of the
range a user would type.
-lc8 became 256 BYTES, which no PPMd model can be built in. It left the
error path entirely and panicked:
ppmd/model.rs:219:27: range end index 258 out of range for
slice of length 256
`parse_mem_with_percents` now owns `parseNumber`'s table: b/k/m/g/t, `^` for
the power of two a caller can no longer reach by accident, `%`/`p` unchanged,
and the suffix lower-cased because `parseNumber` reads `strLower`. It has
three call sites and all three are the -lc/-ld block, so nothing that writes
an archive today can change.
Measured after: `-lc8` and `-lc8m` write the same archive, `-lc24` and
`-lc24m` the same, `-lc512` is accepted, `-lc8x` is still a clean exit 2.
`Tests/run-tests.sh` 24 passed / 0 failed and `arc-golden-check.sh` 118 cases
/ 0 failing are unchanged.
`docs/testing.md` claimed `-lc-`/`-ld-` "are still not accepted at all". That
stopped being true in DavidLee18#129/DavidLee18#130 -- README's "What the archiver cannot do yet"
had been corrected and the two sections disagreed. Measured: both are
accepted and mean no limit.
NOT fixed here, and worth its own decision: `-lc512b` still panics in the
same PPMd place. 512 bytes is a legitimate spelling of a limit, and
`C_PPMD.cpp:130` clamps nothing either (`if (_mem==0) return;`), so the
reference corrupts its heap where this port panics. What a limit below a
codec's minimum should do -- report it the way README promises for LZMA, or
clamp -- is a deliberate divergence, and that call is yours.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi — I see you did a complete rework. I'm still interested in helping you, and
sorry for being inactive for almost a year.
I came back, read the port, and started with something small and checkable.
What this fixes
-lc/-ldwere parsed with the-mgrammar's unit table instead of thememory one. Three wrong answers came out of that one delegation:
-lc241 << 24), silently-lc512ERROR: -lc512: not a memory size, exit 2-lc8ppmd/model.rs:219, exit 101parseMemWithPercents(Utils.hs:78) reads its argument withparseNumber memstr 'm', andparseNumberappends the default specifier before reading, so"512"is"512m".Utils.hs:71states it in one line —"24" означает 24mb— and README's option table agrees:
-lc Nis "Limit memory for compression toN MB".
method::parse_memhas no default specifier, because-md22has tomean a 4 MB dictionary. The two tables agree on b/k/m/g and disagree on exactly
the spelling
-lcdocuments.parse_mem_with_percentsnow ownsparseNumber's table:b/k/m/g/t,^for the power of two a caller can no longer reach by accident,%/punchanged, and the suffix lower-cased because
parseNumberreadsstrLower.It has three call sites and all three are the
-lc/-ldblock, so nothingthat writes an archive today can change. Measured after:
-lc8and-lc8mwrite the same archive,
-lc24and-lc24mthe same,-lc512is accepted,-lc8xis still a clean exit 2.Tests/run-tests.sh24 passed / 0 failed andarc-golden-check.sh118 cases / 0 failing, both unchanged.docs/testing.mdalso said-lc-/-ld-"are still not accepted at all". Thatstopped being true in #129/#130 — README's "What the archiver cannot do yet"
had been corrected and the two sections disagreed. Measured: both are accepted
and mean no limit.
Not fixed here, because it is your call
-lc512bstill panics in the same PPMd place. 512 bytes is a legitimatespelling of a limit, and
C_PPMD.cpp:130clamps nothing either (if (_mem==0) return;), so the reference corrupts its heap where this port panics. What alimit below a codec's minimum should do — report it the way README already
promises for LZMA's
-ld1m, or clamp to the minimum — is a deliberatedivergence, and AGENTS.md says those are marked, not made silently. Happy to
implement whichever you pick.
What I would like to work on next: 32-bit, in this repo rather than beside it
I maintain DArc86, the 32-bit Windows
port, and it is a Haskell/C++ tree that stopped at
c9e207d. Rather than dragit forward as a parallel codebase, I would rather bring 32-bit here as another
target, if you will have it. What I measured this week, from this tree,
unmodified:
i686-pc-windows-gnubuilds clean —darc.exe6.6 MB,unarc.exe4.1 MB,PE32 i386. Round-trips, and archives are byte-identical to the x86_64
build's on the same input (same SHA-256).
i686-unknown-linux-gnubuilds clean and its archives are byte-identical too.i686-win7-windows-gnutarget: the tier-1 binaryimports
WaitOnAddress/WakeByAddress*(Win8),ProcessPrng(Win10 1809)and
GetSystemTimePreciseAsFileTime, so its loader never reachesmain(exit 5, no output, on a real 6.1.7601). Built with
-Z build-std, thoseimports drop to
CreateWaitableTimerExW(Vista+) andSetThreadStackGuarantee(XP SP2+), and the binary runs on real Windows 7 SP1:
a,t,xandunarc xallAll OK, including a 28 MB multi-codec archive written by theLinux x86_64 build, and an archive written on Win7 reads back on Linux. Built
on the pinned 1.97.1, not on nightly — see below.
ccdoes not know the tier-3 triple and fallsback to the host compiler, so
libzstd.acomes out as an ELF object andthe link dies in ~200
undefined reference to ZSTD_*.CC_i686_win7_windows_gnu,AR_i686_win7_windows_gnuand the matchingCARGO_TARGET_*_LINKERfix it.Two honest costs, and I would rather state the second one plainly than sell you
the target.
The Win7 target is tier 3, so its
stdis never distributed —win7appearszero times in both the stable 1.98.1 and the nightly channel manifests, and
rustup target addrefuses it. It has to be built from source with-Z build-std, and that is a property of the tier, not of the Rust version:updating the pin does not help.
That does NOT mean nightly, though, which is what I assumed at first and had to
correct.
RUSTC_BOOTSTRAP=1makes a stable toolchain accept-Z, and thewhole thing then builds with 1.97.1, the revision
rust-toolchain.tomlalready pins — verified, and the binary it produces is the one I ran on
Windows 7 above. That keeps the property the pin exists for, in your own words:
"a release built against a different compiler revision than CI validates would
be a silent difference between what is tested and what is shipped". A pinned
nightly breaks exactly that;
RUSTC_BOOTSTRAPdoes not.What it does cost is honesty about an unstable flag: it is the variable rustc
uses to bootstrap itself, upstream Rust discourages it for production builds,
and it ties the job to
-Z build-std's behaviour in that exact revision. Yourcall which is the lesser evil — I lean to the pinned stable, since the pin is
the thing your own comment says must not move.
And the 32-bit build is capped at roughly 300 MB per file — not by the
address space, but by #180:
darc areads each file whole and builds thearchive in memory, so peak RSS runs 6–7x the input. 300 MB archives fine and
400 MB dies with
memory allocation of 800001606 bytes failed. I first wrotethis off as "32-bit is 32-bit"; measuring the x86_64 build is what corrected me,
since it wants 29.8 GB for a 5 GB file and the factor is the same on both. So a
32-bit target shipped today would be honest for ordinary files and would
disappoint anyone who reaches for it because their machine is old and their
files are big — which is most of that audience. The 32-bit port my fork carries
handles those files, because FreeArc streamed through the
--cachepool.Given that, what I would suggest: take #180 first, and let the 32-bit targets
follow it rather than lead. If you want them sooner anyway, the Windows 10 i686
one is cheap and independent (tier 1, no nightly, one matrix row, verified on a
real kernel the way
windows-amd64-verifyalready does), and the Win7 one cancome separately. And if you would rather keep Win7 out of the tree entirely,
say so and I will keep it in my fork — I would just rather contribute here than
maintain a second archiver.