-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Expand file tree
/
Copy pathCODING-GUIDELINES
More file actions
280 lines (231 loc) · 12.1 KB
/
Copy pathCODING-GUIDELINES
File metadata and controls
280 lines (231 loc) · 12.1 KB
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
Refer also to this page for more information -
https://docs.libretro.com/development/coding-standards/
Struct ordering
---------------
For POD-types, try to order structs as follows (first to last):
* long double (8 bytes, 16 bytes [64bit x86], 12 bytes [32bit x86])
* double (8 bytes)
* int64_t (8 bytes, 8 bytes [32bit ARM],
4 bytes [32bit x86])
* uint64_t (4 bytes [32bit], 8 bytes [32bit ARM], 8 bytes [64bit])
* pointer (4 bytes [32bit], 8 bytes [64bit] [1])
* intptr_t (4 bytes [32bit], 8 bytes [64bit] [1])
* uintptr_t (4 bytes [32bit], 8 bytes [64bit] [1])
* ptrdiff_t (4 bytes [32bit], 8 bytes [64bit] [1])
* ssize_t (4 bytes [32bit], 8 bytes [64bit])
* size_t (4 bytes [32bit], 8 bytes [64bit])
* jmp_buf (4 bytes)
* long (4 bytes [64bit Win], 8 bytes [64bit non-Win],
4 bytes [32bit])
* int32_t (4 bytes)
* unsigned (4 bytes)
* float (4 bytes)
* int (4 bytes)
* enum (4 bytes)
* int16_t (2 bytes)
* char (1 byte)
* bool (1 byte)
[1] PS3 uses 4 byte pointers despite having a 64bit processor
Struct members should be sorted by alignment. Therefore, structs
should be sorted by the largest type inside them.
For example, take a struct like this:
typedef struct
{
size_t capacity;
bool old_format;
bool compress;
bool fuzzy_archive_match;
bool autofix_paths;
char path[PATH_MAX_LENGTH];
char base_content_directory[DIR_MAX_LENGTH];
} playlist_config_t;
size_t has the biggest alignment here, so 'struct playlist_config_t'
inside a struct should come before or after size_t.
*** BEST PRACTICES ***
* If we have pointers and size variable pairs, it's best to
interleave them to increase the probability they go in the
same cacheline. It also makes the code more readable, that
these two variables are connected.
Example:
struct a
{
char* b;
size_t b_len;
char* c;
size_t c_len;
};
Stack size
----------
You have to assume that stack size is going to be limited in
RetroArch. These are the thread stacks the tree creates today:
* GEKKO (Wii/GC) 8 KiB STACKSIZE, rthreads/gx_pthread.h
* 3DS 32 KiB STACKSIZE, rthreads/ctr_pthread.h
* Vita 64 KiB rthreads.c, 0x10000
* Apple 2 MiB rthreads.c, raised from a 512 KiB default
* Windows 1 MiB reserved by default, committed lazily
8 KiB on GEKKO is the floor, and it is the number to design
against. (rthreads/psp_pthread.h names 8 KiB as well but nothing
includes it - rthreads.c takes gx_pthread.h under GEKKO and
ctr_pthread.h under _3DS, and PSP falls through to plain pthreads.)
Which thread your code runs on is not something you get to assume.
Anything reachable from a task handler can end up on one of these:
retro_task_regular_gather() runs handlers on the main thread, but
with a threaded task queue the same handler runs on a worker.
*** THE BUDGET ***
- 4 KiB is the ceiling for a single stack frame anywhere in the
tree.
- 2 KiB is the ceiling for anything under libretro-common, and it
is enforced. tools/stack_budget.py measures every shipped .c
there with -fstack-usage and fails the build over it; it runs in
CI as part of Linux-libretro-common-tests.yml. Read the header
of that script before adding an entry to its allowlist - an
entry there is debt, not an exemption.
Neither number is a per-function allowance to spend. A frame does
not run alone, it runs on top of everything that called it. Ten
1.5 KiB frames in a row pass every per-frame check ever written
and still overflow an 8 KiB stack, which is why the budget sits so
far below the smallest stack.
*** WHY 4 KIB ***
- Small-stack targets. See the table above. A 16 KiB local buffer
is twice the whole GEKKO stack, and nothing warns about it: the
code builds everywhere and overflows only on a target most
contributors cannot build, at whatever moment it first runs on a
worker. config_file_write() carried a 16 KiB stdio buffer as a
local on a path reached from input_autoconfigure_connect_handler
- a task handler - and nothing noticed for years.
- Stack probes on Windows. 4 KiB is the page size, and Windows
commits thread stacks lazily: one guard page sits below the
committed region, and touching it is what makes the OS commit
another page and move the guard down. A frame that moves the
stack pointer past the guard page without touching it faults
outside the guard page, which the OS treats as a plain access
violation rather than a request to grow the stack - the process
dies. Compilers avoid that by emitting a probe: MSVC's default
is /Gs4096 on x86, x64, ARM and ARM64, so any function needing
more than one page of locals gets a call to __chkstk (or
___chkstk_ms under mingw-w64, which uses the same threshold)
that walks the frame page by page in order. So crossing 4 KiB
turns a single subtract into an out-of-line loop at every entry
to that function, and commits every page it touches for the life
of the thread. GCC's -fstack-clash-protection does the same
thing on Linux, and the kernel-side reason is the same.
- Recursion and reentrancy. A frame you can afford once is a frame
you cannot afford at depth 40. Anything recursive - directory
walks, archive nesting, parsers - should carry its state in a
heap-allocated context and bound its depth explicitly, not lean
on the stack to hold the recursion for it.
*** IN PRACTICE ***
- Measure rather than guess. For one file:
gcc -O2 -DPSP -fstack-usage -c foo.c -Ilibretro-common/include
and read foo.su. Or point the script at a file or a directory:
tools/stack_budget.py libretro-common/formats/. -DPSP matters,
because PATH_MAX_LENGTH is 512 on those targets and 2048
elsewhere - a host-shaped measurement overstates every function
holding a path buffer. Adding -Wstack-usage=4096 to a local
build turns the ceiling into a warning while you work.
Frame size is a property of an ABI, not of the source; the same
function measures 16 to 192 bytes larger under mingw-w64 than
under SysV. Do not compare an absolute number against one taken
from a different target triple.
- Per-frame is necessary and not sufficient. tools/stack_chain.py
sums frames along the call graph from a set of roots (task
handlers being the roots that matter) and reports the deepest
chain. It is a screening tool rather than a verdict, which is
why it is not in CI - read its header for what it cannot see.
- Path buffers are the usual culprit. On desktop PATH_MAX_LENGTH
is 2048, so two of them in one frame is already over the
ceiling. Use DIR_MAX_LENGTH (1024) and NAME_MAX_LENGTH (128)
when that is what the buffer actually holds, and reuse one
buffer rather than carrying a second.
- No VLAs and no alloca(). VLAs are not C89 (see below); alloca()
additionally makes the frame unbounded at compile time, and
-fstack-usage can only report such a frame as dynamic, so it
drops out of every check above.
- When a frame is too large, the question is not "can this
overflow today" but "does this need to be on the stack at all".
The answer is usually no, and the fix is usually one of: move
the buffer to the heap, or move it into a state object that is
already allocated once per stream/session, or shrink it. Real
examples, all done rather than argued about:
vh_build 16784 -> 400 scratch to the heap
config_file_write 16432 -> 48 stdio buffer to heap
chd_read_header_core_file 57504 -> 32 chd_file to the heap
rd_gen_lengths 12768 -> 352 scratch into the struct
sha1_calculate 4304 -> 224 mapped view, else heap
- None of this means small structs belong on the heap. Heap
allocations are slow compared to the stack, and scattering tiny
ones fragments memory - which matters most on exactly the
platforms with the small stacks. A balancing act is necessary:
a few hundred bytes of locals is fine and idiomatic, a kilobyte
wants a reason, and 4 KiB is where the decision is made for you.
Functions
---------
- Avoid doing small getter/setter functions. We want a function
to justify its function call overhead by doing a significant
body of work. Small one-line getter/setter functions for what
is predominantly C-style structs is not useful, plus it leads
to people thinking this function is more complex than it
actually is, thus obfuscating the sourcecode instead of it
being easier to read.
If you can find examples in the codebase that violate this
guideline, do not hesitate to point them out to us.
Variable declaration
--------------------
For C source files, we have to insist you stick to the following:
- Declare variables either at the start of a function or the start
of a code block, depending on the scope they need.
- Do not do initial for loop declarations. Refer to the bulletpoint above:
either declare them at the start of the function, or at the start
of the code block.
Not doing this would break compilation on platforms where we are compiling
these C source files in C89 compatibility mode. If such issues occur in pull
requests, we have to request that it be fixed.
VLA (Variable Length Array)
---------------------------
Do not use VLAs (Variable Length Array) in C source files. These are not
C89-compliant.
Main thread and task handlers
-----------------------------
The UI and the main thread must never block. When Threaded Tasks is
disabled - which it is on several platforms, and which users turn off
deliberately - task handlers run on the same thread that drives the
frame loop, so anything a handler does between returns is a frame the
UI cannot paint.
- Do not call task_queue_wait() from code reachable while the menu is
visible or a core is running. Cancel the work and guard against a
stale completion, or move what follows the wait into the task's own
callback. A last-resort safety net is acceptable only where an
invariant genuinely requires it (a worker must not call into a core
that is about to be unloaded, for instance), and then it must be
bounded and commented.
- Never pass a NULL condition to task_queue_wait(). It does not mean
"wait for my task": both queue implementations read it as "until the
queue is empty", so the caller ends up waiting out every unrelated
task in flight - a content scan, a core download. Pass a condition
that describes your own work.
- A handler that processes a collection must consult the shared
per-frame I/O window (task_nbio_slice_open /
task_nbio_slice_within_budget / task_nbio_slice_close) between
items, and must always complete at least one item per invocation so
an exhausted window cannot stall it. Do NOT chunk by a count derived
from the workload - "half the entries", "a quarter of the files" -
because that is not pacing: a bigger job simply produces bigger
chunks and the same number of invocations.
- Putting work on a background thread is not a substitute for pacing
it. The threaded queue is optional; the budget is what protects the
frame loop in the configuration that has no worker.
Debug builds report handlers that overrun a frame - see
task_queue_set_slow_handler_cb() - which is the cheapest way to find
out whether a handler you are adding respects any of this.
Commit message density
----------------------
Keep commit message short and appropriate for the size of the change. One or two
paragraphs appropriately sized based on the size of the change. Only explain how the
code works moving forward, not how it worked historically. Latter rule should also apply to code comments.
Miscellaneous
-------------
- Brace usage follows "Allman style". The brace associated with a control statement is placed on the following line,
indented to the same level as the control statement.
Statements within the braces are indented to the next level.
- A single statement block must not include brackets (unless the block uses a macro that expands into multiple lines)
- If possible, avoid 'while (true)' and use 'for (;;)' instead