[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. 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")) -

[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

[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,

[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,

[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:"

[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:3 +## Test the -X option. +## The option specifies the type of object file on which llvm-ranlib will operate. + Nit: trailing whitespace Comment at:

[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:

[PATCH] D153652: [Support] Don't set "all_exe" mode by default for file written by llvm::writeToOutput

jhenderson accepted this revision. jhenderson added a comment. This revision is now accepted and ready to land. The new test LGTM, albeit with one query: I assume `umask` sets some global state. When the lit unit tests are running, do the tests within the same process run sequentially, or are

[PATCH] D153652: [Support] Don't set "all_exe" mode by default for file written by llvm::writeToOutput

jhenderson added inline comments. Comment at: llvm/unittests/Support/raw_ostream_test.cpp:500 + ASSERT_TRUE(!!Perms); + EXPECT_EQ(0, *Perms & llvm::sys::fs::all_exe); + hokein wrote: > jhenderson wrote: > > Here and below, rather than just checking the all_exe

[PATCH] D153652: [Support] Don't set "all_exe" mode by default for file written by llvm::writeToOutput

jhenderson added a comment. The updated unit test is failing on Windows in the pre-merge checks. Please investigate and fix as appropriate. Comment at: llvm/unittests/Support/raw_ostream_test.cpp:500 + ASSERT_TRUE(!!Perms); + EXPECT_EQ(0, *Perms & llvm::sys::fs::all_exe); +

[PATCH] D153652: [Support] Don't set "all_exe" mode by default for file written by llvm::writeToOutput

jhenderson added inline comments. Comment at: llvm/test/tools/llvm-dwarfutil/ELF/X86/file-permissions.test:1 +# RUN: yaml2obj %p/Inputs/common.yaml -o %t.o +# RUN: chmod a+x %t.o hokein wrote: > jhenderson wrote: > > It might make sense to reuse the test code

[PATCH] D153652: [llvm][Support] Don'tt set "all_exe" mode by default for file written by llvm::writeToOutput.

jhenderson added a comment. Is there anything that can be done to use gtest unit tests for this? The two lit tests are useful, but the problem with them is that if they switch to using a different approach to emitting their data, the lit tests won't cover the Support code you're actually

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

jhenderson added a comment. I haven't looked again at the rest of this patch. I'll do so hopefully in the next couple of weeks. Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:80 + << " -U- Use actual timestamps and uids/gids\n" + << " -X

[PATCH] D149119: [CMake] Use LLVM own tools in extract_symbols.py

jhenderson added a comment. In D149119#4331207 , @ikudrin wrote: > In D149119#4329274 , @tmatheson > wrote: > >> LGTM, thank you for doing this. Please give it a couple of days in case >> others have comments.

[PATCH] D149119: [CMake] Use LLVM own tools in extract_symbols.py

jhenderson added a comment. I've not really looked into this patch significantly, so this may well be addressed in the patch, given I see you have modified stuff to do with the NATIVE build, but in the past I have seen LLVM using its own tools to build other parts of its system. I believe it

[PATCH] D144638: [lit] Detect Inconsistent File Access Times

jhenderson added a comment. Related to my inline comment, your changes will result in some tests being disabled on Windows that weren't before (at least one of the tests pass for me even on my machine where atime is disabled). I think we need to understand why these tests pass on Windows

[PATCH] D144638: [lit] Detect Inconsistent File Access Times

jhenderson added inline comments. Comment at: llvm/utils/lit/lit/llvm/config.py:171 +# in the tests that do the same thing. +(_, try_touch_err) = self.get_process_output(["touch", "-a", "-t", "199505050555.55", f.name]) +if try_touch_err !=

[PATCH] D144638: [lit] Detect Consistent File Access Times

jhenderson added a comment. This looks reasonable to me, with the caveat that I don't know a huge amount about how the different OSes access time systems work. One question though: if your antivirus was causing flakiness (as opposed to outright always-fails), won't it just move that flakiness

[PATCH] D143725: [llvm-objdump][ARM] support --symbolize-operands for ARM/ELF

jhenderson added inline comments. Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1145 + // So far only supports ARM/Thumb, PowerPC and X86. + Triple triple = STI->getTargetTriple(); + if (!triple.isPPC() && !triple.isX86() && !triple.isARM() && covanam

[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] 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

[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

[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

[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 +#

[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

[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 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

[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] 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.

[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

[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.

[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/

[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

[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 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] 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

[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] 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

[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

[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

[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

[PATCH] D120713: [clangd] Make dexp command line options sticky

jhenderson resigned from this revision. jhenderson added a comment. I don't know anything about clangd either. Not sure why I was added as a reviewer :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120713/new/ https://reviews.llvm.org/D120713

[PATCH] D116351: Update Bug report URL to Github Issues

jhenderson accepted this revision. jhenderson added a comment. LGTM, from my point of view. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116351/new/ https://reviews.llvm.org/D116351 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D116351: Update Bug report URL to Github Issues

jhenderson added inline comments. Comment at: llvm/docs/CommandGuide/llvm-install-name-tool.rst:79 -To report bugs, please visit . +To report bugs, please visit . I believe

[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

[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

[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

[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

[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

[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

[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

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

jhenderson added a comment. @ASDenysPetrov, I think you need to mark this patch as Accepted so that someone can close this review. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95808/new/ https://reviews.llvm.org/D95808

[PATCH] D99360: [OpenMP][WIP] Add standard notes for ELF offload images

jhenderson added a comment. It might make sense to do the llvm-readobj portions of this patch in a separate review, since they are somewhat independent. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99360/new/ https://reviews.llvm.org/D99360

[PATCH] D99182: [NFC] Reordering parameters in getFile and getFileOrSTDIN

jhenderson accepted this revision. jhenderson added a comment. This revision is now accepted and ready to land. LGTM, with nit. Comment at: llvm/lib/Support/MemoryBuffer.cpp:275 + return getFileAux( + Filename, /*MapSize=*/-1, 0, /*IsText=*/false, +

[PATCH] D99182: [NFC] Reordering parameters in getFile and getFileOrSTDIN

jhenderson added a comment. Overall, this seems reasonable to me. I have a slight concern that it might cause surprising failures for downstream users, that aren't picked up at build time (due to implicit conversions), but I don't think you need to worry too much about that.

[PATCH] D98278: [test] Add ability to get error messages from CMake for errc substitution

jhenderson added a comment. In D98278#2628433 , @zero9178 wrote: > I addressed your comments in > https://reviews.llvm.org/rG4a17ac0387f078529da02e355a24df99f645d364. Hope it > should be alright now Thanks! Repository: rG LLVM Github Monorepo

[PATCH] D98278: [test] Add ability to get error messages from CMake for errc substitution

jhenderson added inline comments. Comment at: llvm/cmake/modules/GetErrcMessages.cmake:3-6 +# The purpose of this function is to supply those error messages to llvm-lit using the errc_messages config +# Currently supplied and needed error codes: ENOENT, EISDIR, EINVAL and

[PATCH] D98278: [test] Add ability to get error messages from CMake for errc substitution

jhenderson added a comment. Thanks for working on this. A couple of post-commit comments to look at, otherwise looks good. Thanks for figuring out how to do this in a portable manner! Comment at: llvm/cmake/modules/GetErrcMessages.cmake:1 + +# This function returns the

[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

[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

[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#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

[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

[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

[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

[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

[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

[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] 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 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] 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] 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

[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 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

[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 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

[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.

[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] 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] 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] 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"

[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:

[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

[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?

[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] 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",

[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] 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:

[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),

[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),

[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] 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

[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

[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

[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

[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 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

[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

  1   2   >