Hi Andi,
Thanks for the review.
> On 18 Sep 2026, at 9:50 am, Andi Kleen <[email protected]> wrote:
>
> External email: Use caution opening links or attachments
>
>
> In general I'm not sure it's a good idea to rely too much on mmap
> for the perf.data, but more about that later.
>
>> +/* Host tools abort on failure; when linked into cc1, use gcc_assert. */
>> +#ifdef IN_GCC
>> +#define afdo_assert(EXPR) gcc_assert (EXPR)
>> +#else
>> +#include <cstdio>
>> +#include <cstdlib>
>> +#define afdo_assert(EXPR) \
>> + ((void) (!(EXPR) \
>> + ? (std::fprintf (stderr, \
>> + "%s:%d: afdo_assert failed: %s\n", \
>> + __FILE__, __LINE__, #EXPR), \
>> + std::abort (), 0) \
>> + : 0))
>
> Could be just standard assert() ?
We also want to share the code for GCOV reading with GCC. We are now doing this
for GCOV V4 but also can do for V3.
This is the only reason we have this wrapper.
>
>> + if (static_cast<uintmax_t> (st.st_size)
>> + > static_cast<uintmax_t> (std::numeric_limits<size_t>::max ()))
>> + {
>> + ::close (fd);
>> + return false;
>> + }
>
>
> I'm not sure how that could happen? Isn't size_t already 8 bytes
> on a 64bit host?
>
We are doing this so that we could also build it on 32 bit systems. We could
build only for 64 bit systems only.
In that case we can remove this and some others like this later.
>> + const size_t len = static_cast<size_t> (st.st_size);
>> + void *m = ::mmap (nullptr, len, PROT_READ, MAP_PRIVATE, fd, 0);
>
> Do you really need MAP_PRIVATE to write to it? Otherwise MAP_SHARED.
I will switch to MAP_SHARED
>
>> +
>> +/* Mix VALUE into SEED (the Boost hash_combine mixing step). */
>> +template <typename T>
>> +void
>> +hash_combine (size_t &seed, T value) noexcept
>> +{
>> + seed ^= static_cast<size_t> (value)
>> + + static_cast<size_t> (0x9e3779b97f4a7c15ULL) + (seed << 6)
>> + + (seed >> 2);
>> +}
>
> I would have assumed we have a suitable hash somewhere else, maybe
> in libiberty.
Yes, we can use that. I have tried it and it is working.
>
>> +void
>> +start ()
>> +{
>> + s_start = clock_type::now ();
>> + s_started = true;
>> + s_stages.clear ();
>> +}
>
> Hmm are the tools so complicated that a standard profiler is not good
> enough?
I will remove this. I used this while developing to get more fine grained stage
times. I will remove it altogether and simply.
Thanks,
Kugan
>
> -Andi
