Opened 15 months ago

Last modified 8 months ago

#12636 new bug

ProfHeap's printf modifiers are incorrect

Reported by: Phyx- Owned by:
Priority: normal Milestone: 8.4.1
Component: Runtime System Version: 8.0.1
Keywords: newcomer Cc: simonmar
Operating System: Windows Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D3129
Wiki Page:

Description

during compile I noticed

rts\ProfHeap.c: In function 'dumpCensus':

rts\ProfHeap.c:768:26: error:
     warning: format '%lu' expects argument of type 'long unsigned int', but argument 3 has type 'long long unsigned int' [-Wformat=]
             fprintf(hp_file, "VOID\t%lu\n",
                              ^

rts\ProfHeap.c:770:26: error:
     warning: format '%lu' expects argument of type 'long unsigned int', but argument 3 has type 'long long unsigned int' [-Wformat=]
             fprintf(hp_file, "LAG\t%lu\n",
                              ^

rts\ProfHeap.c:772:26: error:
     warning: format '%lu' expects argument of type 'long unsigned int', but argument 3 has type 'long long unsigned int' [-Wformat=]
             fprintf(hp_file, "USE\t%lu\n",
                              ^

rts\ProfHeap.c:774:26: error:
     warning: format '%lu' expects argument of type 'long unsigned int', but argument 3 has type 'long long unsigned int' [-Wformat=]
             fprintf(hp_file, "INHERENT_USE\t%lu\n",
                              ^

rts\ProfHeap.c:776:26: error:
     warning: format '%lu' expects argument of type 'long unsigned int', but argument 3 has type 'long long unsigned int' [-Wformat=]
             fprintf(hp_file, "DRAG\t%lu\n",
                              ^

Change History (12)

comment:1 Changed 10 months ago by bollu

I would like to take this up, do I simply assign it to myself? thanks

comment:2 Changed 10 months ago by Phyx-

Yup, go ahead. This task probably just involves replacing these hardcoded format specifiers with the correct ones from https://github.com/ghc/ghc/blob/master/includes/stg/Types.h.

comment:3 Changed 10 months ago by bollu

Owner: set to bollu

comment:4 Changed 10 months ago by bollu

Why does the Census type ssize_t for void_total, drag_total, etc? wouldn't size_t be more appropriate, as they don't seem to be using the _signed_ property of ssize_t?

The piece of code that causes the warning is

        fprintf(hp_file, "LAG\t%lu\n",
                (unsigned long)(census->not_used - census->void_total) * sizeof(W_));

The sizeof(W_) :: size_t and census->not_used :: ssize_t.

The solution that I'm proposing is to cast the ssize_t to a size_t and then change the format specifier to %zu as discussed on StackOverflow:

http://stackoverflow.com/questions/2125845/platform-independent-size-t-format-specifiers-in-c

comment:5 Changed 10 months ago by Phyx-

I actually question the use of ssize_t at all for these values. I think the calculations violate the expected value ranges of ssize_t namely with

censuses[t].void_total   += size;
censuses[era].void_total -= size;

So I think the types in _counter are wrong and have the potential to do an unsigned underflow as ssize_t is only guaranteed to be able to store values between [-1, {SSIZE_MAX}][1]

[1]http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_types.h.html

comment:6 Changed 10 months ago by Phyx-

Differential Rev(s): Phab:D3129
Status: newpatch

comment:7 Changed 10 months ago by Ben Gamari <ben@…>

In 60c4986/ghc:

Typecast covers entire expression to fix format warning.

- Fixes (#12636).
- changes all the typecasts to _unsinged long long_ to
  have the format specifiers work.

Reviewers: austin, bgamari, erikd, simonmar, Phyx

Reviewed By: erikd, Phyx

Subscribers: thomie

Differential Revision: https://phabricator.haskell.org/D3129

comment:8 Changed 10 months ago by bgamari

Milestone: 8.2.1
Owner: bollu deleted
Status: patchnew

Phyx may have a point in comment:5 so I'm going to leave this open.

comment:9 Changed 10 months ago by bollu

I'm not sure what the expected solution is for the underflow bug. If Phyx / bgmari have ideas, I'd be glad to implement it

comment:10 Changed 10 months ago by Phyx-

It may be that it isn't a problem. In case it is, we should add an assert before censuses[era].void_total -= size;.

assert (censuses[era].void_total >= size); at least this way, if it is a problem we'll trigger it. if it's not it also makes it clear to the next person that we've thought about it and it's not an issue.

if it turns out to be a problem, then we should replace ssize_t with a normal signed type like int64_t or something.

Last edited 10 months ago by Phyx- (previous) (diff)

comment:11 Changed 10 months ago by simonmar

These counts can legitimately be negative, so we should use real signed types here. Sorry I didn't notice this earlier.

comment:12 Changed 8 months ago by bgamari

Milestone: 8.2.18.4.1

Given that 8.2.1-rc1 is imminent, I'm bumping these off to the 8.4

Note: See TracTickets for help on using tickets.