[PATCH] D129107: [BOLT][HUGIFY] adds huge pages support of PIE/no-PIE binaries

[Rafael Auler via Phabricator via llvm-commits]

rafauler added a comment.

Thanks for working on improving hugify!



================
Comment at: bolt/include/bolt/Passes/Hugify.h:3-6
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
----------------
This is the correct license


================
Comment at: bolt/lib/Passes/Hugify.cpp:2-6
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
----------------
Outdated license


================
Comment at: bolt/lib/Passes/Hugify.cpp:55
+  const MCSymbol *StartSym = Start->getSymbol();
+  createSimpleFunction("__bolt_hugify_init_ptr",
+                       BC.MIB->createSymbolTrampoline(StartSym, BC.Ctx.get()));
----------------
update name here


================
Comment at: bolt/lib/RuntimeLibs/HugifyRuntimeLibrary.cpp:64-87
   const BinaryFunction *StartFunction =
       BC.getBinaryFunctionAtAddress(*(BC.StartFunctionAddress));
   assert(!StartFunction->isFragment() && "expected main function fragment");
   if (!StartFunction) {
     errs() << "BOLT-ERROR: failed to locate function at binary start address\n";
     exit(1);
   }
----------------
I think we can delete all of this, right? emitBinary() for HugyfiRuntimeLibrary doesn't need to do anything if you are already creating a new function in a new pass that will be called from the runtime.

And then go to HugifyRuntimeLibrary.h and delete
      SecNames.push_back(".bolt.hugify.entries");


================
Comment at: bolt/runtime/common.h:416
+
+int __uname(struct utsname *buf) {
+  int ret;
----------------
capitalize Buf to follow LLVM coding style


================
Comment at: bolt/runtime/common.h:524-525
+//                        long arg2   long arg3   long arg4   long arg5
+int __prctl(int option, unsigned long arg2, unsigned long arg3,
+            unsigned long arg4, unsigned long arg5) {
+  int ret;
----------------
capitalize Option, Arg2, etc.  to follow LLVM coding style


================
Comment at: bolt/runtime/hugify.cpp:1-8
+//===-- hugify.cpp ----------------------------------------------*- C++ -*-===//
 //
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+// This file contains code that is linked to the final binary with a function
----------------
This is the wrong license. LLVM license has updated to Apache v2.0 with LLVM Exceptions (the one that was previously in the header). Could you revert back this change?


================
Comment at: bolt/runtime/hugify.cpp:22
+// so we can resume regular execution of the function that we hooked.
+extern void __bolt_hugify_init_ptr();
 
----------------
Can you remove "_ptr" from the name, since it is not a function pointer anymore? I suggest naming "__bolt_hugify_start_program" (but it's just a suggestion)


================
Comment at: bolt/runtime/hugify.cpp:29-46
+/// Starting from character at \p buf, find the longest consecutive sequence
+/// of digits (0-9) and convert it to uint32_t. The converted value
+/// is put into \p ret. \p end marks the end of the buffer to avoid buffer
+/// overflow. The function \returns whether a valid uint32_t value is found.
+/// \p buf will be updated to the next character right after the digits.
+static bool scanUInt32(const char *&buf, const char *end, uint32_t &ret) {
+  uint64_t result = 0;
----------------
Move to common.h, close to "hexToLong"

capitalize variables to follow llvm style (see hexToLong example).


================
Comment at: bolt/runtime/hugify.cpp:48
+
+static void get_kernel_version(uint32_t *val) {
+  // release should be in the format: %d.%d.%d
----------------
getKernelVersion(Val)

to follow LLVM coding style

(I realize this file doesn't follow LLVM style, but it is small enough that we can just update it)


================
Comment at: bolt/runtime/hugify.cpp:76
+/// thp works only starting from 5.10
 static bool has_pagecache_thp_support() {
+  char buf[64];
----------------
hasPagecacheTHPSupport


================
Comment at: bolt/runtime/hugify.cpp:77
 static bool has_pagecache_thp_support() {
-  char buf[256] = {0};
-  const char *madviseStr = "always [madvise] never";
+  char buf[64];
+  const uint64_t madvise_options = 2;
----------------
Buf


================
Comment at: bolt/runtime/hugify.cpp:81
 
   int fd = __open("/sys/kernel/mm/transparent_hugepage/enabled",
                   0 /* O_RDONLY */, 0);
----------------
FD


================
Comment at: bolt/runtime/hugify.cpp:91-97
+  typedef struct {
+    uint32_t major;
+    uint32_t minor;
+    uint32_t release;
+  } kernel_version_t;
+
+  kernel_version_t kernel_version;
----------------


struct KernelVersionTy {
    uint32_t major;
    uint32_t minor;
    uint32_t release;
  };

KernelVersionTy KernelVersion;


================
Comment at: bolt/runtime/hugify.cpp:110
+
+static void hugify_for_old_kernel(uint8_t *from, uint8_t *to,
+                                  uint8_t *fromAlignedPage,
----------------
hugifyForOldKernel(From, To ..


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129107/new/

https://reviews.llvm.org/D129107