On Thu, Sep 17, 2026 at 10:39:01AM +1000, Kugan Vivekanandarajah wrote:
> +
> +/* Sniff a profile's format from its leading bytes (shared by afdo_dump /
> + afdo_merge to pick a reader). */
> +profile_format
> +detect_format (const std::string &path)
> +{
> + /* Only the leading v3 magic word is needed; do not map the whole file. */
> + uint32_t magic = 0;
> + FILE *f = fopen (path.c_str (), "rb");
> + if (f == nullptr)
> + return profile_format::unknown;
> + const bool have_magic
> + = fread (&magic, 1, sizeof (magic), f) == sizeof (magic);
> + fclose (f);
> + if (have_magic && magic == v3::format::AFDO_GCOV_DATA_MAGIC)
> + return profile_format::v3_binary;
> + return profile_format::unknown;
> +}
This makes the format endian dependent. I guess it's ok, but it should
probably be documented somewhere, or ideally give a suitable message.
It would be nice to detect at least v1 and v2 and give a better error
than just unknown format.
> +#include <cstdio>
> +#include <memory>
> +
> +namespace gcov {
> +
> +/* On-disk size of one summary histogram row: cutoff, min_count,
> + num_counts. */
> +constexpr size_t DETAILED_ENTRY_BYTES
> + = sizeof (uint32_t) + 2 * sizeof (uint64_t);
> +
> +/* Hard cap on nested inline callsites. Remaining-byte checks alone still
> + allow a deep spine of tiny records to overflow the C stack. */
s/spine/nest/
spine sounds like Claudese.
> +constexpr unsigned MAX_INLINE_DEPTH = 64;
But in any case the stack limit can be always increased, while this
constant is harder. I guess you could make it configurable, or
alternatively detect stack overflow with a sigaltstack handler.
> +/* Read a record length word and reject a record that cannot fit in the
> input.
> + The value is only a lower bound on the record: producers disagree on both
> + its unit and its coverage, as create_gcov counts 32-bit words and leaves
> + the file-name table out of the FILE_NAMES length and the v3 timestamps out
> + of the FUNCTION length. Comparing it against the bytes left accepts
> either
> + unit while still rejecting a record that claims more than the file holds;
> + parsing itself stays bounded by END. */
> +static bool
> +read_record_length (const uint8_t *&cur, const uint8_t *end)
> +{
> + uint32_t record_length = 0;
> + if (!gcov_read_uint32 (cur, end, record_length))
> + return false;
> + return cur <= end && record_length <= (size_t) (end - cur);
FWIW I suspect a lot of this would be easier if you just exited
on a bad input file with a fatal error. I assume there is no good other
recovery strategy anyways. All these custom error handling paths are
nearly impossible to test and you might reget them if someone ever gets around
to fuzz this.
Also this would allow a specific error message in the right place for
each case, which makes it much easier to figure out what went wrong.
> +}
> +
> +/* Read a record tag and check it against EXPECTED. NAME identifies the
> + record in the diagnostic emitted on a mismatch. */
> +static bool
> +read_expected_tag (const uint8_t *&cur, const uint8_t *end, uint32_t
> expected,
> + const char *name)
> +{
> + uint32_t tag;
> + if (!gcov_read_uint32 (cur, end, tag))
> + return false;
> + if (tag != expected)
> + {
> + fprintf (stderr,
> + _("Error: Expected %s tag (0x%x), got 0x%x\n"),
> + name,
> + (unsigned) expected,
> + (unsigned) tag);
> + return false;
> + }
> + return true;
> +}
At least for the top level tags it would be much nicer if we only
warned for unknown ones and skipped them if the length is available. This would
allow later extension with new records without breaking compatibility
with old releases.
> + return false;
> +
> + uint32_t num_files = 0;
> + if (!gcov_read_uint32 (cur_, end_, num_files))
> + return false;
> + /* Length word plus trailing NUL. */
> + if (cur_ > end_
> + || (num_files != 0
> + && num_files > (size_t) (end_ - cur_) / (sizeof (uint32_t) + 1)))
> + return false;
> + file_names.reserve (num_files);
> + for (uint32_t i = 0; i < num_files; i++)
> + {
> + file_names.emplace_back ();
> + if (!gcov_read_string (cur_, end_, file_names.back ()))
> + return false;
Bad indent? In some other places too.
> +
> + std::unique_ptr<gcov_location> location;
> + if (num_targets == 0)
> + location = std::make_unique<gcov_location_normal> (count);
> + else
> + {
> + auto call = std::make_unique<gcov_location_call> (count);
> + /* value_profile_type, function_id, target_count. */
> + if (cur_ > end_
> + || (num_targets != 0
> + && num_targets
> + > (size_t) (end_ - cur_)
> + / (sizeof (uint32_t) + 2 * sizeof (uint64_t))))
Can you put this into some helper macro or inline? All the open coded
boundary checks are difficult and don't even fit on lines.
> + {
> + fprintf (stderr, _("Error: v3 function name index out of range\n"));
> + return false;
> + }
> + func.desc.name_index = name_idx;
> + func.id = name_idx;
> + const uint32_t file_idx = file_name_map[name_idx];
> + func.desc.file_index
> + = (file_idx < file_names.size ()) ? file_idx : INVALID_INDEX;
> + if (!gcov_read_uint32 (cur_, end_, num_pos)
> + || !gcov_read_uint32 (cur_, end_, num_inline_calls))
> + return false;
> + const size_t n_pos = num_pos;
> + const size_t n_cs = num_inline_calls;
> + if (n_pos > ~(size_t) 0 - n_cs)
> + return false;
I'm not sure the test really works, e.g. what happens if n_cs is 0?
> + {
> + std::unique_ptr<gcov_location> location = nullptr;
Does a NULL unique_ptr make sense?
> +/* Map the profile and read its header and the four AFDO records. */
> +bool
> +gcov_reader_v3::read ()
> +{
> + afdo_tools::file_mmap fb;
It would be nice if this supported streaming input, I could imagine
doing pipelines of the compiler and the autofdo tools.
You don't really seem to use the benefits of the direct memory map
much anyways with the complicated parsing functions.
> +
> + if (version != v3::format::AFDO_GCOV_VERSION)
> + {
> + fprintf (stderr, _("Error: unsupported v3 version %u\n"),
> + (unsigned) version);
> + return false;
> + }
> +
> +namespace gcov {
> +
> +/* Open PATH for writing. A failure is reported here and left for write ()
> to
> + turn into an error, so construction never throws. */
We don't throw anyways.
> +/* Patch the record length word at LENGTH_POS with the size of the payload
> + written after it, counted in 32-bit words as v3 readers expect. */
> +bool
> +finish_record_length (FILE *file, long length_pos)
> +{
> + const long end = ftell (file);
> + if (length_pos < 0 || end < 0)
> + return false;
> + const long body = length_pos + (long) sizeof (uint32_t);
> + if (end < body)
> + return false;
> + /* Strings are not padded, so round the payload up to a whole word. */
> + const unsigned long words
> + = ((unsigned long) (end - body) + sizeof (uint32_t) - 1)
> + / sizeof (uint32_t);
> + if (words > 0xfffffffful)
> + return false;
> + if (fseek (file, length_pos, SEEK_SET) != 0)
> + return false;
> + gcov_write_uint32 (file, (uint32_t) words);
> + return fseek (file, end, SEEK_SET) == 0;
Again streaming would be nice eventually. Not sure how much it would
complicate here.
-Andi
