[Bug driver/105171] New: `local_tick` can overflow and become -1

[Bug driver/105171] New: `local_tick` can overflow and become -1

[jason at zx2c4 dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105171

            Bug ID: 105171
           Summary: `local_tick` can overflow and become -1
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: driver
          Assignee: unassigned at gcc dot gnu.org
          Reporter: jason at zx2c4 dot com
  Target Milestone: ---

In gcc/toplev.c, there's the comment:

    /* A local time stamp derived from the time of compilation. It will be
       zero if the system cannot provide a time.  It will be -1u, if the
       user has specified a particular random seed.  */
    unsigned local_tick;

This is affirmed by init_local_tick()'s comment:

    /* Initialize local_tick with the time of day, or -1 if
       flag_random_seed is set.  */
    static void init_local_tick (void)

And indeed we see it assigned -1 when flag_random_seed != NULL:

    else
      local_tick = -1;

So far so good. However, in the case where flag_random_seed == NULL, local_tick
is assigned like this:

    struct timeval tv;
    gettimeofday (&tv, NULL);
    local_tick = (unsigned) tv.tv_sec * 1000 + tv.tv_usec / 1000;

Recall that local_tick is of type "unsigned". Somewhat often, that expression
calculates to be 0xffffffff, which means local_tick winds up being -1 even when
flag_random_seed == NULL.

This is a problem for plugins that follow the documentation comments in order
to determine whether -frandom-seed is being used. To work around this bug,
these plugins should just call get_random_seed(noinit=true) in their plugin
init functions and check there whether the return value is zero. If they use a
local_tick==-1 check, once in a blue moon, it'll be wrong.

Probably, though, the type of local_tick should be changed to a HOST_WIDE_INT,
or something similar to that.

[Bug driver/105171] gcc/toplev.c: `local_tick` can overflow and become -1

[jason at zx2c4 dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105171

Jason A. Donenfeld <jason at zx2c4 dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|plugins                     |driver

--- Comment #1 from Jason A. Donenfeld <jason at zx2c4 dot com> ---
Hi Andrew - I see you just changed the component to "plugins". That's partially
right, but partially wrong too, because this also affects components of gcc
itself. In particular, from gcc/coverage.c:

----------------------------------------------------------------------
void
coverage_finish (void)
{
  if (bbg_file_name && gcov_close ())
    unlink (bbg_file_name);

  if (!flag_branch_probabilities && flag_test_coverage
      && (!local_tick || local_tick == (unsigned)-1))
    /* Only remove the da file, if we're emitting coverage code and
       cannot uniquely stamp it.  If we can stamp it, libgcov will DTRT.  */
    unlink (da_file_name);
----------------------------------------------------------------------

Here we have two chances for local_tick to wrap around to 0 or to -1, when
perhaps that wasn't the intention.

So I'm going to set the component field back to driver, where the bug lives, as
this clearly affects multiple things (coverage, plugins, maybe other stuff I
don't know).

[Bug plugins/105171] gcc/toplev.c: `local_tick` can overflow and become -1

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105171

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |WAITING
          Component|driver                      |plugins
   Last reconfirmed|                            |2022-04-05

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
>    local_tick = (unsigned) tv.tv_sec * 1000 + tv.tv_usec / 1000;

The only place where local_tick is used is inside coverage.cc:
  if (!flag_branch_probabilities && flag_test_coverage
      && (!local_tick || local_tick == (unsigned)-1))
    /* Only remove the da file, if we're emitting coverage code and
       cannot uniquely stamp it.  If we can stamp it, libgcov will DTRT.  */
    unlink (da_file_name);


This does not make a difference really here.

Plus inside a plugin, I don't see why you care about if -frandom-seed was
supplied or not.

[Bug plugins/105171] gcc/toplev.c: `local_tick` can overflow and become -1

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105171

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
>toplev.c
You mean toplev.cc right?

[Bug plugins/105171] gcc/toplev.c: `local_tick` can overflow and become -1

[jason at zx2c4 dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105171

--- Comment #4 from Jason A. Donenfeld <jason at zx2c4 dot com> ---
(In reply to Andrew Pinski from comment #2)
> >    local_tick = (unsigned) tv.tv_sec * 1000 + tv.tv_usec / 1000;
> 
> The only place where local_tick is used is inside coverage.cc:
>   if (!flag_branch_probabilities && flag_test_coverage
>       && (!local_tick || local_tick == (unsigned)-1))
>     /* Only remove the da file, if we're emitting coverage code and
>        cannot uniquely stamp it.  If we can stamp it, libgcov will DTRT.  */
>     unlink (da_file_name);
> 
> 
> This does not make a difference really here.

Really? Looks to me like the behavior changes. If it doesn't make a difference,
then remove that part of the condition clause? If you object to doing that,
then the overflow seems like an issue.

> Plus inside a plugin, I don't see why you care about if -frandom-seed was
> supplied or not.

So that I can decide whether I am allowed to do things totally randomly or if I
must follow some sort of seeded determinism.

The broader answer is of course, "because plugins can do many things."

> You mean toplev.cc right?

I guess I'm reading gcc 11 source code, but yea, sure, same difference.

[Bug plugins/105171] gcc/toplev.c: `local_tick` can overflow and become -1

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105171

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|WAITING                     |RESOLVED
         Resolution|---                         |WONTFIX

--- Comment #5 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
> So that I can decide whether I am allowed to do things totally randomly
> or if I must follow some sort of seeded determinism.

That is almost always the wrong approach really.
Just assume the seed it always set and then use the seed as given. Don't ever
do something non-determinstic if you can avoid it.  GCC has changed away from
that years ago. 

The randomness in GCC is only needed to get unique symbol names across two
translation units which might have the same file name. The seed is there so you
can set it to get the same symbol across builds if you know if the file names
are unique in the first place.


The coverage.cc is a non-issue as libgcov will do the right thing as mentioned.

[Bug plugins/105171] gcc/toplev.c: `local_tick` can overflow and become -1

[pageexec at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105171

--- Comment #6 from PaX Team <pageexec at gmail dot com> ---
(In reply to Andrew Pinski from comment #5)
> > So that I can decide whether I am allowed to do things totally randomly
> > or if I must follow some sort of seeded determinism.
> 
> That is almost always the wrong approach really.
> Just assume the seed it always set and then use the seed as given. Don't
> ever do something non-determinstic if you can avoid it.  GCC has changed
> away from that years ago. 

'almost always' is the keyword here as there are situations where
1. we want to introduce compile time randomness into the compilation,
2. use gcc's PRNG or something else.

case in point, our structure layout randomization gcc plugin that is now even
in upstream linux. the problem is the decision in requirement #2 as so called
'reproducible builds' won't work too well without knowing whether the user
wanted to control gcc's PRNG via -frandom-seed. this decision should be made
based upon 'local_tick' but due to its undersized nature (should be u64) it can
lead to false decisions. that's the bug we're talking about, other uses may or
may not suffer but at this point are irreleant really.

> The coverage.cc is a non-issue as libgcov will do the right thing as
> mentioned.

you didn't mention the 'why'. the conditional expression can falsely execute
the unlink even when the file's uniquely stamped but the seed just happened to
be 0 or -1 due to the bug. if those seed values don't matter then the comment
and the conditional are wrong.

[Bug plugins/105171] gcc/toplev.c: `local_tick` can overflow and become -1

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105171

--- Comment #7 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
I think there might be a way to check if the option -frandom-seed was passed
and that might be a better option for your usage anyways. You should check the
options handling code.

[Bug plugins/105171] gcc/toplev.c: `local_tick` can overflow and become -1

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105171

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
flag_random_seed is static inside of toplev.cc, but I guess the plugin can walk
  bool deterministic = false;
  for (unsigned i = 1; i < save_decoded_options_count; ++i)
    if (save_decoded_options[i].opt_index == OPT_frandom_seed
        && !save_decoded_options[i].value)
      deterministic = false;
    else if (save_decoded_options[i].opt_index == OPT_frandom_seed_)
      deterministic = true;

[Bug plugins/105171] gcc/toplev.c: `local_tick` can overflow and become -1

[pageexec at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105171

--- Comment #9 from PaX Team <pageexec at gmail dot com> ---
(In reply to Andrew Pinski from comment #7)
> I think there might be a way to check if the option -frandom-seed was passed
> and that might be a better option for your usage anyways. You should check
> the options handling code.
i had checked it before but the trace that -frandom-seed/set_random_seed()
leave behind is a static variable (flag_random_seed), local_tick would be an
indirect indicator were it not for this bug. the only other way i can see is
the rather big hammer of having to reparse common_deferred_options or
save_decoded_options as mentioned by Jakub (both are gcc 4.6+ only)...

[Bug plugins/105171] gcc/toplev.c: `local_tick` can overflow and become -1

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105171

--- Comment #10 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
What is interesting here is any change will always going to be gcc 13+. So
requiring gcc 4.6+ is not a bad thing.
Gcc 12 does not even compile with gcc 4.7 or before now with the requirement of
c++11. Gcc 4.8 was released over 9 years ago. Gcc 4.6 was 11 years ago. So
supporting a 11 year old compiler for plugin is not realistic at this point
really.

[Bug plugins/105171] gcc/toplev.c: `local_tick` can overflow and become -1

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105171

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|jakub at redhat dot com            |

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I think kernel requires gcc >= 5.1 anyway.

[Bug plugins/105171] gcc/toplev.c: `local_tick` can overflow and become -1

[pageexec at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105171

--- Comment #12 from PaX Team <pageexec at gmail dot com> ---
(In reply to Jakub Jelinek from comment #11)
> I think kernel requires gcc >= 5.1 anyway.
there's more than one kernel version in active use and under
development/support, even by upstream, not to mention companies/communities.
for example linux 4.14 builds with gcc 3.2+, linux 5.4 builds with gcc 4.6+,
linux 5.10 builds with gcc 4.9+, you get the idea. obviously plugins compiling
for older gcc will have to use some hack to handle the bug here but going
forward it'd be nice to do something better.

[Bug plugins/105171] gcc/toplev.c: `local_tick` can overflow and become -1

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105171

--- Comment #13 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
As has been said, we can't retroactively change released compilers from the
last 2 decades.  And going forward, I think walking the save_decoded_options is
just fine and reliable, other plugins use that too.

[Bug plugins/105171] gcc/toplev.c: `local_tick` can overflow and become -1

[pageexec at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105171

--- Comment #14 from PaX Team <pageexec at gmail dot com> ---
(In reply to Jakub Jelinek from comment #13)
> As has been said, we can't retroactively change released compilers from the
> last 2 decades.  And going forward, I think walking the save_decoded_options
> is just fine and reliable, other plugins use that too.
nobody talked about fixing old compilers, it's about fixing this bug on trunk.
seriously, this is a one-liner u32->u64 change (or the simplification in
coverage_finish), is the bikeshed on irrelevant tangents in over a dozen
comments worth it vs. just fixing it?

[Bug plugins/105171] gcc/toplev.c: `local_tick` can overflow and become -1

[jason at zx2c4 dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105171

Jason A. Donenfeld <jason at zx2c4 dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|WONTFIX                     |---
             Status|RESOLVED                    |WAITING

--- Comment #15 from Jason A. Donenfeld <jason at zx2c4 dot com> ---
I submitted a patch for this:
https://gcc.gnu.org/pipermail/gcc-patches/2022-April/592897.html

I'm also reopening this bug.

Feel free to take that patch and do whatever you want with it. I get the idea
you don't generally like outside contributions and I've probably submitted that
patch with all the wrong conventions, so I won't mind if you take that and
mangle it in a totally different direction without further feedback. Mostly I
just want to point out how easy this is to fix (at least I think?).

[Bug plugins/105171] gcc/toplev.c: `local_tick` can overflow and become -1

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105171

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |patch
     Ever confirmed|1                           |0
                URL|                            |https://gcc.gnu.org/piperma
                   |                            |il/gcc-patches/2022-April/5
                   |                            |92897.html
          Component|driver                      |plugins
             Status|WAITING                     |UNCONFIRMED

--- Comment #16 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
WAITING is not the right thing.

[Bug plugins/105171] gcc/toplev.c: `local_tick` can overflow and become -1

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105171

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |WONTFIX
             Status|UNCONFIRMED                 |RESOLVED

--- Comment #17 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
The stamp value is used to synchronize
   note and data files and to synchronize merging within a data
   file. It need not be an absolute time stamp, merely a ticker that
   increments fast enough and cycles slow enough to distinguish
   different compile/run/compile cycles.

So unlinking the da file is ok as it will be merged (remove data) correctly
anyways.

I think you are reading the comment in coverage.cc incorrectly.

    /* Only remove the da file, if we're emitting coverage code and
       cannot uniquely stamp it.  If we can stamp it, libgcov will DTRT.  */

Basically the unlink is there just in case we can't uniquely stamp the da file
and do another compile that was already there. That is it will be emptied out
when the program is run with a different timestamp; removing the file while
compiling is just in case so you don't get mix matched results.

Again there is no need to fix this.

[Bug plugins/105171] gcc/toplev.c: `local_tick` can overflow and become -1

[jason at zx2c4 dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105171

--- Comment #18 from Jason A. Donenfeld <jason at zx2c4 dot com> ---
If it truly doesn't matter whether local_tick is a valid value or an overflowed
one (to 0 or to -1 in this case), then just remove `&& (!local_tick ||
local_tick == (unsigned)-1)`. If you object to removing that line, then surely
you intend for local_tick to play some role in this. In that case, the fact
that one condition can overflow to become another condition sounds like the
logic is occasionally suboptimal.

In other words, it strikes me that your choices are:

1) remove a conditional clause that has no meaning;
2) fix the overflow in the value of a variable checked by that conditional
clause;
3) neglect to change anything, and have that conditional clause sometimes have
meaning and sometimes not.

Maybe having buggy code -- going with choice (3) -- doesn't matter overly much,
because other things in your system still make okay overall decisions. But that
doesn't change the fact that the condition is buggy when it overflows.

Anyway, I defer to comment 14, and note that we're now on comment 18, over a
bug with a trivial fix and an available patch. I think that's an indication
that whatever is happening here is truly out of my hands, so I'll duck out now.

[Bug plugins/105171] gcc/toplev.c: `local_tick` can overflow and become -1

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105171

--- Comment #19 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
It is a small optimization to remove the file is the timestamp is valid while
compiling.

[Bug plugins/105171] gcc/toplev.c: `local_tick` can overflow and become -1

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105171

--- Comment #20 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #19)
> It is a small optimization to remove the file is the timestamp is valid
> while compiling.

uh, what coverage.cc is doing is "odd" at best.  local_tick shouldn't have been
exported in the first place.

[Bug plugins/105171] gcc/toplev.c: `local_tick` can overflow and become -1

[jason at zx2c4 dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105171

--- Comment #21 from Jason A. Donenfeld <jason at zx2c4 dot com> ---
FYI, Linux is working around this shortcoming with the trick in this commit:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c40160f2998c897231f8454bf797558d30a20375

[Bug plugins/105171] gcc/toplev.c: `local_tick` can overflow and become -1

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105171

--- Comment #22 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
That doesn't look right, get_random_seed (true) can return 0 even if it uses a
random seed which just happens to be 0.

[Bug plugins/105171] gcc/toplev.c: `local_tick` can overflow and become -1

[jason at zx2c4 dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105171

--- Comment #23 from Jason A. Donenfeld <jason at zx2c4 dot com> ---
The output of crc32_string() is always >0 for that case (I think?). And the
case where the user passes an explicit "0" to -frandom-seed was already
considered to be "disabled" by the prior code. A 0 seed also plunges that
plugin's lfsr into the trivial linear subspace, rendering it inoperable. So all
things considered, 0 is a reasonable "off" value for us to use. There might be
some room for improvement -- starting with this bug report here -- but things
seem fine enough as-is (plus, the bikeshedding in this bug report already
doesn't make that road too appealing).