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

jhenderson added a comment. In D70769#1763703 , @kadircet wrote: > @jhenderson I am planning to commit this if the discussion around `std::errc` > vs `llvm::errc` is resolved, I don't have any preference towards one or the > other both seems to get the w

[PATCH] D74433: [llvm-objdump] Print file format in lowercase to match GNU output.

jhenderson added a comment. I don't really have a strong opinion either way. By strict style rules, "ELF" is an acronym, so should be all upper-case. In regular text, I'd expect it to be ELF everywhere too. On the other hand, as pointed out, "elf" is the bfd naming style. For WASM, I recall th

[PATCH] D72820: [FPEnv] Add pragma FP_CONTRACT support under strict FP.

jhenderson added inline comments. Comment at: llvm/docs/LangRef.rst:16145 +'``llvm.experimental.constrained.fmuladd``' Intrinsic +^^^ + This underline isn't long enough and is breaking the sphinx build bot. Please

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

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

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

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

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

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

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

[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] 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 {32|6

[PATCH] D29027: [Stack Protection] Add remark for reasons why Stack Protection has been applied

jhenderson created this revision. Herald added a subscriber: fhahn. Depends on https://reviews.llvm.org/D29023, which is currently under review. In that change, I am adding diagnostic information to LLVM for why Stack Smash Protection has been applied to each function. This is the second stage,

[PATCH] D29027: [Stack Protection] Add remark for reasons why Stack Protection has been applied

jhenderson updated this revision to Diff 85570. jhenderson edited the summary of this revision. jhenderson added a comment. Thanks for the comments. I have updated the diff accordingly, by removing the unknown case, adding a check to distinguish between the function attribute and command-line sw

[PATCH] D29027: [Stack Protection] Add remark for reasons why Stack Protection has been applied

jhenderson added a comment. Ping https://reviews.llvm.org/D29027 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29027: [Stack Protection] Add remark for reasons why Stack Protection has been applied

jhenderson added a comment. Ping! https://reviews.llvm.org/D29027 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29027: [Stack Protection] Add remark for reasons why Stack Protection has been applied

jhenderson abandoned this revision. jhenderson added a comment. Abandoning. Due to changes made in https://reviews.llvm.org/D29023, there is no more need for this patch. The new method followed in that patch means that clang gets support for the new remarks via -Rpass=stack-protector. https://

[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 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 llvm-install-nam

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

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

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

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

[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] 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, + /*RequiresNullTer

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

<    1   2