[PATCH] D42758: Support `#pragma comment(lib, "name")` in the frontend for ELF

jhenderson added a comment. In https://reviews.llvm.org/D42758#997880, @erichkeane wrote: > I'd like to see @probinson s PS4 discussion bottom out, but I don't see any > reason to hold this up otherwise. The PS4 compiler uses its linker options mechanism to communicate three different things

[PATCH] D59746: [LibTooling] Fix '-h' option

jhenderson added a comment. Just chiming in from a LLVM binutils developer perspective. Some of the binutils are missing -h as an alias, when they really should have it for compatibility (specifically I checked llvm-nm and llvm-symbolizer). As a result, a solution that auto-adds -h as an alias

[PATCH] D59746: [CommandLineParser] Add DefaultOption flag

jhenderson added a comment. I've not looked at the implementation in depth, but cl::DefaultOption seems like a nice way of handling this. Please make sure that there is testing in llvm-readobj and llvm-objdump that test their own short alias interpretation somewhere though. Repository: rG L

[PATCH] D83912: [llvm-readobj] Update tests because of changes at llvm-readobj behavior

jhenderson added a comment. Hi @Elvina, Just to let you know that I had to fix up three clang tests that were using the old behaviour too, prior to committing. I also noticed that you've got the stack the wrong way around - you needed to fix the tests before changing the behaviour in this case

[PATCH] D84473: Dump Accumulator

jhenderson added a comment. Could the llvm-readobj bit be spun off into a separate review, please. As things stand it doesn't have any of its own testing, which should be added at the same time as adding support in the tool itself. The new option is also lacking any documentation in the llvm-re

[PATCH] D84473: Dump Accumulator

jhenderson requested changes to this revision. jhenderson added a comment. This revision now requires changes to proceed. I'm assuming you need llvm-readobj to test your changes? In which case, the llvm-readobj change needs to come first, and then you can write tests using it to test this change

[PATCH] D84473: Dump Accumulator

jhenderson added a comment. By the way, has the design of this been discussed on llvm-dev? It seems like something should have an RFC if it hasn't already. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84473/new/ https://reviews.llvm.org/D84473 _

[PATCH] D77683: [Docs] Make code review policy clearer about requested pre-commit reviews

jhenderson added a comment. My impression is that you shouldn't be making changes in code you are not already familiar with without a review, even if the fix seems fairly obvious (it's surprising how often an "obvious" fix isn't actually the right thing to do). I'd have assumed this was covered

[PATCH] D81672: [Driver] When forcing a crash call abort to get the correct diagnostic

jhenderson added a comment. This seems sensible to me, but I have zero experience with the `CrashRecoveryContext` class. It might be best to get someone who has more knowledge of it to look at it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D8167

[PATCH] D81672: [Driver] When forcing a crash call abort to get the correct diagnostic

jhenderson added a comment. In D81672#2101513 , @aganea wrote: > But that won't work when compiling & crashing with `-fno-integrated-cc1`, > would it? (or if building with `cmake ... -DCLANG_SPAWN_CC1=1`). In that > case, normal crashes (not -gen-reprodu

[PATCH] D81672: [Driver] When forcing a crash print the bug report message

jhenderson accepted this revision. jhenderson added a comment. This revision is now accepted and ready to land. LGTM, but wait for others too, please. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81672/new/ https://reviews.llvm.org/D81672

[PATCH] D81672: [Driver] When forcing a crash print the bug report message

jhenderson accepted this revision. jhenderson added a comment. Latest version LGTM too. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81672/new/ https://reviews.llvm.org/D81672 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https:/

[PATCH] D74813: [RFC] Add hash of block contents to function block names

jhenderson added inline comments. Comment at: llvm/test/tools/llvm-cxxfilt/invalid.test:1 -RUN: llvm-cxxfilt -n _Z1fi __Z1fi f ___ZSt1ff_block_invoke | FileCheck %s +RUN: llvm-cxxfilt -n _Z1fi __Z1fi f ___ZSt1ff_block_invoke ___Z7my_mainv_block_invoke_ZTSi_ZTSj_ZTSPVK3sss | File

[PATCH] D79276: [FileCheck] Support comment directives

jhenderson added a comment. I hesitate to suggest this, but is it worth using `REM` as a comment prefix? It's the comment marker for Windows batch files, so has some precedence. Comment at: llvm/docs/CommandGuide/FileCheck.rst:57 + By default, FileCheck ignores any occurrence

[PATCH] D85337: [AMDGPU] gfx1031 target

jhenderson added inline comments. Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:1844 LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX1030), + LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX1031), LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_XNACK), Wh

[PATCH] D85337: [AMDGPU] gfx1031 target

jhenderson added inline comments. Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:1844 LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX1030), + LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX1031), LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_XNACK), ra

[PATCH] D85474: Add -fbinutils-version= to gate ELF features on the specified binutils version

jhenderson added a comment. Looks like there's only an `llc` test, but the option is in clang too? Should we have a test there? Is there any documentation for clang this option needs adding to? Also, this likely deserves a release note, I feel. Comment at: clang/include/clan

[PATCH] D79276: [FileCheck] Support comment directives

jhenderson added inline comments. Comment at: llvm/docs/CommandGuide/FileCheck.rst:57 + By default, FileCheck ignores any occurrence in ``match-filename`` of any check + prefix if it is preceded on the same line by "``COM:``" or "``RUN:``". See the + section `The "COM:" direct

[PATCH] D79276: [FileCheck] Support comment directives

jhenderson added inline comments. Comment at: llvm/lib/Support/FileCheck.cpp:1928 static const char *DefaultCheckPrefixes[] = {"CHECK"}; +static const char *DefaultCommentPrefixes[] = {"COM", "RUN"}; jdenny wrote: > jhenderson wrote: > > Similar to what I ment

[PATCH] D79276: [FileCheck] Support comment directives

jhenderson accepted this revision. jhenderson added a comment. This revision is now accepted and ready to land. LGTM, but might not hurt to have someone else looking at it. Comment at: llvm/test/FileCheck/comment/bad-comment-prefix.txt:40 +RUN:

[PATCH] D79276: [FileCheck] Support comment directives

jhenderson added inline comments. Comment at: llvm/test/FileCheck/comment/suffixes.txt:1-2 +# Comment prefixes plus check directive suffixes are not comment directives +# and are treated as plain text. + jdenny wrote: > jhenderson wrote: > > jdenny wrote: > > > j

[PATCH] D128612: RISC-V big-endian support implementation

jhenderson added inline comments. Comment at: llvm/include/llvm/ADT/Triple.h:864 /// Tests whether the target is RISC-V (32- and 64-bit). bool isRISCV() const { Perhaps worth updating to mention big and little endian here, like `isPPC64` above? ===

[PATCH] D128667: [WIP] Add Zstd ELF support

jhenderson added inline comments. Comment at: llvm/lib/ObjCopy/ELF/ELFObject.cpp:567-568 +compression::zstd::compress( +StringRef(reinterpret_cast(OriginalData.data()), + OriginalData.size()), +CompressedData); This StringRef

[PATCH] D128612: RISC-V big-endian support implementation

jhenderson added a comment. Objcopy aspects look good, thanks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128612/new/ https://reviews.llvm.org/D128612 ___ cfe-commits mailing list cfe-commits@lists.ll

[PATCH] D123100: [Support/Hash functions] Change the `final()` and `result()` of the hashing functions to return an array of bytes

jhenderson added inline comments. Comment at: bolt/lib/Core/DebugData.cpp:823 Hasher.update(AbbrevData); -StringRef Key = Hasher.final(); +auto Hash = Hasher.final(); +StringRef Key((const char *)Hash.data(), Hash.size()); Amir wrote: > I think i

[PATCH] D130689: [LLVM] Update C++ standard to 17

jhenderson accepted this revision. jhenderson added a comment. This revision is now accepted and ready to land. In D130689#3684330 , @mehdi_amini wrote: > Nice, LGTM, thanks for driving this! > >> Remember that if we want to adopt some new feature in a b

[PATCH] D130689: [LLVM] Update C++ standard to 17

jhenderson added a comment. FWIW, I've got a Visual Studio configuration (admittedly using a direct Visual Studio generator, not ninja), and I don't have any issues - C++17 is being used. However, I currently only have LLD as an additional project enabled, and don't build compiler-rt. Reposit

[PATCH] D123682: [clang-tblgen] Automatically document options values

jhenderson added a comment. In D123682#3455793 , @aaron.ballman wrote: > In D123682#3454627 , > @serge-sans-paille wrote: > >> @aaron.ballman Any thoughs on the above suggestion? I'd be happy to adopt >> any of

[PATCH] D123957: Update the developer policy to mention release notes

jhenderson accepted this revision. jhenderson added a comment. LGTM, with suggested nits. Comment at: llvm/docs/DeveloperPolicy.rst:188 +notes, typically found in ``docs/ReleaseNotes.rst`` for the project. Changes to +a project that are user-facing or users may wish to know abo

[PATCH] D123538: [symbolizer] Parse DW_TAG_variable DIs to show line info for globals

jhenderson added inline comments. Comment at: llvm/test/tools/llvm-symbolizer/data-location.yaml:46 +## symbolized correctly. In future (if D123534 gets merged), this can be updated +## to include a check that llvm-symbolize can also symbolize constant strings, +## like `const c

[PATCH] D130161: Added command-line options to exclude functions from function instrumentation output.

jhenderson resigned from this revision. jhenderson added a comment. I'm not sure why I've been added as a reviewer, as I don't develop or review things within clang... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130161/new/ https://reviews.llvm.

[PATCH] D130161: Added command-line options to exclude functions from function instrumentation output.

jhenderson added a comment. In D130161#3665527 , @jlegare wrote: > @jhenderson Apologies, I'm new to the process here. Usually, you should look at who has recently modified the touched files, or reviewed those same files, and add them as reviewers. Ofte

[PATCH] D123538: [symbolizer] Parse DW_TAG_variable DIs to show line info for globals

jhenderson added a comment. Okay, nothing else from me, but @dblaikie or another debuginfo person should review it too. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123538/new/ https://reviews.llvm.org/D123538 ___

[PATCH] D125604: [FileCheck] Catch missspelled directives.

jhenderson added a comment. Code change looks good to me. Are the TODO cases where the test fails if changing them? Comment at: llvm/test/FileCheck/missspelled-directive.txt:18 + +P4_COUNT-2: foo +CHECK4: error: misspelled directive 'P4_COUNT-2:' What about `P

[PATCH] D85474: Add -fbinutils-version= to gate ELF features on the specified binutils version

jhenderson added a comment. This looks reasonable to me, but I don't know the surrounding code well enough to be comfortable LGTM-ing it. Sorry! Comment at: llvm/include/llvm/MC/MCAsmInfo.h:387 + // Generated object files can only use features support by GNU ld of this + /

[PATCH] D87095: [Triple][MachO] Define "arm64e", an AArch64 subarch for Pointer Auth.

jhenderson added inline comments. Comment at: llvm/lib/Object/MachOObjectFile.cpp:2745 ArrayRef MachOObjectFile::getValidArchs() { - static const std::array validArchs = {{ + static const std::array validArchs = {{ "i386", "x86_64", "x86_64h", "armv4t", "arm","a

[PATCH] D78938: Fixing all comparisons for C++20 compilation.

jhenderson added a comment. Not that I have anything particularly against this, but won't this likely rot fairly rapidly? It's not like LLVM is even on C++17 let alone C++20 yet, so trying to make it work like the latter when it's just going to break again seems a bit like wasted effort to me.

[PATCH] D78938: Fixing all comparisons for C++20 compilation.

jhenderson added a comment. In D78938#2259342 , @BRevzin wrote: > In D78938#2258557 , @jhenderson > wrote: > >> Not that I have anything particularly against this, but won't this likely >> rot fairly rapidly? It's

[PATCH] D78938: Make LLVM build in C++20 mode

jhenderson added a comment. In D78938#2261411 , @jfb wrote: > On C++20 mode rotting: it won't if someone sets up a bot. If it rots, then > it's easier to un-rot with Barry's patch. I assume this would be a private bot? It can't be a public bot, since LLV

[PATCH] D91229: [FileCheck] Flip -allow-unused-prefixes to false by default

jhenderson added a subscriber: RKSimon. jhenderson added a comment. Maybe one way to do this is to do what @RKSimon suggests in the D90281 , and to enable it on a per-directory basis using lit.local.cfg. That would potentially require changing the "0 or 1" occur

[PATCH] D85474: Add -fbinutils-version= to gate ELF features on the specified binutils version

jhenderson added a comment. In D85474#2398858 , @MaskRay wrote: > Sent https://lists.llvm.org/pipermail/llvm-dev/2020-November/146676.html "Add > -fbinutils-version=" (cross posted to cfe-dev) so that more folks can notice > it. > > About "generate code

[PATCH] D88916: [AMDGPU] Add gfx602, gfx705, gfx805 targets

jhenderson added inline comments. Comment at: clang/lib/Basic/Cuda.cpp:1 #include "clang/Basic/Cuda.h" (aside - this file seems to be missing the copyright header - probably should be fixed separately though) Comment at: llvm/tools/llvm-rea

[PATCH] D103125: [Clang][WIP] Allow renaming of "clang"

jhenderson added a comment. In D103125#2780936 , @dblaikie wrote: > Can't say I'm super enthusiastic about this (I assume the build already > supports prefixes and suffixes, which I'd hope would be adequate - but > presumably are not for your use case),

[PATCH] D95246: [SystemZ][z/OS] Fix No such file or directory expression error matching in lit tests - continued

jhenderson added a subscriber: grimar. jhenderson added a comment. Sorry, could you revert this please. I don't think this is a good fix, as you've reduced coverage within the test, and some changes are completly redundant/add extra noise. I've commented inline with examples. Skimming D94239

[PATCH] D95246: [SystemZ][z/OS] Fix No such file or directory expression error matching in lit tests - continued

jhenderson added a comment. In D95246#2519642 , @abhina.sreeskantharajan wrote: > In D95246#2518989 , @jhenderson > wrote: > >> Sorry, could you revert this please. I don't think this is a good fix, as >> you've

[PATCH] D85474: Add -fbinutils-version= to gate ELF features on the specified binutils version

jhenderson accepted this revision. jhenderson added a comment. This revision is now accepted and ready to land. Basically LGTM - (I'm happy for this to land either with or without my comments being addressed). Would be nice to get a second reviewer to confirm they're happy. C

[PATCH] D95246: [SystemZ][z/OS] Fix No such file or directory expression error matching in lit tests - continued

jhenderson added a comment. I did briefly consider one alternative, which was to build these directly into FileCheck, but it doesn't quite feel like the right approach for FileCheck to need to know system level details. In practice, I think the lit substitution may be the best way forward overa

[PATCH] D95246: [SystemZ][z/OS] Fix No such file or directory expression error matching in lit tests - continued

jhenderson added a comment. In D95246#2522913 , @abhina.sreeskantharajan wrote: > Please let me know if there are other guides I will need to update that I'm > not aware of. There's also the lit CommandGuide located at `llvm/docs/CommandGuide/lit.rst`.

[PATCH] D95246: [SystemZ][z/OS] Fix No such file or directory expression error matching in lit tests - continued

jhenderson added inline comments. Comment at: clang/test/Driver/clang-offload-bundler.c:74 +// RUN: not clang-offload-bundler -type=i -targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu -inputs=%t.i,%t.tgt1,%t.tgt2.notexist -outputs=%t.

[PATCH] D95246: [SystemZ][z/OS] Fix No such file or directory expression error matching in lit tests - continued

jhenderson added a comment. One nit, but otherwise looks good to me, thanks! Please go ahead with the other test updates. Do you plan on doing them in this patch? Comment at: llvm/docs/TestingGuide.rst:545 + + The following error codes are supported: ENOENT, EISDIR. + --

[PATCH] D95246: [SystemZ][z/OS] Fix No such file or directory expression error matching in lit tests - continued

jhenderson added a comment. In D95246#2527999 , @abhina.sreeskantharajan wrote: > In D95246#2527961 , @jhenderson > wrote: > >> One nit, but otherwise looks good to me, thanks! Please go ahead with the >> other t

[PATCH] D95246: [test] Use host platform specific error message substitution in lit tests

jhenderson accepted this revision. jhenderson added a comment. This revision is now accepted and ready to land. Great work! LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95246/new/ https://reviews.llvm.org/D95246

[PATCH] D95808: [test] Use host platform specific error message substitution in lit tests - continued

jhenderson added a comment. I think you need to add the new values to the list of recognised substitutions in the documentation? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95808/new/ https://reviews.llvm.org/D95808

[PATCH] D95849: [FileCheck] Default --allow-unused-prefixes to false

jhenderson accepted this revision. jhenderson added a comment. This revision is now accepted and ready to land. I assume this now runs cleanly. If so, LGTM. Probably worth mentioning on the mailing list thread too, so wrap up that conversation. By the way, did we ever figure out how many of the

[PATCH] D95808: [test] Use host platform specific error message substitution in lit tests - continued

jhenderson accepted this revision. jhenderson added a comment. This revision is now accepted and ready to land. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95808/new/ https://reviews.llvm.org/D95808

[PATCH] D95246: [test] Use host platform specific error message substitution in lit tests

jhenderson added a comment. In D95246#2565351 , @abhina.sreeskantharajan wrote: > Do you know what is different between your environments which is causing the > spelling to be capitalized? This might help me determine how to fix the > current host platf

[PATCH] D95246: [test] Use host platform specific error message substitution in lit tests

jhenderson added a comment. @ASDenysPetrov, could you provide your link command-line for one of the tools that produces the failing message, please, e.g. llvm-ar? That may give us a hint about whether different libraries are being used. It's possible we need some additional cmake-time configura

[PATCH] D95246: [test] Use host platform specific error message substitution in lit tests

jhenderson added a comment. In D95246#2568084 , @ASDenysPetrov wrote: > @jhenderson > >> @ASDenysPetrov, could you provide your link command-line for one of the >> tools that produces the failing message, please, e.g. llvm-ar? > > Here is output of runni

[PATCH] D97138: [Driver] replace argc_ with argc

jhenderson resigned from this revision. jhenderson added a comment. In general, I don't work on the clang side. As such, I don't know the norms surrounding this sort of change in this area, and don't feel comfortable reviewing. You should look at getting reviewers who do work in that part of the

[PATCH] D95246: [test] Use host platform specific error message substitution in lit tests

jhenderson added a comment. @ASDenysPetrov, if you run "ninja -v llvm-ar", what is the output of the line that actually builds the llvm-ar executable? That'll tell us which libraries are in use, which should hopefully point to the difference to our own environments. For example, the last line o

[PATCH] D95246: [test] Use host platform specific error message substitution in lit tests

jhenderson added a comment. In D95246#2587123 , @ASDenysPetrov wrote: >> This shows, for example, that the `libz.so` my build uses is located in >> `/usr/lib/x86_64-linux-gnu`. By having your equivalent path, we can >> hopefully confirm that the issue i

[PATCH] D95246: [test] Use host platform specific error message substitution in lit tests

jhenderson added a comment. In D95246#2587167 , @ASDenysPetrov wrote: > @jhenderson > >> Thanks (for clarity, `libz` isn't what I'm looking for, it was just an >> example of how this information might be useful). As it is, I think we might >> need more

[PATCH] D95246: [test] Use host platform specific error message substitution in lit tests

jhenderson added a comment. In D95246#2587277 , @ASDenysPetrov wrote: > @jhenderson > >> Sorry, but `-W --trace` is not the same as `-Wl,--trace`. The former is a >> pair of options used by the compiler (one of which describes the files used >> by the c

[PATCH] D95246: [test] Use host platform specific error message substitution in lit tests

jhenderson added a comment. In D95246#2587410 , @ASDenysPetrov wrote: > @jhenderson > I think I'm done. > Here is output: F15648076: linker_trace_output.txt > > Is it OK now? Thanks, yes that does the trick. As I exp

[PATCH] D104088: Add clang frontend flags for MIP

jhenderson added inline comments. Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:59 + StringRef(Sec.Name).startswith(".zdebug") || + Sec.Name == ".gdb_index" || Sec.Name == "__llvm_mipmap"; } This doesn't look like it belongs as part of t

[PATCH] D104804: [AMDGPU] Add gfx1035 target

jhenderson added inline comments. Comment at: llvm/test/Object/AMDGPU/elf-header-flags-mach.yaml:189 +# RUN: sed -e 's//64/' -e 's//AMDGCN_GFX1035/' %s | yaml2obj -o %t.o.AMDGCN_GFX1035 +# RUN: llvm-readobj -S --file-headers %t.o.AMDGCN_GFX1035 | FileCheck --check-prefixes=EL

[PATCH] D114639: Raise the minimum Visual Studio version to VS2019

jhenderson added inline comments. Comment at: clang/docs/UsersManual.rst:3546 -cmake -G"Visual Studio 15 2017" -T LLVM .. +cmake -G"Visual Studio 16 2019" -T LLVM .. Maybe make this VS2022 instead, to help it last longer? Comment at

[PATCH] D114639: Raise the minimum Visual Studio version to VS2019

jhenderson added a comment. No more comments from me (apart from one minor nit). This should definitely get someone with more familiarity with how these things are configured to take a look though. Comment at: clang/docs/UsersManual.rst:3546 -cmake -G"Visual Studio 15 2

[PATCH] D114639: Raise the minimum Visual Studio version to VS2019

jhenderson added inline comments. Comment at: clang/docs/UsersManual.rst:3546 -cmake -G"Visual Studio 15 2017" -T LLVM .. +cmake -G"Visual Studio 17 2022" -T LLVM .. RKSimon wrote: > aaron.ballman wrote: > > jhenderson wrote: > > > I think the missing

[PATCH] D114639: Raise the minimum Visual Studio version to VS2019

jhenderson added a comment. Re. the bots: I'd hope we'd have at least some bots using VS2019 rather than all rushing to VS2022. There's nothing worse than claiming to support a minimum version of something and then not actually supporting it...! Also internally, we are switching to a default of

[PATCH] D136796: [llvm-objdump][Offload] Use common offload extraction method

jhenderson added a comment. As a general rule, dumping tools should avoid hard errors, because usually they prevent the dumping tool from continuing to dump other useful and valid information as part of the same execution. I don't have a strong objection in this particular case, because I presu

[PATCH] D136796: [llvm-objdump][Offload] Use common offload extraction method

jhenderson added inline comments. Comment at: llvm/test/tools/llvm-objdump/Offloading/elf_dynamic.test:1 -## Check that we can dump an offloading binary directly. -# RUN: yaml2obj %S/Inputs/binary.yaml -o %t.bin -# RUN: llvm-objdump --offloading %t.bin | FileCheck %s --match-full

[PATCH] D134284: [AIX] change the clang tests with llvm-nm -Xany

jhenderson added a comment. Wouldn't it be better to change the lit config to specify the `OBJECT_MODE` environment variable on AIX? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134284/new/ https://reviews.llvm.org/D134284 __

[PATCH] D134284: [AIX] change the clang tests with llvm-nm -Xany

jhenderson added a comment. In D134284#3802766 , @DiggerLin wrote: > In D134284#3802763 , @jhenderson > wrote: > >> Wouldn't it be better to change the lit config to specify the `OBJECT_MODE` >> environment vari

[PATCH] D134284: [AIX] change the clang tests with llvm-nm -Xany

jhenderson added a subscriber: jasonliu. jhenderson added a comment. In D134284#3802793 , @DiggerLin wrote: > In D134284#3802791 , @jhenderson > wrote: > >> In D134284#3802766

[PATCH] D134284: [AIX] change "llvm-nm" to "env OBJECT_MODE=any llvm-nm" in clang/test for AIX OS

jhenderson accepted this revision. jhenderson added a comment. LGTM, with nit. Comment at: clang/test/lit.cfg.py:287 + +# llvm-nm tools support an environment variable "OBJECT_MODE" on AIX OS, which +# controls the kind of objects they will support. If there is no "OBJECT_MODE"

[PATCH] D135067: [lit] RUN commands without stdin.

jhenderson added inline comments. Comment at: llvm/utils/lit/tests/Inputs/shtest-stdin/print-stdin.py:6 +print(sys.stdin.read()) \ No newline at end of file Nit: add newline at EOF. Comment at: llvm/utils/lit/tests/shtest-stdin.py:28 +# CHECK-

[PATCH] D136796: [llvm-objdump][Offload] Use common offload extraction method

jhenderson added a comment. In D136796#3903393 , @jhuber6 wrote: > Is this good to land now? The LLVM community practice is to wait a week between pings unless there's something urgent (and if something is urgent, please say so). Aside from the clang b

[PATCH] D137338: Fix dupe word typos

jhenderson added a comment. Herald added a reviewer: jdoerfert. Herald added subscribers: Michael137, sstefan1, JDevlieghere. This is going to be impossible to cleanly review as-is. Could it be broken into lots of smaller chunks? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D142660: [AIX] supporting -X options for llvm-ranlib in AIX OS

jhenderson added a comment. My apologies, this patch fell off my review queue somehow. I still have concerns about the option-parsing logic being duplicated, but I'm out of time to review it now. I'll try to look tomorrow. Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.

[PATCH] D142660: [AIX] supporting -X options for llvm-ranlib in AIX OS

jhenderson added inline comments. Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:3 +## Test the -X option. +## The option specifies the type of object file on which llvm-ranlib will operate. + Nit: trailing whitespace Comment at: l

[PATCH] D142660: [AIX] supporting -X options for llvm-ranlib in AIX OS

jhenderson added inline comments. Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:17-18 +## Test the OBJECT_MODE environment variable when adding symbol table. +# RUN: unset OBJECT_MODE +# RUN: llvm-ranlib t_X32.a +# RUN: llvm-nm --print-armap t_X32.a 2>&1 | FileCheck -

[PATCH] D142660: [AIX] supporting -X options for llvm-ranlib in AIX OS

jhenderson added inline comments. Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:68 + +## Test the invalid -X option and OBJECT_MODE enviornment var. +# RUN: not env OBJECT_MODE= llvm-ranlib t_X32.a 2>&1 | FileCheck --implicit-check-not="error:" --check-prefixes=INVA

[PATCH] D142660: [AIX] supporting -X options for llvm-ranlib in AIX OS

jhenderson added a comment. In D142660#4535693 , @stephenpeckham wrote: > I don't see any reason to check the OBJECT_MODE environment variable if the > -X flag is used. What would the error be: "You specified a valid -X flag, > but by the way, OBJECT

[PATCH] D142660: [AIX] supporting -X options for llvm-ranlib in AIX OS

jhenderson accepted this revision. jhenderson added a comment. I'm going to accept this change, although I still have significant concerns about how the whole parsing logic seems more complicated than it needs to be. Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:74 + << "

[PATCH] D142660: [AIX] supporting -X options for llvm-ranlib in AIX OS

jhenderson added a comment. In D142660#4538936 , @DiggerLin wrote: >> As an alternative (but I think adds unnecessary complexity, due to needing >> an extra variable), you could have both tools read the environment variable >> into a string variable, th

[PATCH] D142660: [AIX] supporting -X options for llvm-ranlib in AIX OS

jhenderson added a comment. In D142660#4541406 , @jhenderson wrote: > In D142660#4538936 , @DiggerLin > wrote: > >>> As an alternative (but I think adds unnecessary complexity, due to needing >>> an extra varia

[PATCH] D142660: [AIX] supporting -X options for llvm-ranlib in AIX OS

jhenderson added a comment. clang-format is complaining in the pre-merge CI. Comment at: clang/test/lit.cfg.py:391 if "system-aix" in config.available_features: -config.substitutions.append(("llvm-nm", "env OBJECT_MODE=any llvm-nm")) -config.substitutions.append(("llvm

[PATCH] D76572: Replace `T(x)` with `reinterpret_cast(x)` everywhere it means reinterpret_cast. No functional change

jhenderson added a comment. @Quuxplusone - did you see that there are several clang-format warnings reported against these changes? Please could you fix them. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76572/new/ https://reviews.llvm.org/D76572

[PATCH] D76572: Replace `T(x)` with `reinterpret_cast(x)` everywhere it means reinterpret_cast. No functional change

jhenderson accepted this revision. jhenderson added a comment. LGTM, with nit, with regards to the llvm-readobj and libObject parts. I haven't looked at the other bits though. Comment at: llvm/lib/CodeGen/AsmPrinter/OcamlGCPrinter.cpp:150 + ">= 65536.\n

[PATCH] D76572: Replace `T(x)` with `reinterpret_cast(x)` everywhere it means reinterpret_cast. No functional change

jhenderson added a comment. In D76572#1942024 , @Quuxplusone wrote: > Btw, @jhenderson and @rsmith, if you wanted to land your own files' pieces of > this patch while waiting on whoever-it-is to approve the pieces in other > files, that'd be cool. As I m

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

jhenderson added a comment. Are there any instances where we DON'T want to get the real file system? If not, could the `*llvm::vfs::getRealFileSystem()` call be put inside `cl::ExpandResponseFiles`? Comment at: llvm/include/llvm/Support/CommandLine.h:32 #include "llvm/Suppor

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

jhenderson added a comment. In D70769#1761428 , @lh123 wrote: > In D70769#1761394 , @jhenderson > wrote: > > > Are there any instances where we DON'T want to get the real file system? If > > not, could the `*llvm:

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

jhenderson added a comment. In D70769#1761517 , @sammccall wrote: > It's not just unit-tests, in D70222 we will > ultimately use FSes other than getRealFilesystem() in clangd. I see, thank you. > Having non-VFS-clean v

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

jhenderson added a comment. Aside from one nit, this looks okay to me. I'd like someone else to look at it though too. I'm not at all familiar with the file system stuff to be confident to say everything there is good. Comment at: llvm/lib/Support/CommandLine.cpp:1148 +

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

jhenderson added inline comments. Comment at: llvm/lib/Support/CommandLine.cpp:1148 + llvm::ErrorOr RHS = FS.status(RFile.File); + if (!LHS || !RHS) { +return false; kadircet wrote: > again you need to `consumeError`s before returning. I would f

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

jhenderson added inline comments. Comment at: llvm/lib/Support/CommandLine.cpp:1071 + return llvm::make_error( + "Could not convert UTF16 To UTF8", llvm::inconvertibleErrorCode()); Str = StringRef(UTF8Buf); Not sure, but you might want to use `

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

jhenderson added inline comments. Comment at: llvm/lib/Support/CommandLine.cpp:1069 + return llvm::createStringError( + std::make_error_code(std::errc::illegal_byte_sequence), + "Could not convert UTF16 To UTF8"); `std::make_error_code(std:

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

jhenderson added inline comments. Comment at: llvm/lib/Support/CommandLine.cpp:1069 + return llvm::createStringError( + std::make_error_code(std::errc::illegal_byte_sequence), + "Could not convert UTF16 To UTF8"); jhenderson wrote: > `std::

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

jhenderson added inline comments. Comment at: llvm/lib/Support/CommandLine.cpp:1069 + return llvm::createStringError( + std::make_error_code(std::errc::illegal_byte_sequence), + "Could not convert UTF16 To UTF8"); lh123 wrote: > jhenderson

  1   2   >