[PATCH] D36728: Switch to consumeError(), since this can crash otherwise.

lhames added a comment. The preferred solution to this is actually to wrap the call with cantFail (See http://llvm.org/docs/ProgrammersManual.html#using-cantfail-to-simplify-safe-callsites) -- it will handle both the assertion and consumption of the value for you, and will simplify calls that

[PATCH] D36728: Switch to consumeError(), since this can crash otherwise.

lhames added a comment. > Is it ok to drop the assertion in that case (and convert it to a comment)? I > didn't want to alter too much of this check, since perhaps the original > author(s) were more skeptical about this breaking (hence the assertion). > Something like: > > // Replacements

[PATCH] D35103: Expand clang-interpreter with example of throwing in and from the JIT for Windows64.

lhames accepted this revision. lhames added a comment. This revision is now accepted and ready to land. Otherwise this looks good to me. Comment at: examples/clang-interpreter/CMakeLists.txt:73 +set_property(TARGET ${TARGET} PROPERTY COMPILE_FLAGS ${editedFlags}) +

[PATCH] D36429: [clang-import-test] Option to dump the IR for an expression

lhames accepted this revision. lhames added a comment. This revision is now accepted and ready to land. LGTM! Comment at: tools/clang-import-test/clang-import-test.cpp:301-303 + if (ShouldDumpIR) { +CG.GetModule()->print(llvm::outs(), nullptr); + } LLVM

[PATCH] D36806: Switch to cantFail(), since it does the same assertion.

lhames added a comment. In https://reviews.llvm.org/D36806#848246, @hintonda wrote: > It's just too bad llvm::cantFail() doesn't take an optional string. > But the code is cleaner, so I won't comment further. That's not a bad idea actually. Let me add an optional error message to cantFail

[PATCH] D36806: Switch to cantFail(), since it does the same assertion.

lhames added a comment. I've added an optional ErrMsg argument to cantFail in r312066 - you can now use this to preserve your explanatory text. https://reviews.llvm.org/D36806 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D45897: Convert clang-interpreter to ORC JIT API

lhames accepted this revision. lhames added a comment. This revision is now accepted and ready to land. Herald added a subscriber: mgrang. Looks good to me. :) Repository: rC Clang https://reviews.llvm.org/D45897 ___ cfe-commits mailing list

[PATCH] D36806: Switch to cantFail(), since it does the same assertion.

lhames accepted this revision. lhames added a comment. This revision is now accepted and ready to land. Looks like this was LGTM'd but never applied. Stephen -- do you have commit access? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D36806/new/ https://reviews.llvm.org/D36806

[PATCH] D63518: BitStream reader: propagate errors

lhames added a comment. I haven't had a chance to audit the whole patch yet, but in general the error suppression idioms are unsafe (though maybe no more so than the existing code?). I would be inclined to audit all those FIXMEs and replace them with cantFails or consumeErrors. consumeError

[PATCH] D66705: FileManager: Use llvm::Expected in new getFileRef API

lhames added a comment. In D66705#1644166 , @dexonsmith wrote: > Updated patch after running `check-clang` and learning more about `Expected`. > I've added a parallel API using `Optional` for clients that > don't want to do anything with the error. > >

[PATCH] D65829: [clang][DirectoryWatcher][NFC] Swapping asserts for llvm fatal_error in ::create.

lhames added a comment. I think the right line is: logAllUnhandledErrors(DW.takeError(), errs(), ""); It's a bit wordy, but will log a sensible error and works with and without ENABLE_ABI_BREAKING_CHECKS enabled. I'm not against improving the output for cantFail, nor totally against abusing

[PATCH] D65853: Use ASSERT_THAT_ERROR instead of logAllUnhandledErrors/exit

lhames accepted this revision. lhames 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/D65853/new/ https://reviews.llvm.org/D65853

[PATCH] D71397: [clang] Improve LLVM-style RTTI support in ExternalASTSource/ExternalSemaSource

lhames added a comment. In D71397#1784325 , @teemperor wrote: > I would prefer landing this as-is because it is just doing the same we do in > other places in LLVM. Afterwards migrating the whole RTTI system in LLVM to a > new system (whether that will

[PATCH] D71397: [clang] Improve LLVM-style RTTI support in ExternalASTSource/ExternalSemaSource

lhames added a comment. Side note: This https://reviews.llvm.org/D39111 seems related. The patch has languished as I have been busy with other work, but if it would be useful for you guys I'd be happy to try to land it. Repository: rC Clang CHANGES SINCE LAST ACTION

[PATCH] D69356: [NFC] Rename LLVM_NO_DEAD_STRIP

lhames added subscribers: beanz, lhames. lhames added a comment. In D69356#1726074 , @beanz wrote: > ... It seems to me that maybe a more appropriate approach is that > `LLVM_SUPPORT_PLUGINS` implies `LLVM_NO_DEAD_STRIP`, rather than conflating > the

[PATCH] D103855: [clang] Exclude function pointers on DefaultedComparisonAnalyzer

lhames added a comment. Disregard 80f30a6b855b : I messed up a copy-paste of a commit message. That was for https://reviews.llvm.org/D104480. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

lhames added a comment. In D96033#2757222 , @hubert.reinterpretcast wrote: > ... > I have a local build I can apply a patch to. Hi Hubert, Could you apply the following patch and let me know the output from the failing test? I'm trying to work out

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

lhames added a comment. In D96033#2757930 , @hubert.reinterpretcast wrote: > ... > Command Output (stderr): > > triple: powerpc64-ibm-aix7.2.0.0 > datalayout: E-m:a-i64:64-n32:64-S128-v256:256:256-v512:512:512 > error: Added modules have incompatible

[PATCH] D102756: [clang-repl] Tell the LLJIT the exact target triple we use.

lhames accepted this revision. lhames added a comment. This revision is now accepted and ready to land. LGTM. :) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102756/new/ https://reviews.llvm.org/D102756 ___

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

lhames added a comment. > The other aspect of this is that upon unloading of these pieces of code we > need to run the destructors (that's why we need some non-canonical handling > of when we run the atexit handlers). I just noticed this comment. I think long term you could handle this by

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

lhames added inline comments. Comment at: clang/lib/CodeGen/CodeGenAction.cpp:908 +CodeGenerator *CodeGenAction::getCodeGenerator() const { + return BEConsumer->getCodeGenerator(); v.g.vassilev wrote: > sgraenitz wrote: > > v.g.vassilev wrote: > > >

[PATCH] D105014: added some example code for llvm::Expected

lhames added a comment. I missed this thread earlier -- thanks to Dave for pointing me to it. @kuhnel -- Thanks very much for working on this. Out of interest, did you see https://llvm.org/docs/ProgrammersManual.html#error-handling ? If not (and if you find it helpful) then maybe we need to

[PATCH] D107049: [clang-repl] Re-implement clang-interpreter as a test case.

lhames added a comment. LGTM, but a clang dev should probably check this out too. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107049/new/ https://reviews.llvm.org/D107049 ___ cfe-commits mailing list

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

lhames added a comment. In D111863#3074404 , @housel wrote: > To be clear, this new code parses exactly as much of each FDE as the existing > `__register_frame`/`__unw_add_dynamic_fde` does, including doing the same > work to compute the record length.

[PATCH] D112229: Reapply [ORC-RT] Configure the ORC runtime for more architectures and platforms

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

[PATCH] D111273: [clang-format-diff] Fix missing formatting for zero length git diff lines

lhames added a comment. Heads up -- I think I just hit an error due to this while formatting a local commit: % ./clang/tools/clang-format/git-clang-format HEAD~1 Assertion failed: (Line &&

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

lhames added a comment. In D111863#3071353 , @joerg wrote: > I would strongly prefer if ORCv2 doesn't depend on this. It essentially > forces libunwind to parse the whole section just to find the delimiters of > the FDEs. That's a lot of unnecessary

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

lhames added a comment. In D111863#3072827 , @joerg wrote: > `__register_frame` requires parsing the CIE header, but not the whole FDE > program. E.g. that's the `findPCRange` logic. After that, the FDE is just > added to the internal block list.

[PATCH] D112056: [clang-format] git-clang-format throws an assertion when removing files as part of the commit

lhames added a comment. I think that this should fix the issue I saw, but `clang-format` will still crash when passed a `-lines=0:X` option. Would it make sense to either error out in that tool, or teach the tool to ignore `-lines=0:0` options if they're guaranteed to be no-ops for formatting?

[PATCH] D111454: Move TargetRegistry.(h|cpp) from Support to MC

lhames added a comment. Yeah -- this seems like a good idea to me. Thanks Reid! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111454/new/ https://reviews.llvm.org/D111454 ___ cfe-commits mailing list

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

lhames accepted this revision. lhames added a comment. This looks good to me, but we should get a libunwind contributor to weigh in too. I've been trying to think of a good way to test this, but it's awkward. The best strategy that I've come up with, at least for testing within libunwind

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

lhames added a comment. There doesn't seem to be any objection to this approach, so I think you're free to land @housel. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111863/new/ https://reviews.llvm.org/D111863

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

lhames accepted this revision. lhames added a comment. @joerg @dim @emaste Any further comments on this? I'd like @housel to be able to land this this week if possible. It will significantly improve the usability of libunwind for JIT clients by giving us an API with behavior that is consistent

[PATCH] D107049: [clang-repl] Re-implement clang-interpreter as a test case.

lhames added a comment. In D107049#3096727 , @uabelho wrote: > Hi, > > We're seeing a problem with this patch in our downstream (not public) > buildbots. With an asan-built compiler we see the following: > > ... > 10:08:55 [ RUN ]

[PATCH] D107049: [clang-repl] Re-implement clang-interpreter as a test case.

lhames added a comment. In D107049#3101456 , @dblaikie wrote: > In D107049#3100630 , @rnk wrote: > >> So, to back up a bit, do I understand correctly that this change adds tests >> to the check-clang test suite

[PATCH] D107049: [clang-repl] Re-implement clang-interpreter as a test case.

lhames added a comment. In D107049#3104558 , @dblaikie wrote: > In D107049#3103984 , @lhames wrote: > >> In D107049#3101456 , @dblaikie >> wrote: >> >>> Yeah, seems

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

lhames added a comment. @joerg Any further thoughts on this? I'm happy for it to go in as-is -- we can always make the implementation of `__unw_add_dynamic_eh_frame_section` / `__unw_remove_dynamic_eh_frame_section` later, but the API looks good to me, and I think this is a strict improvement

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

lhames added a comment. @joerg, @emaste -- I finally got a chance to experiment with a Linux docker container and confirmed that libgcc_s's `__register_frame` can handle individual frames. Unfortunately that does not help us on FreeBSD. If we ignore FreeBSD for a moment we could imagine switch

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

lhames added a comment. In D111863#3071356 , @steven_wu wrote: > In D111863#3071328 , @housel wrote: > >> In D111863#3069279 , @lhames wrote: >> >>> I think the ORC

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

lhames added a comment. I've chatted about this with Joerg on a side thread and wanted to summarize my understanding of the situation. The problem is that we have incompatible behaviors for `__register_frame` and `__deregister_frame` between unwinders: - libgcc_s and the FreeBSD port of

[PATCH] D122258: [MC][RFC] Omit DWARF unwind info if compact unwind is present for all archs

lhames added a comment. > I'm currently doing that via MCContext::getGenDwarfForAssembly... This isn't my wheelhouse, but at first glance GenDwarfForAssembly looks like it might control all Dwarf sections (including debug ones). If that's the case you might need to introduce something eh-frame

[PATCH] D118087: Fix running orc-rt tests with LLVM_BUILD_EXTERNAL_COMPILER_RT

lhames accepted this revision. lhames added a comment. This revision is now accepted and ready to land. LGTM! Does check-orc-rt need to be added to `COMPILER_RT_TEST_SUITES` too? I guess not, otherwise we wouldn't have seen this missing dependency in the first place? Repository: rG LLVM

[PATCH] D159115: [clang-repl] Adapt to the recent dylib-related changes in ORC.

lhames accepted this revision. lhames added a comment. This revision is now accepted and ready to land. Otherwise LGTM! Comment at: clang/lib/Interpreter/IncrementalExecutor.cpp:96-100 + JITDylibSearchOrder O; + JITDylibLookupFlags Flags =

[PATCH] D159167: [clang-repl][Orc] Export executable symbols in ClangReplInterpreterExceptionTests

lhames added a comment. @vitalybuka If we want to disable this test for now, what's the canonical way to do it? Should we just wrap the whole `add_clang_unittest` block in the conditional? # export_executable_symbols triggers Lsan report.

[PATCH] D159167: [clang-repl][Orc] Export executable symbols in ClangReplInterpreterExceptionTests

lhames added a comment. lsan disabled in 47625fea5e37 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159167/new/ https://reviews.llvm.org/D159167

[PATCH] D159167: [clang-repl][Orc] Export executable symbols in ClangReplInterpreterExceptionTests

lhames added a comment. Ok -- I'll try disabling `lsan` for now. It'll be good to test this path (even with a leak), and it'll make reproduction extra simple when someone has cycles to look into the leak. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D122258: [MC] Omit DWARF unwind info if compact unwind is present for all archs

lhames added inline comments. Comment at: llvm/tools/llc/llc.cpp:697-700 +auto = MMIWP->getMMI().getContext(); +Ctx.setGenDwarfForAssembly(Target->Options.ForceDwarfFrameSection); const_cast(LLVMTM.getObjFileLowering()) +->Initialize(Ctx, *Target);

[PATCH] D144447: [Clang] Teach buildFMulAdd to peek through fneg to find fmul.

lhames added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3738 - assert(!(negMul && negAdd) && "Only one of negMul and negAdd should be set."); - Value *MulOp0 = MulOp->getOperand(0); craig.topper wrote: > kpn wrote: > > If I'm reading

[PATCH] D148481: [clang-repl] Enable debugging of JIT-ed code.

lhames accepted this revision. lhames added a comment. This revision is now accepted and ready to land. Looks good to me! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148481/new/ https://reviews.llvm.org/D148481 ___ cfe-commits mailing list

[PATCH] D141215: [clang-repl] Introduce Value to capture expression results

lhames added a comment. > OTOH it's a challenge to review since the patch is really big. Is there no > way to break it up and test in isolation? Or strip away details that could be > added in iterations? I had a short look and some comments, but I won't find > the time in the coming weeks to

[PATCH] D150549: Move SubtargetFeature.h from MC to TargetParser

lhames added a comment. FWIW I'm in favor of this: as @jobnoorman mentioned it'd eliminate JITLink's library dependence on MC. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150549/new/ https://reviews.llvm.org/D150549