User Details
- User Since
- Dec 30 2016, 3:24 PM (281 w, 2 d)
- Roles
- Administrator
Today
Perhaps wait a day or so in case others have opinions.
IIRC there is no built-in way supporting multiple (but fixed number of) values for an option (e.g. -Xoffload-linker-<triple> <arg>). In D105330 (llvm-nm option refactoring) I used a hack to support -s __DATA __data.
The multiple-value support for OptTable does not allow positional arguments after the option.
It's better to avoid JoinedAndSeparate for new options. It is for --xxx val and --xxxval but not intended for the option this patch will add.
Sat, May 21
LGTM. Thanks for pinging the relevant folks on individual tests. You can push the fixes yourself before landing this patch.
FP_XSTATE_MAGIC1 is only available on glibc 2.27 and upwards
The name is not descriptive. It misses the important words about "hash map". ConcurrentHashMap may be a better name.
Where is the undefined behavior? size_t is unsigned long long on 32-bit ELF platforms. You need %zu.
Fix MCSymbolizer::tryAddingSymbolicOperand() interface
However, for proper symbolic disassembly on X86 we need to know both sizes.
Fri, May 20
Overloaded virtual functions seem more difficult to read and are more error-prone. Having distinct function names can be useful as people searching code can find the relevant one easily.
Thu, May 19
While I think there are still significant problems needing to address, it may work with quite a few application. It'd be nice if folks can check how this patch works without -mno-relax.
On lld/ELF/Relocations.cpp:1439, R_RELAX_HINT should not trigger sym.hasDirectReloc = true;, otherwise an ifunc may unnecessarily be converted to a canonical PLT. You may read *-ifunc-nonpreemptible*.s tests for what they do. I have some notes on https://maskray.me/blog/2021-01-18-gnu-indirect-function#address-significance
Thanks!
I think placing the OS before && is more common.
Seems fine to me for libunwind, but I don't know the build bot setup enough. I will give a LGTM if you still lack an approval to make #libunwind green :)
Wed, May 18
(issue 55527)
We don't attach PR number to test filenames. That's old practice which has been abandoned.
You can add the bug number to the body of the file is necessary. In this case I'd say not necessary.
Reviewers: David Tenty,Mark de Wever
In addition, if every new GCC release requires addition to /opt/rh/gcc-toolset-$major (if I understand correctly), we probably want to switch to llvm::vfs::directory_iterator iteration and using the largest version.
It may be time to add unittests to clang/unittests/Driver/ToolChainTest.cpp for /opt/rh detection. You may use TEST(ToolChainTest, VFSGCCInstallation) as an example creating a pseudo file hierarchy in memory.
The differential has the same intention/subject as D118352. In such a case reusing the preexisting differential would have been better. I have left more comments on https://reviews.llvm.org/D125825#3522911
I see that a new patch with the same intent/subject has been created: D125880.
Normally the same differential should be used to keep track of all past discussions on a differential.
The "History" tab on the page allows a user to compare two patches to get insights of the evolution of the differential.
Tue, May 17
Please add some description about what tests previously failed now work.
Looks great! Best to wait for a second opinion.
s/riscv/RISCV/
Mon, May 16
With sdesmalen's suggestion applied.
I haven't checked but for ninja check-llvm-tools-gold, we still hope that the tests are disabled if the system does not provide gold. There may need a toggle for the tests.
Special thanks to Julian Mestre for creating this block coverage inference algorithm.
Thanks for the patch. User-facing tools are better using llvm::OptTable. I've only spot one thing other than what has been mentioned.
This seems like work of a default linker script. Nowadays you may use OVERWRITE_SECTIONS to create a new output section description: https://maskray.me/blog/2021-07-04-sections-and-overwrite-sections
Sat, May 14
Clone https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git , apply this patch:
--- i/arch/riscv/Makefile +++ w/arch/riscv/Makefile @@ -38,9 +38,9 @@ endif
Is there a specification or reference implementation stating that -E is used?
Fri, May 13
According to a developer at the FreeBSD project, FreeBSD's total compilation time increases by 2.6% when the host system is built against compiler-rt instead of libgcc. This is likely due to the fact that GCC has assembly-written versions of the division and modulo routines, while compiler-rt does not.
It may be useful to leave a space before the period as many applications may consider . part of the URI.
LGTM. (I need to learn this stuff but I'm happy since bots are happy:))
I'll wait a bit before pushing to check whether that further opinions.
LGTM.
Looks great! But best to wait for @jhenderson
Mostly looks good to me.