From 63932bc3b0ea6e1152d48110010929673680165b Mon Sep 17 00:00:00 2001 From: YadeWira Date: Thu, 17 Sep 2026 20:28:20 -0500 Subject: [PATCH] -lc/-ld: a bare figure is MEGABYTES, not a power of two MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 #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, 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) --- docs/testing.md | 11 +++- rust/darc-arc/src/memlimit.rs | 104 ++++++++++++++++++++++++++++++++-- 2 files changed, 110 insertions(+), 5 deletions(-) diff --git a/docs/testing.md b/docs/testing.md index 949a783..57ca213 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -356,7 +356,16 @@ so the next unimplemented method gets listed rather than silently failing — an a case that fails for any *other* reason is still a failure. Every `-m` knob is implemented: `-mm`, `-mc`, `-md`, `-ms`, `-mt` and `-ma`. -`-lc-`/`-ld-` are still not accepted at all. +`-lc-`/`-ld-` are accepted and mean no limit, which this line denied until it +was measured; README's "What the archiver cannot do yet" had already been +corrected in #129/#130 and the two sections disagreed. + +The figure they take is `parseMemWithPercents`'s, not the `-m` grammar's: a +bare number is MEGABYTES (`Utils.hs:71` — `"24" означает 24mb`). Reading it +with `method::parse_mem` instead made `-lc24` mean 16 MB, refused `-lc512` +outright and let `-lc8` reach PPMd as 256 bytes, where the model panicked. +Gated in `memlimit.rs`'s `a_bare_figure_is_megabytes` and +`a_bare_figure_is_not_a_power_of_two`. `-ma` is the one worth reading the harness for. It selects between the TWO paths of `splitFileTypes`, and the port had implemented neither clause of diff --git a/rust/darc-arc/src/memlimit.rs b/rust/darc-arc/src/memlimit.rs index 5a52c7d..42946e5 100644 --- a/rust/darc-arc/src/memlimit.rs +++ b/rust/darc-arc/src/memlimit.rs @@ -806,17 +806,50 @@ fn physical_memory_raw() -> u64 { /// /// The default unit is megabytes, `b` means bytes, and `%` or `p` means a /// percentage of `memory`. Anything else is an error there, and `None` here. +/// +/// The unit table is `parseNumber`'s (`Utils.hs:55`), which APPENDS the default +/// specifier to the string before reading it, so a bare `"512"` is read as +/// `"512m"`. `Utils.hs:71` states it in one line: `"24" означает 24mb`. +/// +/// That is NOT `method::parse_mem`'s table, which this used to delegate to. +/// There a bare figure is a POWER OF TWO, because the `-m` grammar spells a +/// 4 MB dictionary `-md22`. The two tables agree on `b`/`k`/`m`/`g` and +/// disagree on exactly the spelling `-lc` documents — README's option table +/// reads `-lc N` as "N MB" — so routing the memory limits through the method +/// table produced three wrong answers at once: +/// +/// * `-lc24` meant 16 MB (`1 << 24`), not 24 MB, and said nothing about it. +/// * `-lc512` was refused outright with "not a memory size", because +/// `1 << 512` does not fit and the method table returns `None` at 32. +/// * `-lc8` became 256 BYTES, which no PPMd model can be built in: it left the +/// error path entirely and panicked in `ppmd/model.rs` on a slice range of +/// 258 into 256. +/// +/// `t` and `^` are here because `parseNumber` has them; `^` is the explicit +/// power-of-two spelling, which is how a caller asks for what the method table +/// gave by accident. The suffix is lower-cased because `parseNumber` reads +/// `strLower`. pub fn parse_mem_with_percents(memory: u64, s: &str) -> Option { let digits: String = s.chars().take_while(char::is_ascii_digit).collect(); if digits.is_empty() { return None; } let n: u64 = digits.parse().ok()?; - match s[digits.len()..].chars().next() { + match s[digits.len()..] + .chars() + .next() + .map(|c| c.to_ascii_lowercase()) + { Some('%') | Some('p') => Some(memory.saturating_mul(n) / 100), - // Everything else is a plain size, and parse_mem owns the unit table - // so the two spellings cannot drift apart. - _ => crate::method::parse_mem(s).map(u64::from), + Some('b') => Some(n), + Some('k') => Some(n.saturating_mul(KB)), + // The default specifier, and the explicit spelling of it. + None | Some('m') => Some(n.saturating_mul(MB)), + Some('g') => Some(n.saturating_mul(1024 * MB)), + Some('t') => Some(n.saturating_mul(1024 * 1024 * MB)), + // `2 ^ readI digits`, in bytes. 64 and up cannot be represented. + Some('^') => (n < 64).then(|| 1u64 << n), + Some(_) => None, } } @@ -1333,5 +1366,68 @@ mod lzma2_and_lc_tests { assert_eq!(get_dictionary(&r), 8 * MB as u32); } + /// `Utils.hs:71`, verbatim: `"24" означает 24mb`. The figure `-lc` and + /// `-ld` are documented in is megabytes (README's option table: "Limit + /// memory for compression to N MB"), and a bare number is that figure. + #[test] + fn a_bare_figure_is_megabytes() { + assert_eq!(parse_mem_with_percents(0, "24"), Some(24 * MB)); + assert_eq!(parse_mem_with_percents(0, "512"), Some(512 * MB)); + assert_eq!(parse_mem_with_percents(0, "8"), Some(8 * MB)); + } + + /// The method table would have answered `1 << n` to all three above, so + /// these are the values that used to come back, pinned to keep the two + /// tables from being merged again by someone reading only the call site. + #[test] + fn a_bare_figure_is_not_a_power_of_two() { + assert_ne!(parse_mem_with_percents(0, "24"), Some(1 << 24)); + assert_ne!(parse_mem_with_percents(0, "8"), Some(1 << 8)); + // `1 << 512` has no answer at all, which is how `-lc512` came to be + // refused as "not a memory size" while being exactly what the option + // documents. + assert!(parse_mem_with_percents(0, "512").is_some()); + } + + /// `parseNumber` (`Utils.hs:55`) — the whole table, including the `^` + /// spelling a caller needs now that a bare figure no longer gives it. + #[test] + fn the_unit_table_is_parse_numbers() { + assert_eq!(parse_mem_with_percents(0, "512b"), Some(512)); + assert_eq!(parse_mem_with_percents(0, "32k"), Some(32 * KB)); + assert_eq!(parse_mem_with_percents(0, "8m"), Some(8 * MB)); + assert_eq!(parse_mem_with_percents(0, "2g"), Some(2 * 1024 * MB)); + assert_eq!(parse_mem_with_percents(0, "1t"), Some(1024 * 1024 * MB)); + assert_eq!(parse_mem_with_percents(0, "8^"), Some(256)); + // `strLower` runs before the table is consulted. + assert_eq!(parse_mem_with_percents(0, "8M"), Some(8 * MB)); + assert_eq!(parse_mem_with_percents(0, "512B"), Some(512)); + } + + /// Only the first suffix character is read, as `span isDigit` leaves it. + #[test] + fn only_the_first_suffix_character_is_read() { + assert_eq!(parse_mem_with_percents(0, "16kb"), Some(16 * KB)); + assert_eq!(parse_mem_with_percents(0, "1mb"), Some(MB)); + } + + /// Percentages are of the figure passed in, and keep working. + #[test] + fn percentages_are_of_the_memory_given() { + assert_eq!(parse_mem_with_percents(1000, "75%"), Some(750)); + assert_eq!(parse_mem_with_percents(1000, "75p"), Some(750)); + } + + /// Anything the table does not name is an error there and `None` here, + /// which is what makes the CLI print "not a memory size" for it. + #[test] + fn an_unknown_suffix_is_still_rejected() { + assert_eq!(parse_mem_with_percents(0, "8x"), None); + assert_eq!(parse_mem_with_percents(0, "abc"), None); + assert_eq!(parse_mem_with_percents(0, ""), None); + // 64 and up do not fit in the shift. + assert_eq!(parse_mem_with_percents(0, "64^"), None); + } + }