1============== 2MyFirstTypoFix 3============== 4 5.. contents:: 6 :local: 7 8Introduction 9============ 10 11This tutorial will guide you through the process of making a change to 12LLVM, and contributing it back to the LLVM project. 13 14.. note:: 15 The code changes presented here are only an example and not something you 16 should actually submit to the LLVM project. For your first real change to LLVM, 17 the code will be different but the rest of the guide will still apply. 18 19We'll be making a change to Clang, but the steps for other parts of LLVM are the same. 20Even though the change we'll be making is simple, we're going to cover 21steps like building LLVM, running the tests, and code review. This is 22good practice, and you'll be prepared for making larger changes. 23 24We'll assume you: 25 26- know how to use an editor, 27 28- have basic C++ knowledge, 29 30- know how to install software on your system, 31 32- are comfortable with the command line, 33 34- have basic knowledge of git. 35 36 37The change we're making 38----------------------- 39 40Clang has a warning for infinite recursion: 41 42.. code:: console 43 44 $ echo "void foo() { foo(); }" > ~/test.cc 45 $ clang -c -Wall ~/test.cc 46 test.cc:1:12: warning: all paths through this function will call itself [-Winfinite-recursion] 47 48This is clear enough, but not exactly catchy. Let's improve the wording 49a little: 50 51.. code:: console 52 53 test.cc:1:12: warning: to understand recursion, you must first understand recursion [-Winfinite-recursion] 54 55 56Dependencies 57------------ 58 59We're going to need some tools: 60 61- git: to check out the LLVM source code, 62 63- a C++ compiler: to compile LLVM source code. You'll want `a recent 64 version <host_cpp_toolchain>` of Clang, GCC, or Visual Studio. 65 66- CMake: used to configure how LLVM should be built on your system, 67 68- ninja: runs the C++ compiler to (re)build specific parts of LLVM, 69 70- python: to run the LLVM tests. 71 72As an example, on Ubuntu: 73 74.. code:: console 75 76 $ sudo apt-get install git clang cmake ninja-build python 77 78 79Building LLVM 80============= 81 82 83Checkout 84-------- 85 86The source code is stored `on 87Github <https://github.com/llvm/llvm-project>`__ in one large repository 88("the monorepo"). 89 90It may take a while to download! 91 92.. code:: console 93 94 $ git clone https://github.com/llvm/llvm-project.git 95 96This will create a directory "llvm-project" with all of the source 97code. (Checking out anonymously is OK - pushing commits uses a different 98mechanism, as we'll see later.) 99 100Configure your workspace 101------------------------ 102 103Before we can build the code, we must configure exactly how to build it 104by running CMake. CMake combines information from three sources: 105 106- explicit choices you make (is this a debug build?) 107 108- settings detected from your system (where are libraries installed?) 109 110- project structure (which files are part of 'clang'?) 111 112First, create a directory to build in. Usually, this is ``llvm-project/build``. 113 114.. code:: console 115 116 $ mkdir llvm-project/build 117 $ cd llvm-project/build 118 119Now, run CMake: 120 121.. code:: console 122 123 $ cmake -G Ninja ../llvm -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_PROJECTS=clang 124 125If all goes well, you'll see a lot of "performing test" lines, and 126finally: 127 128.. code:: console 129 130 Configuring done 131 Generating done 132 Build files have been written to: /path/llvm-project/build 133 134And you should see a ``build.ninja`` file in the current directory. 135 136Let's break down that last command a little: 137 138- **-G Ninja**: Tells CMake that we're going to use ninja to build, and to create 139 the ``build.ninja`` file. 140 141- **../llvm**: this is the path to the source of the "main" LLVM 142 project 143 144- The two **-D** flags set CMake variables, which override 145 CMake/project defaults: 146 147 - **CMAKE_BUILD_TYPE=Release**: build in optimized mode, which is 148 (surprisingly) the fastest option. 149 150 If you want to run under a debugger, you should use the default Debug 151 (which is totally unoptimized, and will lead to >10x slower test 152 runs) or RelWithDebInfo which is a halfway point. 153 154 Assertions are not enabled in ``Release`` builds by default. 155 You can enable them using ``LLVM_ENABLE_ASSERTIONS=ON``. 156 157 - **LLVM_ENABLE_PROJECTS=clang**: this lists the LLVM subprojects 158 you are interested in building, in addition to LLVM itself. Multiple 159 projects can be listed, separated by semicolons, such as ``clang;lldb``. 160 In this example, we'll be making a change to Clang, so we only add clang. 161 162Finally, create a symlink (or copy) of ``llvm-project/build/compile-commands.json`` 163into ``llvm-project/``: 164 165.. code:: console 166 167 $ ln -s build/compile_commands.json ../ 168 169(This isn't strictly necessary for building and testing, but allows 170tools like clang-tidy, clang-query, and clangd to work in your source 171tree). 172 173 174Build and test 175-------------- 176 177Finally, we can build the code! It's important to do this first, to 178ensure we're in a good state before making changes. But what to build? 179In ninja, you specify a **target**. If we just want to build the clang 180binary, our target name is "clang" and we run: 181 182.. code:: console 183 184 $ ninja clang 185 186The first time we build will be very slow - Clang + LLVM is a lot of 187code. But incremental builds are fast: ninja will only rebuild the parts 188that have changed. When it finally finishes you should have a working 189clang binary. Try running: 190 191.. code:: console 192 193 $ bin/clang --version 194 195There's also a target for building and running all the clang tests: 196 197.. code:: console 198 199 $ ninja check-clang 200 201This is a common pattern in LLVM: check-llvm is all the checks for the core of 202LLVM, other projects have targets like ``check-lldb``, ``check-flang`` and so on. 203 204 205Making changes 206============== 207 208 209The Change 210---------- 211 212We need to find the file containing the error message. 213 214.. code:: console 215 216 $ git grep "all paths through this function" .. 217 ../clang/include/clang/Basic/DiagnosticSemaKinds.td: "all paths through this function will call itself">, 218 219The string that appears in ``DiagnosticSemaKinds.td`` is the one that is 220printed by Clang. ``*.td`` files define tables - in this case it's a list 221of warnings and errors clang can emit and their messages. Let's update 222the message in your favorite editor: 223 224.. code:: console 225 226 $ vi ../clang/include/clang/Basic/DiagnosticSemaKinds.td 227 228Find the message (it should be under ``warn_infinite_recursive_function``). 229Change the message to "in order to understand recursion, you must first understand recursion". 230 231 232Test again 233---------- 234 235To verify our change, we can build clang and manually check that it 236works. 237 238.. code:: console 239 240 $ ninja clang 241 $ bin/clang -c -Wall ~/test.cc 242 test.cc:1:12: warning: in order to understand recursion, you must first understand recursion [-Winfinite-recursion] 243 244We should also run the tests to make sure we didn't break something. 245 246.. code:: console 247 248 $ ninja check-clang 249 250Notice that it is much faster to build this time, but the tests take 251just as long to run. Ninja doesn't know which tests might be affected, 252so it runs them all. 253 254.. code:: console 255 256 ******************** 257 Failing Tests (1): 258 Clang :: SemaCXX/warn-infinite-recursion.cpp 259 260Well, that makes sense… and the test output suggests it's looking for 261the old string "call itself" and finding our new message instead. 262Note that more tests may fail in a similar way as new tests are added 263over time. 264 265Let's fix it by updating the expectation in the test. 266 267.. code:: console 268 269 $ vi ../clang/test/SemaCXX/warn-infinite-recursion.cpp 270 271Everywhere we see ``// expected-warning{{call itself}}`` (or something similar 272from the original warning text), let's replace it with 273``// expected-warning{{to understand recursion}}``. 274 275Now we could run **all** the tests again, but this is a slow way to 276iterate on a change! Instead, let's find a way to re-run just the 277specific test. There are two main types of tests in LLVM: 278 279- **lit tests** (e.g. ``SemaCXX/warn-infinite-recursion.cpp``). 280 281These are fancy shell scripts that run command-line tools and verify the 282output. They live in files like 283``clang/**test**/FixIt/dereference-addressof.c``. Re-run like this: 284 285.. code:: console 286 287 $ bin/llvm-lit -v ../clang/test/SemaCXX/warn-infinite-recursion.cpp 288 289- **unit tests** (e.g. ``ToolingTests/ReplacementTest.CanDeleteAllText``) 290 291These are C++ programs that call LLVM functions and verify the results. 292They live in suites like ToolingTests. Re-run like this: 293 294.. code:: console 295 296 $ ninja ToolingTests && tools/clang/unittests/Tooling/ToolingTests --gtest_filter=ReplacementTest.CanDeleteAllText 297 298 299Commit locally 300-------------- 301 302We'll save the change to a local git branch. This lets us work on other 303things while the change is being reviewed. Changes should have a 304title and description, to explain to reviewers and future readers of the code why 305the change was made. 306 307For now, we'll only add a title. 308 309.. code:: console 310 311 $ git checkout -b myfirstpatch 312 $ git commit -am "[clang][Diagnostic] Clarify -Winfinite-recursion message" 313 314Now we're ready to send this change out into the world! 315 316The ``[clang]`` and ``[Diagnostic]`` prefixes are what we call tags. This loose convention 317tells readers of the git log what areas a change is modifying. If you don't know 318the tags for the modules you've changed, you can look at the commit history 319for those areas of the repository. 320 321.. code:: console 322 323 $ git log --oneline ../clang/ 324 325Or using GitHub, for example https://github.com/llvm/llvm-project/commits/main/clang. 326 327Tagging is imprecise, so don't worry if you are not sure what to put. Reviewers 328will suggest some if they think they are needed. 329 330Code review 331=========== 332 333Uploading a change for review 334----------------------------- 335 336LLVM code reviews happen through pull-request on GitHub, see the 337:ref:`GitHub <github-reviews>` documentation for how to open 338a pull-request on GitHub. 339 340Finding a reviewer 341------------------ 342 343Changes can be reviewed by anyone in the LLVM community. For larger and more 344complicated changes, it's important that the 345reviewer has experience with the area of LLVM and knows the design goals 346well. The author of a change will often assign a specific reviewer. ``git blame`` 347and ``git log`` can be useful to find previous authors who can review. 348 349Our GitHub bot will also tag and notify various "teams" around LLVM. The 350team members contribute and review code for those specific areas regularly, 351so one of them will review your change if you don't pick anyone specific. 352 353Review process 354-------------- 355 356When you open a pull-request, some automation will add a comment and 357notify different members of the sub-projects depending on the parts you have 358changed. 359 360Within a few days, someone should start the review. They may add 361themselves as a reviewer, or simply start leaving comments. You'll get 362another email any time the review is updated. For more detail see the 363:ref:`Code Review Poilicy <code_review_policy>`. 364 365Comments 366~~~~~~~~ 367 368The reviewer can leave comments on the change, and you can reply. Some 369comments are attached to specific lines, and appear interleaved with the 370code. You can reply to these. Perhaps to clarify what was asked or to tell the 371reviewer that you have done what was asked. 372 373Updating your change 374~~~~~~~~~~~~~~~~~~~~ 375 376If you make changes in response to a reviewer's comments, simply update 377your branch with more commits and push to your GitHub fork of ``llvm-project``. 378It is best if you answer comments from the reviewer directly instead of expecting 379them to read through all the changes again. 380 381For example you might comment "I have done this." or "I was able to do this part 382but have a question about...". 383 384Review expectations 385------------------- 386 387In order to make LLVM a long-term sustainable effort, code needs to be 388maintainable and well tested. Code reviews help to achieve that goal. 389Especially for new contributors, that often means many rounds of reviews 390and push-back on design decisions that do not fit well within the 391overall architecture of the project. 392 393For your first patches, this means: 394 395- be kind, and expect reviewers to be kind in return - LLVM has a 396 :ref:`Code of Conduct <LLVM Community Code of Conduct>` 397 that everyone should be following; 398 399- be patient - understanding how a new feature fits into the 400 architecture of the project is often a time consuming effort, and 401 people have to juggle this with other responsibilities in their 402 lives; **ping the review once a week** when there is no response; 403 404- if you can't agree, generally the best way is to do what the reviewer 405 asks; we optimize for readability of the code, which the reviewer is 406 in a better position to judge; if this feels like it's not the right 407 option, you can ask them in a comment or add another reviewer to get a second 408 opinion. 409 410 411Accepting a pull request 412------------------------ 413 414When the reviewer is happy with the change, they will **Approve** the 415pull request. They may leave some more minor comments that you should 416address before it is merged, but at this point the review is complete. 417It's time to get it merged! 418 419 420Commit access 421============= 422 423Commit by proxy 424--------------- 425 426As this is your first change, you won't have access to merge it 427yourself yet. The reviewer **does not know this**, so you need to tell 428them! Leave a comment on the review like: 429 430 Thanks @<username of reviewer>. I don't have commit access, can you merge this 431 PR for me? 432 433The pull-request will be closed and you will be notified by GitHub. 434 435Getting commit access 436--------------------- 437 438Once you've contributed a handful of patches to LLVM, start to think 439about getting commit access yourself. It's probably a good idea if: 440 441- you've landed 3-5 patches of larger scope than "fix a typo" 442 443- you'd be willing to review changes that are closely related to yours 444 445- you'd like to keep contributing to LLVM. 446 447 448The process is described in the :ref:`developer policy document <obtaining_commit_access>`. 449 450With great power 451---------------- 452 453Actually, this would be a great time to read the rest of the :ref:`developer 454policy <developer_policy>` too. 455 456 457.. _MyFirstTypoFix Issues After Landing Your PR: 458 459Issues After Landing Your PR 460============================ 461 462Once your change is submitted it will be picked up by automated build 463bots that will build and test your patch in a variety of configurations. 464 465The "console" view at http://lab.llvm.org/buildbot/#/console displays results 466for specific commits. If you want to follow how your change is affecting the 467build bots, this should be the first place to look. 468 469The columns are build configurations and the rows are individual commits. Along 470the rows are colored bubbles. The color of the bubble represents the build's 471status. Green is passing, red has failed and yellow is a build in progress. 472 473A red build may have already been failing before your change was committed. This 474means you didn't break the build but you should check that you did not make it 475any worse by adding new problems. 476 477.. note:: 478 Only recent changes are shown in the console view. If your change is not 479 there, rely on PR comments and build bot emails to notify you of any problems. 480 481If there is a problem in a build that includes your changes, you may receive 482a report by email or as a comment on your PR. Please check whether the problem 483has been caused by your changes specifically. As builds contain changes from 484many authors and sometimes fail due to unrelated infrastructure problems. 485 486To see the details of a build, click the bubble in the console view, or the link 487provided in the problem report. You will be able to view and download logs for 488each stage of that build. 489 490If you need help understanding the problem, or have any other questions, you can 491ask them as a comment on your PR, or on `Discord <https://discord.com/invite/xS7Z362>`__. 492 493If you do not receive any reports of problems, no action is required from you. 494Your changes are working as expected, well done! 495 496Reverts 497------- 498 499If your change has caused a problem, it should be reverted as soon as possible. 500This is a normal part of :ref:`LLVM development <revert_policy>`, 501that every committer (no matter how experienced) goes through. 502 503If you are in any doubt whether your change can be fixed quickly, revert it. 504Then you have plenty of time to investigate and produce a solid fix. 505 506Someone else may revert your change for you, or you can create a revert pull request using 507the `GitHub interface <https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/reverting-a-pull-request>`__. 508Add your original reviewers to this new pull request if possible. 509 510Conclusion 511========== 512 513Now you should have an understanding of the life cycle of a contribution to the 514LLVM Project. 515 516If some details are still unclear, do not worry. The LLVM Project's process does 517differ from what you may be used to elsewhere on GitHub. Within the project 518the expectations of different sub-projects may vary too. 519 520So whatever you are contributing to, know that we are not expecting perfection. 521Please ask questions whenever you are unsure, and expect that if you have missed 522something, someone will politely point it out and help you address it. 523