Open Bug 536427 Opened 13 years ago Updated 2 years ago

Flag functions that should be static if they are only called within a particular compilation unit

Categories

(Firefox Build System :: Source Code Analysis, defect, P3)

defect

Tracking

(Not tracked)

People

(Reporter: taras.mozilla, Unassigned)

References

Details

Attachments

(2 files)

Marking functions as static should let the compiler be smarter about inclining and will make GCC produce a warning if a function ever goes unused.
Assignee: nobody → ehren.m
Status: NEW → ASSIGNED
Depends on: 537857
Attached file analysis results
This was generated against revision 89d33cf3490c. The modifications for callgraph to generate the extra columns in 'node' are straightforward but I can post them. Total functions identified: 3987
This list looks pretty good. I don't think there is much we can do about the C code.

Looks like we should static-ify content/layout/accessibility. Should probably to whip a basic pork tool to do these en-mass(module granularity). C

A bunch of these are public APIs, if we end up maintaining this list we should deal with that. 

You should definitely integrate your changes into callgraph. I think once we take care of the obvious/easy/to/fix stuff we should flag this info in dxr.

Aside: roc mentioned that these issues are most likely to be fixed by contributors working on patching the affected area and these should somehow be flagged during the patch review cycle.
Yeah, this is the kind of thing it would be really good to have during review.
cairo decorates all of it's public symbols with 'cairo_public' so it should be possible to avoid the false positives there.
Attached patch callgraph patchSplinter Review
Patch for callgraph to add 'isStatic' and 'isMethod' to 'node'. Conceivably, we could do without 'isMethod' since separate sqlite views for functions and methods could be created by parsing the 'name' column.
Attachment #420090 - Flags: review?
Attachment #420090 - Flags: review? → review?(dwitte)
Depends on: 538014
Comment on attachment 420090 [details] [diff] [review]
callgraph patch

Looks good!
Attachment #420090 - Flags: review?(dwitte) → review+
It seems there is a patch for this. Is it current, and ready to go?
(In reply to Worcester12345 from comment #7)
> It seems there is a patch for this. Is it current, and ready to go?

What is latest status on this bug? Is it working ("FIXED" "WFM")? Is it not going to be fixed ("WONTFIX")?

Thanks.
Flags: needinfo?(michael)
I don't believe that we have code for this in tree right now. I imagine that this patch is stale enough (It's been 8 years!) that it can't land as-is.

I'm inclined to say that this is going to not be fixed anytime soon, but I'll mark it P3 anyway. It would probably need to be done with the sixgill analysis because we don't have full callgraphs in the clang static analysis.
Flags: needinfo?(michael)
Priority: -- → P3
Ehren, can you look at this patch, and bring it up to date?

Thanks.
Flags: needinfo?(ehren.m)
Hi Worchester. This patch is against 'callgraph' which is a dehydra (treehydra) script. Dehydra is tightly coupled to GCC 4.5 (which I don't believe will compile current firefox) and an ancient SpiderMonkey version so going forward with this patch isn't the best approach.

A custom clang plugin or perhaps modifications to dxr might be the better bet. Although, the 'callgraph patch' might guide you in modifications to sixgill, assuming that's still a GCC plugin.

Note that the attachment 'analysis results' contains an SQL query against the callgraph sqlite db to find functions which have not been marked static but which have no callers outside their compilation unit. The actual patch, 'callgraph patch' is just a modification to callgraph to record which functions (and methods) have been marked static (in order to support the query in 'analysis results').
Flags: needinfo?(ehren.m)
Should this bug just be marked "INVALID", then?
Product: Core → Firefox Build System

Maybe we could do that with the searchfox db...

Assignee: ehren.m → nobody
Status: ASSIGNED → NEW

Searchfox uses a clang plugin to get its knowledge about C++ code. Instead of trying to shoehorn it into searchfox it would be better to just write a new clang plugin.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #14)

Searchfox uses a clang plugin to get its knowledge about C++ code. Instead of trying to shoehorn it into searchfox it would be better to just write a new clang plugin.

What Sylvestre meant is to use the data produced by the searchfox Clang Plugin, not putting it into searchfox. I think a new Clang plugin wouldn't save a lot, you would still have to generate all the data, then postprocess it (because you have to scan for other invocations of the function, which you can't do directly from the Clang plugin).

I don't know if the searchfox data has all the information available for this, but I can take a look at it next week maybe.

clang's -Wmissing-prototypes option can identify some global functions that can be made static (or removed if they are unused).

-Wmissing-prototypes warns about global functions definitions that have no previous function prototype (e.g. from a header file) in scope. These functions could be made static (e.g. bug 1443402) because they are probably not called by other compilation units. Without a function prototype in a header file, another compilation unit would need to declare its own prototype for the function or use some dynamic mechanism like dlsym().

https://clang.llvm.org/docs/DiagnosticsReference.html#wmissing-prototypes

Unfortunately, there are currently 1331 -Wmissing-prototypes warnings, so fixing them all is probably impractical. Also, these warnings would not catch global functions that do have a function prototype but are still only called from one compilation unit (or not called at all).

Confusingly, gcc's -Wmissing-prototypes option is not equivalent to clang's -Wmissing-prototypes. -Wmissing-declarations is gcc's name for clang's -Wmissing-prototypes.

https://gcc.gnu.org/onlinedocs/gcc-8.2.0/gcc/Warning-Options.html

(In reply to Christian Holler (:decoder) from comment #15)

What Sylvestre meant is to use the data produced by the searchfox Clang Plugin, not putting it into searchfox. I think a new Clang plugin wouldn't save a lot, you would still have to generate all the data, then postprocess it (because you have to scan for other invocations of the function, which you can't do directly from the Clang plugin).

I don't know if the searchfox data has all the information available for this, but I can take a look at it next week maybe.

Ah I see. The goals of searchfox and this bug still seem fundamentally different to me, so I'm not sure code sharing is such a good idea. Feel free to fork the searchfox clang-plugin though. If you do reuse the code as-is keep in mind the output format is subject to change at any time, no guarantees provided etc. Also the searchfox jobs currently only run on {mac,linux,windows}-debug builds so they might miss stuff.

See Also: → 1528600
See Also: → 1528881
See Also: → 1532128
Depends on: 1534878
OS: Linux → Unspecified
Hardware: x86 → Unspecified
See Also: 1528600, 1528881, 1532128

Is this patch good to go? If not, can it be updated?

You need to log in before you can comment on or make changes to this bug.