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

[Lang Hames via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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
  https://reviews.llvm.org/D159167/new/

https://reviews.llvm.org/D159167

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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. 


 
  if (NOT LLVM_USE_SANITIZER MATCHES ".*Address.*")
  
add_clang_unittest(ClangReplInterpreterExceptionTests
  InterpreterExceptionTest.cpp
  )
  
llvm_update_compile_flags(ClangReplInterpreterExceptionTests)
target_link_libraries(ClangReplInterpreterExceptionTests PUBLIC
  clangAST
  clangBasic
  clangInterpreter
  clangFrontend
  )
add_dependencies(ClangReplInterpreterExceptionTests clang-resource-headers)
  
export_executable_symbols(ClangReplInterpreterExceptionTests)
  endif()


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159167/new/

https://reviews.llvm.org/D159167

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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 = JITDylibLookupFlags::MatchExportedSymbolsOnly;
+  O.push_back({>getMainJITDylib(), Flags});
+  O.push_back({Jit->getPlatformJITDylib().get(), Flags});
+  O.push_back({Jit->getProcessSymbolsJITDylib().get(), Flags});

I think this should be equivalent to
```
auto SO =
  makeJITDylibSearchFlags(>getMainJITDylib(),
  Jit->getPlatformJITDylib().get(),
  Jit->getProcessSymbolsJITDylib.get());
```
but this is purely cosmetic -- either way works.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159115/new/

https://reviews.llvm.org/D159115

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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 give it the attention it deserves.
>
> My biggest conceptual concern is that Value is part of the Interpreter 
> library. This is going to be a showstopper for out-of-process execution. 
> Conceptually, it should move into a runtime library (like the ORC runtime in 
> compiler-rt) and this won't get easier the more it is entangled with the 
> interpreter. I do see that this adds significant complexity and I guess it's 
> ok to keep this for later, but I do wonder what would be the best way to 
> prepare for it. Essentially, all communication with the interpreter will be 
> asynchronous. Is this something we can do already?

Similar thoughts here. In particular, is there a way to break the 
`IsSemiMissing`/ `repl_input_end` parts out from the Value part?

It sounds like it would be helpful for `clang-repl` to land this as-is, but we 
should have a plan to replace this with something compatible with 
out-of-process execution in the longer term. At a guess I think that would mean 
using regular global variable instances to manage the storage on the executor 
side, an extended `MemoryAccess` interface to read/write the value from the 
REPL side when needed (e.g. for printing), and emitting glue functions to pass 
the variable's value in to callers.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141215/new/

https://reviews.llvm.org/D141215

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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 this right it looks like the assert() wasn't needed before. 
> > Do we know why it was added in the first place?
> I don't. I've added @lhames who wrote the code originally, but's been 10 
> years.
It was probably included as a guard rail during bringup (from memory we didn't 
have any patterns in LLVM that matched both negations) and was never removed. I 
think it should be fine to remove.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D17/new/

https://reviews.llvm.org/D17

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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);

int3 wrote:
> int3 wrote:
> > lhames wrote:
> > > This option hand-off feels very manual. I wonder if we should add a 
> > > `propagateTargetOptionsToMC` function, but maybe the set of options that 
> > > it would apply to is small enough not to bother for now?
> > > 
> > > I would add something like this to `LLVMTargetMachine::addPassesToEmitMC` 
> > > too, for the the case where the caller passes in a null `MCContext*`. 
> > thanks for the pointers! I'll look into that
> actually, I'm not sure I understood your suggestion... which class do you 
> envision `propagateTargetOptionsToMC` being defined in? How would it make 
> things "less manual"?
Oh! `TargetOptions` has an `MCTargetOptions` member already. If 
`GenDwarfUnwindInfo` === `ForceDwarfUnwindInfo`, could you just sink 
`ForceDwarfUnwindInfo` into `MCTargetOptions`? 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122258/new/

https://reviews.llvm.org/D122258

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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

> Previously, omitting unnecessary DWARF unwinds was only done for
> watchOS, but I've heard that it might be possible to do this for all
> archs.

Sounds like this was for bincompat with older unwinders when compact-unwind was 
first released. I think we can turn this off by default now.

> I believe the root cause is that libunwind is unable to load compact
> unwind dynamically. (I believe this limitation applies not just to MCJIT but 
> to
> ORC and the regular interpreter as well -- @lhames, would you be able to
> confirm?)

That's right. Adding dynamic registration for compact unwind is pretty high on 
my todo list. I'm hoping to get to it within the next month or so. We should 
still have a way to force eh-frames though.




Comment at: 
clang/unittests/Interpreter/ExceptionTests/InterpreterExceptionTest.cpp:48
 
+// FIXME
 TEST(InterpreterTest, CatchException) {

I guess this is just a placeholder? The final commit should include more detail.



Comment at: llvm/lib/ExecutionEngine/RuntimeDyld/RTDyldMemoryManager.cpp:131-132
+   size_t Size) {
+  if (Size == 0)
+return;
   registerEHFramesInProcess(Addr, Size);

Is this needed? With your change, registerEHFramesInProcess should be a no-op 
if `Size == 0`.



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);

This option hand-off feels very manual. I wonder if we should add a 
`propagateTargetOptionsToMC` function, but maybe the set of options that it 
would apply to is small enough not to bother for now?

I would add something like this to `LLVMTargetMachine::addPassesToEmitMC` too, 
for the the case where the caller passes in a null `MCContext*`. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122258/new/

https://reviews.llvm.org/D122258

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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 Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118087/new/

https://reviews.llvm.org/D118087

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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 libunwind accept either null-terminated 
whole frames or single FDEs. If called with single FDEs, FreeBSD's port of 
libunwind will have quadratic cost for registration, as it always walks the 
remainder of the section. I suspect libgcc_s's behavior is similar..
- llvm-project's libunwind accepts only single FDEs.

Attempts to reliably detect the unwinder being used (so that we can call it the 
"best" way) have been flaky, leading to hard errors for clients. We're looking 
for a consistent behavior that we could rely on (at least when we can detect 
that it is available). Ideally, the proposed scheme should allow for efficient 
implementations.

The options that I have considered and discussed with others are:

1. Always registering single frames.

The problem with this is that it forces up front costs on the caller: all FDE 
starts must be identified, and we have one libcall per FDE to register (and 
probably a heap allocation embedded in that call). On top of this, two major 
unwinders (libgcc_s and the FreeBSD port of libunwind) seem like they have poor 
implementations of single-FDE processing that may have quadratic overhead in 
the worst case.

2. Teach libunwind's `_register_frame`  to also accept whole frames.

We could update libunwind's `__register_frame` implementation to also be able 
to take a whole section. For this to be useful though, we would still need some 
reliable way to detect whether this behavior is available on the libunwind 
version being used. A new special symbol would do 
(__unw_register_frame_accepts_whole_sections?), but this feels awkward compared 
to @housel's proposal for a new function with clearly defined behavior.

3. Register .eh_frame_hdr sections.

The .eh_frame_hdr section is a pre-built index of the .eh_frame section. Its 
format is documented in 
https://refspecs.linuxfoundation.org/LSB_1.3.0/gLSB/gLSB/ehframehdr.html. This 
section is not present in relocatable objects (which LLVM's JIT APIs use as 
input), but is synthesized by the static linker.

For ahead of time compilation this link-time synthesis provides a lot of value 
-- indexing the FDEs once at link times means that you never have to do it at 
launch time.

For a JIT I don't think this provides any value: Indexing of the .eh_frame will 
necessarily be happening at "launch" time. At the same time, forcing synthesis 
of an .eh_frame_hdr section for registration constrains unwinder 
implementations: they definitely can't index lazily (the caller already eagerly 
indexed for them), and if they don't want to use .eh_frame_hdr as their 
internal index data structure then the scheme has wasted two encoded pointers 
per FDE registered, plus 4 bytes and two more encoded values for the 
.eh_frame_hdr header.

4. @housel's solution -- Add a new registration function that is defined to 
take an __eh_frame start address.

This requires no up-front computation, we just pass in the start of the 
.eh_frame section. It places no constraints on the unwinder: Implementations 
are free to index eagerly or lazily as they like, and to choose whatever 
internal representation best suits them.

I think that @housel's solution is the best option, and that we should proceed 
with it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111863/new/

https://reviews.llvm.org/D111863

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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 to registering single 
frames everywhere, but this would be an unrecoverable performance regression on 
Linux -- we would necessarily have to walk all CFI entries in the section to 
use the API.

One possible alternative solution would be to update libunwind's 
__register_ehframe function to also accept whole sections the same way libgcc_s 
does, but I don't think this is a good solution: We'd be overloading the 
behavior in a surprising way to match a libgcc_s behavior that doesn't seem 
well-documented, and we'd still be left needing some other way to detect 
whether the current unwinder supported the desired semantics.

I think the ideal solution would be to always pass in whole sections, as 
@housel's proposed solution does.

I think we can debate the right API signature (e.g. should it pass the section 
range so that we really can skip walking the CFI records, or is this cheap 
enough not to bother?), function name, and implementation, but does anyone have 
any objection to the direction? If so, why?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111863/new/

https://reviews.llvm.org/D111863

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

lhames added a comment.

In D111863#3071356 , @steven_wu wrote:

> In D111863#3071328 , @housel wrote:
>
>> In D111863#3069279 , @lhames wrote:
>>
>>> I think the ORC runtime provides a much more natural way to test this. Did 
>>> you manage to come up with some ORC-runtime based tests in the end?
>>
>> My current plan is to automate what I've been doing manually, namely running 
>> a test of C++ exception handling using `llvm-jitlink`, both with the default 
>> configuration (using libgcc-provided unwinding), and with libunwind 
>> `LD_PRELOAD`ed to force it as the unwinding provider.
>
> @lhames  The other possibility is to lift JIT runtime to top level in 
> llvm-project so it can link libunwind into its runtime so you don't need to:
>
> - Create a public API in libunwind to support JIT
> - Worry about deploying a compatible libunwind together with JIT (or worry 
> about back-deploy)
> - You can write all the tests in ORCJIT context.

@steven_wu I missed this earlier, sorry!

> so it can link libunwind into its runtime

I don't follow this. Are you suggesting that the ORC runtime would link in 
portions of libunwind? I don't think that'd give us the behavior that we want 
-- we need frames to be registered with the processes copy of libunwind so that 
unwinding is consistent across JIT and AOT-compiled code.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111863/new/

https://reviews.llvm.org/D111863

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

lhames added a comment.

In D107049#3104558 , @dblaikie wrote:

> In D107049#3103984 , @lhames wrote:
>
>> In D107049#3101456 , @dblaikie 
>> wrote:
>>
>>> Yeah, seems we've had some precedent for this for a while, as @lhames says 
>>> (doing some archaeology I see one of the clang-interpreter tests added 
>>> here, for instance: 
>>> https://github.com/llvm/llvm-project/commit/1d487617f20ab9df65ab60d6cf9431ef288312ab
>>
>> Also all of the MJCIT and Orc tests in LLVM.
>
> I think @rnk was asking/drawing a distinction between execution tests in LLVM 
> (which is at least only testing LLVM code in LLVM) and execution tests in 
> Clang (which is both execution, and cross-project) - so I was just talking 
> about the Clang->LLVM->execution tests. Though I think in LLVM there'd also 
> be some opportunity to find some ways to isolate/test JIT infrastructure with 
> less execution (had sort of hoped in the early days of Orc that the layered 
> architecture would allow more isolated/unit testing to robustly test pieces 
> without execution for that testing).

Orc has plenty of isolated unit testing and regression testing too, it's just 
that the nature of the project (dynamic compilation, and with relatively few 
resources dedicated to it) makes end to end testing attractive for now too. 
Perhaps in the long term we could build out enough mocking infrastructure to be 
confident that the JIT works without running any JIT'd code, but we're a fair 
way off that.

I think testing of the clang-repl is probably in a similar place: In theory you 
could build infrastructure to test out all the interesting states that a REPL 
could put your AST into. In practice we don't have the resources to do that 
yet, and without execution tests we're left with essentially no testing of the 
feature.

Execution testing seems like the right compromise for now, as long as we can 
make sure that there's not too much noise from the testers. The lesson from the 
LLVM execution tests is that this is possible without an excessive amount of 
work.

> I think cross-project-tests (originally debuginfo-tests) has always had/been 
> a place for execution tests. Not all of them, and still value in having 
> lit-style non-execution cross-project tests where possible for more 
> robust/variation testing where possible, with fewer full 
> end-to-end-including-execution tests where possible (always going to be fuzzy 
> lines of what's worth the cost/benefit, etc - and we're all going to have 
> different points on that spectrum).

Yeah. Cross-project tests sounds like a great place to add more JIT tests, and 
maybe it's the correct home for clang-repl tests, but I think for now the right 
trade off is still to have execution tests and just restrict them to running on 
known-good configs.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107049/new/

https://reviews.llvm.org/D107049

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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 that JIT compiles C++ code for the host and 
>> throws C++ exceptions? Can we reconsider that?
>>
>> We have a policy of not running execution tests in the clang test suite 
>> because we know they always turn out to be unreliable, flaky, and highly 
>> dependent on the environment rather than the code under test. Integration 
>> tests are great, everyone needs them, but they should definitely not be part 
>> of the default set of tests that developers run anywhere and everywhere and 
>> expect to work out of the box, regardless of the environment. +@dblaikie to 
>> confirm if I am off base here about our testing strategy, and maybe Lang can 
>> advise us about past JIT testing strategies.
>
> Yeah, seems we've had some precedent for this for a while, as @lhames says 
> (doing some archaeology I see one of the clang-interpreter tests added here, 
> for instance: 
> https://github.com/llvm/llvm-project/commit/1d487617f20ab9df65ab60d6cf9431ef288312ab

Also all of the MJCIT and Orc tests in LLVM.

> I push back pretty hard on end-to-end tests in clang in most cases - one 
> place that slips through either untested or with end to end tests is anything 
> in MCOptions, etc, since they aren't in IR and are only observable through 
> their effect on the resulting assembly. And JITs - I didn't actually know we 
> had live JIT tests up in clang - and yeah, would rather we didn't - perhaps 
> via a mode in such frontends that emits the IR at various points, but it'll 
> be a pretty significant investment to make something fairly robust, I would 
> imagine (sort of like lldb reproducers replays (which are being removed due 
> to unmaintainability, by the sounds of it) but being able to stub out the 
> actual execution) -  hmm, perhaps a remote execution mock would be feasible? 
> That'd at least remove the execution part, make it portable to run on 
> non-native environments - but wouldn't remove the end-to-end-ness, for that 
> maybe you'd need a mock target that worked as though it ran IR natively, 
> perhaps? Not sure.

The LLVM MCJIT and Orc tests have been stable enough once we constrained the 
environments that they run in. I don't see why this would be any less stable. 
The features being exercised (essentially the object runtimes) are likely to be 
more stable in practice than the environments being tested in the LLDB tests.

> Then maybe the more end-to-end-y tests could move out to cross-project-tests.

Are you suggesting execution tests for cross-project-tests?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107049/new/

https://reviews.llvm.org/D107049

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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  ] InterpreterTest.CatchException
>   10:08:55 libunwind: __unw_add_dynamic_fde: bad fde: FDE is really a CIE
>   10:08:55 libc++abi: terminating with uncaught exception of type 
> custom_exception
>   ...

I suspect that this is a Linux distro that's using libunwind as the unwinder, 
at least for this test. Linux distros typically use libgcc_s for unwinding. The 
two libraries have different behavior for their `__register_frame` functions: 
libunwind's function expects to be passed a single FDE, libgcc_s's expects to 
be passed an entire .eh_frame section. We try to guess the unwinder in the JIT 
and use the appropriate argument (see [1][2]), but when we get it wrong this is 
often the result: we try to pass an .eh-frame section to libunwind's 
`__register_frame` and it errors out on a CIE at the start of the section.

@uabelho -- In your setup I'm seeing:

  -- Looking for __unw_add_dynamic_fde
  -- Looking for __unw_add_dynamic_fde - not found

So the question is, how are we failing to find `__unw_add_dynamic_fde` during 
config, only to crash in it during the test? Is use of libunwind on your 
platform expected?

side note: Peter Housel recently posted https://reviews.llvm.org/D111863 to add 
a new registration API with consistent behavior. Hopefully in the future we can 
rely on dynamic detection of this to eliminate the issue for users of future 
libunwinds.

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 that JIT compiles C++ code for the host and 
> throws C++ exceptions? Can we reconsider that?
>
> We have a policy of not running execution tests in the clang test suite 
> because we know they always turn out to be unreliable, flaky, and highly 
> dependent on the environment rather than the code under test. Integration 
> tests are great, everyone needs them, but they should definitely not be part 
> of the default set of tests that developers run anywhere and everywhere and 
> expect to work out of the box, regardless of the environment. +@dblaikie to 
> confirm if I am off base here about our testing strategy, and maybe Lang can 
> advise us about past JIT testing strategies.

The JIT has always used execution tests. It's difficult to test the JIT 
properly without them, since it doesn't have persistent output.

In practice the trick has always been to tighten the criteria for where these 
can run until things stabilize. We're seeing more failures on the test cases 
that Vassil is writing because he's pushing the boundaries of ORC's feature 
set, but I think the solution should still be the same: fix the JIT when we 
find bugs there, and restrict the tests to environments where they're expected 
to pass.

[1] 
https://github.com/llvm/llvm-project/commit/957334382cd12ec07b46c0ddfdcc220731f6d80f
[2] https://llvm.org/PR44074


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107049/new/

https://reviews.llvm.org/D107049

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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 across platforms.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111863/new/

https://reviews.llvm.org/D111863

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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 
in functionality for JIT clients.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111863/new/

https://reviews.llvm.org/D111863

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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. Neither needs to parse the instructions at 
> registration time.

Right -- I should have actually looked at the implementation of 
`__unw_add_dynamic_fde`. ;)

@joerg -- We have always registered every FDE in the JIT. This is an 
improvement over what we have, not a regression. Is there some more efficient 
approach that you would recommend? Otherwise I think this should go in.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111863/new/

https://reviews.llvm.org/D111863

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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. Parsing a whole segment is more involved as 
> it needs to look for the terminator of each block to find the next FDE.

Ok -- that makes sense. So I think `__unw_add_dynamic_eh_frame_section` just 
needs to walk the section and register the FDEs with `__unw_add_dynamic_fde`, 
rather than calling into DwarfFDECache up-front. Does that sound right?

From the JIT's perspective we're looking for two things: (1) to register a 
whole section with a single call (this avoids jumping in and out of libunwind 
for every FDE), and (2) a new symbol name whose semantics we can rely on, since 
`__register_frame` behaves differently in different unwinding libraries.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111863/new/

https://reviews.llvm.org/D111863

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112056/new/

https://reviews.llvm.org/D112056

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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 && Col && "Line and column should start from 1!"), 
function translateLineCol, file 
./llvm-project/clang/lib/Basic/SourceManager.cpp, line 1699.
  PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash 
backtrace.
  Stack dump:   
 
  0.  Program arguments: clang-format -lines=15:15 -lines=24:24 
-lines=46:47 -lines=73:74 -lines=82:82 -lines=117:117 -lines=128:128 
-lines=168:168 -lines=177:177 -lines=0:0 
llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp
  Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH 
or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):  
 
  0  clang-format 0x000101390dad 
llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) + 61
  1  clang-format 0x00010139135b 
PrintStackTraceSignalHandler(void*) + 27
  2  clang-format 0x00010138f00b llvm::sys::RunSignalHandlers() 
+ 123
  3  clang-format 0x000101393fb8 SignalHandler(int) + 232   
 
  4  libsystem_platform.dylib 0x7fff20639d7d _sigtramp + 29 
 
  5  libsystem_platform.dylib 00 _sigtramp + 
18446603339972764320
  6  libsystem_c.dylib0x7fff20549411 abort + 120

 
  7  libsystem_c.dylib0x7fff205487e8 err + 0
  8  clang-format 0x00010146e898 
clang::SourceManager::translateLineCol(clang::FileID, unsigned int, unsigned 
int) const + 136
  9  clang-format 0x0001011eb586 
clang::format::fillRanges(llvm::MemoryBuffer*, 
std::__1::vector >&) + 966
 
  10 clang-format 0x0001011dd718 
clang::format::format(llvm::StringRef) + 1064   
  11 clang-format 0x0001011dcc1b main + 955 

 
  12 libdyld.dylib0x7fff2060ff3d start + 1  
 
  13 libdyld.dylib0x000c start + 18446603339972935888   

 
  error: `clang-format -lines=15:15 -lines=24:24 -lines=46:47 -lines=73:74 
-lines=82:82 -lines=117:117 -lines=128:128 -lines=168:168 -lines=177:177 
-lines=0:0 llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp` failed

Reverting this patch fixed the error.

I am able to reliably reproduce this crash on Darwin by running:

  % git checkout fd26ca4e7515e7dd32ae02e777bd21693afc68ff
  % git am jitlink-table-manager-updates.patch
  % ./clang/tools/clang-format/git-clang-format HEAD~1

F19699034: jitlink-table-manager-updates.patch 



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111273/new/

https://reviews.llvm.org/D111273

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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 work as JIT use normally allows 
> registering functions individually.

I don't follow this. Does libunwind provide some way to register FDEs without 
parsing the FDE content? If so we can definitely use that, but we should still 
process the whole section: ORC links objects (not functions), and we should 
register every FDE for an object when it's linked in.

It's also worth noting that ORC and MCJIT have always called `__register_frame` 
on every frame, which seems like it should be at least as much work as this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111863/new/

https://reviews.llvm.org/D111863

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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 
itself, would be to compile a test case with eh-frame tables written in by hand 
(as C arrays, compile it with `-fno-asynchronous-unwind-tables`, patch up the 
tables manually with C code at runtime, and then register/use/deregister them. 
That's all pretty awkward, which is probably why there are no existing tests 
for the dynamic registration APIs.

I think the ORC runtime provides a much more natural way to test this. Did you 
manage to come up with some ORC-runtime based tests in the end?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111863/new/

https://reviews.llvm.org/D111863

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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 make that document more 
discoverable. If that document is not helpful then we should improve it.

> I guess the major difference in perspective may be due to different valuation 
> of the cost/"disease" it's intended to prevent - for myself I think the cost 
> is worthwhile but I understand that's not the case for you.

I've certainly found Error / Expected to be valuable for my work, but I 
designed them with my use-cases in mind so take that with a grain of salt. ;)

As a general observation: critiques of Error/Expected include the avoidable 
ergonomic issues (potentially addressable by API improvements), unavoidable 
ergonomic issues (we really want language support for this, but can't have it), 
and questions around correct application of Error/Expected -- sometimes error 
handling is just hard and Error/Expected feel awkward because they force you to 
confront it, and other times they're just not a good fit for whatever you're 
trying to do and you have to recognize when they should be dropped in favor of 
a more appropriate tool.

Especially on that first point (the avoidable ergonomic issues) I think that 
Error/Expected could benefit from review now that we've gained more practical 
experience with them. I'm not in a position to drive any such effort, but would 
be happy to help out.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105014/new/

https://reviews.llvm.org/D105014

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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
  https://reviews.llvm.org/D103855/new/

https://reviews.llvm.org/D103855

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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 data layouts: 
> e-m:e-i64:64-n32:64-S128-v256:256:256-v512:512:512 (module) vs 
> E-m:a-i64:64-n32:64-S128-v256:256:256-v512:512:512 (jit)

Thanks @hubert.reinterpretcast!

Ok, looks like the JIT is getting the layout right, but clang-repl is 
constructing a module with an little-endian layout for some reason. I'm not 
sure why that would be but it's probably a question for @v.g.vassilev tomorrow.

In the mean time I have conditionally disabled this test on ppc64 in 71a0609a2b 
.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96033/new/

https://reviews.llvm.org/D96033

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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 whether the JIT is getting the triple or the data 
layout wrong.

  diff --git a/clang/tools/clang-repl/ClangRepl.cpp 
b/clang/tools/clang-repl/ClangRepl.cpp
  index b5b5bf6e0c6b..cbf67f0e163e 100644
  --- a/clang/tools/clang-repl/ClangRepl.cpp
  +++ b/clang/tools/clang-repl/ClangRepl.cpp
  @@ -57,6 +57,12 @@ int main(int argc, const char **argv) {
 llvm::InitializeNativeTarget();
 llvm::InitializeNativeTargetAsmPrinter();
   
  +  auto JTMB = ExitOnErr(llvm::orc::JITTargetMachineBuilder::detectHost());
  +  llvm::errs() << "triple: " << JTMB.getTargetTriple().str() << "\n";
  +  llvm::errs() << "datalayout: "
  +   << ExitOnErr(JTMB.getDefaultDataLayoutForTarget())
  +  .getStringRepresentation()
  +   << "\n";
 if (OptHostSupportsJit) {
   auto J = llvm::orc::LLJITBuilder().create();
   if (J)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96033/new/

https://reviews.llvm.org/D96033

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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 
introducing an "initialization generation" -- each time you run 
`jit_dlopen_repl` you would increment the generation. You'd point the 
`__cxa_atexit` alias at a custom function that keeps a map: `__dso_handle -> 
(generation -> [ atexits ])`. Then you could selectively run atexits for each 
generation before removing them.




Comment at: clang/lib/CodeGen/CodeGenAction.cpp:908
 
+CodeGenerator *CodeGenAction::getCodeGenerator() const {
+  return BEConsumer->getCodeGenerator();

v.g.vassilev wrote:
> lhames wrote:
> > v.g.vassilev wrote:
> > > sgraenitz wrote:
> > > > v.g.vassilev wrote:
> > > > > @rjmccall, we were wondering if there is a better way to ask CodeGen 
> > > > > to start a new module. The current approach seems to be drilling hole 
> > > > > in a number of abstraction layers.
> > > > > 
> > > > > In the past we have touched that area a little in 
> > > > > https://reviews.llvm.org/D3 and the answer may be already there 
> > > > > but I fail to connect the dots.
> > > > > 
> > > > > Recently, we thought about having a new FrontendAction callback for 
> > > > > beginning a new phase when compiling incremental input. We need to 
> > > > > keep track of the created objects (needed for error recovery) in our 
> > > > > Transaction. We can have a map of `Transaction*` to `llvm::Module*` 
> > > > > in CodeGen. The issue is that new JITs take ownership of the 
> > > > > `llvm::Module*` which seems to make it impossible to support jitted 
> > > > > code removal with that model (cc: @lhames, @rsmith).
> > > > When compiling incrementally, doeas a "new phase" mean that all 
> > > > subsequent code will go into a new module from then on? How will 
> > > > dependencies to previous symbols be handled? Are all symbols external?
> > > > 
> > > > > The issue is that new JITs take ownership of the llvm::Module*
> > > > 
> > > > That's true, but you can still keep a raw pointer to it, which will be 
> > > > valid at least as long as the module wasn't linked. Afterwards it 
> > > > depends on the linker:
> > > > * RuntimeDyld can return ownership of the object's memory range via 
> > > > `NotifyEmittedFunction`
> > > > * JITLink provides the `ReturnObjectBufferFunction` for the same purpose
> > > > 
> > > > > seems to make it impossible to support jitted code removal with that 
> > > > > model
> > > > 
> > > > Can you figure out what symbols are affected and remove these? A la: 
> > > > https://github.com/llvm/llvm-project/blob/13f4448ae7db1a47/llvm/include/llvm/ExecutionEngine/Orc/Core.h#L1020
> > > > 
> > > > I think @anarazel has ported a client with code removal to OrcV2 
> > > > successfully in the past. Maybe there's something we can learn from it.
> > > > When compiling incrementally, doeas a "new phase" mean that all 
> > > > subsequent code will go into a new module from then on? How will 
> > > > dependencies to previous symbols be handled? Are all symbols external?
> > > 
> > > There is some discussion on this here 
> > > https://reviews.llvm.org/D3#812418
> > > 
> > > I think the relevant bit is that 'we have just one ever growing TU [...] 
> > > which we send to the RuntimeDyLD allowing only JIT to resolve symbols 
> > > from it.  We aid the JIT when resolving symbols with internal linkage by 
> > > changing all internal linkage to external (We haven't seen issues with 
> > > that approach)'.
> > > 
> > > > 
> > > > > The issue is that new JITs take ownership of the llvm::Module*
> > > > 
> > > > That's true, but you can still keep a raw pointer to it, which will be 
> > > > valid at least as long as the module wasn't linked. 
> > > 
> > > That was my first implementation when I upgraded cling to llvm9 where the 
> > > `shared_ptr`s went to `unique_ptr`s. This was quite problematic for many 
> > > of the use cases we support as the JIT is somewhat unpredictable to the 
> > > high-level API user. 
> > > 
> > > 
> > > >Afterwards it depends on the linker:
> > > > * RuntimeDyld can return ownership of the object's memory range via 
> > > > `NotifyEmittedFunction`
> > > > * JITLink provides the `ReturnObjectBufferFunction` for the same purpose
> > > > 
> > > 
> > > That's exactly what we ended up doing (I would like to thank Lang here 
> > > who gave a similar advice).
> > > 
> > > > > seems to make it impossible to support jitted code removal with that 
> > > > > model
> > > > 
> > > > Can you figure out what symbols are affected and remove these? A la: 
> > > > https://github.com/llvm/llvm-project/blob/13f4448ae7db1a47/llvm/include/llvm/ExecutionEngine/Orc/Core.h#L1020
> > > > 
> > > > I think @anarazel has ported a client with code removal to OrcV2 
> > > > successfully 

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

[Lang Hames via Phabricator via cfe-commits]

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:
> > > @rjmccall, we were wondering if there is a better way to ask CodeGen to 
> > > start a new module. The current approach seems to be drilling hole in a 
> > > number of abstraction layers.
> > > 
> > > In the past we have touched that area a little in 
> > > https://reviews.llvm.org/D3 and the answer may be already there but I 
> > > fail to connect the dots.
> > > 
> > > Recently, we thought about having a new FrontendAction callback for 
> > > beginning a new phase when compiling incremental input. We need to keep 
> > > track of the created objects (needed for error recovery) in our 
> > > Transaction. We can have a map of `Transaction*` to `llvm::Module*` in 
> > > CodeGen. The issue is that new JITs take ownership of the `llvm::Module*` 
> > > which seems to make it impossible to support jitted code removal with 
> > > that model (cc: @lhames, @rsmith).
> > When compiling incrementally, doeas a "new phase" mean that all subsequent 
> > code will go into a new module from then on? How will dependencies to 
> > previous symbols be handled? Are all symbols external?
> > 
> > > The issue is that new JITs take ownership of the llvm::Module*
> > 
> > That's true, but you can still keep a raw pointer to it, which will be 
> > valid at least as long as the module wasn't linked. Afterwards it depends 
> > on the linker:
> > * RuntimeDyld can return ownership of the object's memory range via 
> > `NotifyEmittedFunction`
> > * JITLink provides the `ReturnObjectBufferFunction` for the same purpose
> > 
> > > seems to make it impossible to support jitted code removal with that model
> > 
> > Can you figure out what symbols are affected and remove these? A la: 
> > https://github.com/llvm/llvm-project/blob/13f4448ae7db1a47/llvm/include/llvm/ExecutionEngine/Orc/Core.h#L1020
> > 
> > I think @anarazel has ported a client with code removal to OrcV2 
> > successfully in the past. Maybe there's something we can learn from it.
> > When compiling incrementally, doeas a "new phase" mean that all subsequent 
> > code will go into a new module from then on? How will dependencies to 
> > previous symbols be handled? Are all symbols external?
> 
> There is some discussion on this here https://reviews.llvm.org/D3#812418
> 
> I think the relevant bit is that 'we have just one ever growing TU [...] 
> which we send to the RuntimeDyLD allowing only JIT to resolve symbols from 
> it.  We aid the JIT when resolving symbols with internal linkage by changing 
> all internal linkage to external (We haven't seen issues with that approach)'.
> 
> > 
> > > The issue is that new JITs take ownership of the llvm::Module*
> > 
> > That's true, but you can still keep a raw pointer to it, which will be 
> > valid at least as long as the module wasn't linked. 
> 
> That was my first implementation when I upgraded cling to llvm9 where the 
> `shared_ptr`s went to `unique_ptr`s. This was quite problematic for many of 
> the use cases we support as the JIT is somewhat unpredictable to the 
> high-level API user. 
> 
> 
> >Afterwards it depends on the linker:
> > * RuntimeDyld can return ownership of the object's memory range via 
> > `NotifyEmittedFunction`
> > * JITLink provides the `ReturnObjectBufferFunction` for the same purpose
> > 
> 
> That's exactly what we ended up doing (I would like to thank Lang here who 
> gave a similar advice).
> 
> > > seems to make it impossible to support jitted code removal with that model
> > 
> > Can you figure out what symbols are affected and remove these? A la: 
> > https://github.com/llvm/llvm-project/blob/13f4448ae7db1a47/llvm/include/llvm/ExecutionEngine/Orc/Core.h#L1020
> > 
> > I think @anarazel has ported a client with code removal to OrcV2 
> > successfully in the past. Maybe there's something we can learn from it.
> 
> Indeed. That's not yet on my radar as seemed somewhat distant in time.
> 
> Recently, we thought about having a new FrontendAction callback for beginning 
> a new phase when compiling incremental input. We need to keep track of the 
> created objects (needed for error recovery) in our Transaction. We can have a 
> map of Transaction* to llvm::Module* in CodeGen. The issue is that new JITs 
> take ownership of the llvm::Module* which seems to make it impossible to 
> support jitted code removal with that model (cc: @lhames, @rsmith).

In the new APIs, in order to enable removable code, you can associate Modules 
with ResourceTrackers when they're added to the JIT. The ResourceTrackers then 
allow for removal. Idiomatic usage looks like:

  auto Mod = /* create module */;
  auto RT = JD.createResourceTracker();
  J.addModule(RT, std::move(Mod));
  //...
  if (auto Err = RT.remove())
/* handle 

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

[Lang Hames via Phabricator via cfe-commits]

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 be D39111  or 
> just a ClassID type) sounds good to me.


Yep -- To be clear I didn't raise D39111  as 
an objection to this approach, just as a matter of interest. As you noted it 
should be easy to switch later.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71397/new/

https://reviews.llvm.org/D71397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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
  https://reviews.llvm.org/D71397/new/

https://reviews.llvm.org/D71397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69356: [NFC] Rename LLVM_NO_DEAD_STRIP

[Lang Hames via Phabricator via cfe-commits]

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 two options.


Yep — there are use-cases for no-dead-strip that aren’t plugins. I’m not sure 
this rename helps. I think the .*PLUGINS.* options need more consideration, and 
that LLVM_NO_DEAD_STRIP should be reinstated.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69356/new/

https://reviews.llvm.org/D69356



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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.
>
> @lhames, is it reasonable to add `llvm::expectedToOptional`?


Absolutely: We've already got an errorToBool, so expectedToOptional makes sense.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66705/new/

https://reviews.llvm.org/D66705



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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 
cantFail in unit tests, but Puyan's right that it would be abuse: in production 
code cantFail should only be used at call sites where you know from context 
that the call //definitely// can't fail (e.g. subsequent calls to an API that 
can only fail on the first call).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65829/new/

https://reviews.llvm.org/D65829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63518: BitStream reader: propagate errors

[Lang Hames via Phabricator via cfe-commits]

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 will generally match existing behavior in places 
where you were ignoring errors. cantFail will get you aggressive crashes, which 
can be handy for debugging but not so fun in release code.

Also, if this patch passes the regression tests we need more failure tests. :)




Comment at: clang-tools-extra/clang-doc/BitcodeReader.cpp:527-529
+  if (llvm::Error Err = readSubBlock(BlockOrCode, I)) {
+if (llvm::Error Skipped = Stream.SkipBlock())
+  return Skipped;

Bigcheese wrote:
> This is a pretty big behavior change.  Is clang-doc expecting to get files 
> with unknown blocks?
The inner test here is unsafe, as it will discard the outer error. You need:

  if (llvm::Error Err = readSubBlock(BlockOrCode, I)) {
if (llvm::Error Skipped = Stream.SkipBlock()) {
  consumeError(std::move(Err));
  return Skipped;
}
return Err;
  }

or:

  if (llvm::Error Err = readSubBlock(BlockOrCode, I)) {
if (llvm::Error Skipped = Stream.SkipBlock()) {
  return joinErrors(std::move(Err), std::move(Skipped));
return Err;
  }



Comment at: clang-tools-extra/clang-doc/BitcodeReader.cpp:611-618
+Expected MaybeCode = Stream.ReadCode();
+if (!MaybeCode) {
+  // FIXME this drops the error on the floor.
+  consumeError(MaybeCode.takeError());
+}
+
+// FIXME check that the enum is in range.

Bigcheese wrote:
> Does `consumeError` return?  `MaybeCode.get()` will crash if `!MaybeCode`.
Yes: consumeError returns. This should probably be:

  if (!MaybeCode)
return consumeError(MaybeCode.takeError()), Cursor::Badness;




Comment at: clang/lib/Frontend/CompilerInstance.cpp:2051-2053
+  // FIXME this drops the error on the floor. This code is only used for
+  // typo correction and drops more than just this one source of errors
+  // (such as the directory creation failure above).

This will crash if writeIndex ever generates an error. For that reason I would 
suggest writing this as:

  // FIXME: Can actually fail! 
  cantFail(GlobalModuleIndex::writeIndex(...));

It has the same effect, but without the control flow.




Comment at: clang/lib/Frontend/CompilerInstance.cpp:2082
+  getPreprocessor().getHeaderSearchInfo().getModuleCachePath())) {
+// FIXME As above, this drops the error on the floor.
+  }

Another cantFail candidate.



Comment at: clang/lib/Frontend/FrontendAction.cpp:945-948
+// FIXME this drops the error on the floor, but
+// Index/pch-from-libclang.c seems to rely on dropping at least some of
+// the error conditions!
+consumeError(std::move(Err));

I'd suggest cantFail here, if not for that fixme comment. Yikes.



Comment at: clang/lib/Frontend/Rewrite/FrontendActions.cpp:132-133
  CI.getLangOpts(), FixItOpts.get());
-  FixAction->Execute();
+  if (llvm::Error Err = FixAction->Execute())
+err = true; // FIXME this drops the error on the floor.
 

You need a consumeError here or you'll get a runtime crash if an error is 
generated.



Comment at: clang/lib/Frontend/SerializedDiagnosticReader.cpp:89-91
+  if (llvm::Error Err = Stream.SkipBlock())
+return SDError::MalformedTopLevelBlock; // FIXME propagate the error
+// details.

This needs a consumeError or it will crash if an error is generated. How about:

if (llvm::Error Err = Stream.SkipBlock())
  return consumeError(std::move(Err)), SDError::MalformedTopLevelBlock;




Comment at: clang/lib/Frontend/SerializedDiagnosticReader.cpp:149-151
+  if (llvm::Error Err =
+  Stream.EnterSubBlock(clang::serialized_diags::BLOCK_META))
+return SDError::MalformedMetadataBlock; // FIXME propagate the error

Needs a consumeError.



Comment at: clang/lib/Frontend/SerializedDiagnosticReader.cpp:166-167
 case Cursor::BlockBegin:
-  if (Stream.SkipBlock())
-return SDError::MalformedMetadataBlock;
+  if (llvm::Error Err = Stream.SkipBlock())
+return SDError::MalformedMetadataBlock; // FIXME propagate the error
+// details.

Needs a consumeError.



Comment at: clang/lib/Frontend/SerializedDiagnosticReader.cpp:194-196
+  if (llvm::Error Err =
+  Stream.EnterSubBlock(clang::serialized_diags::BLOCK_DIAG))
+return SDError::MalformedDiagnosticBlock; // 

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

[Lang Hames via Phabricator via cfe-commits]

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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 for you.


https://reviews.llvm.org/D36806



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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 must not conflict since ranges have been merged.
>  llvm::cantFail(FakeReplaces.add(...));

Yep - cantFail asserts that the result is success, so your proposed change 
(with the assertion text above the call) is ideal.


Repository:
  rL LLVM

https://reviews.llvm.org/D36728



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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 return an Expected.


Repository:
  rL LLVM

https://reviews.llvm.org/D36728



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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 style says no braces for single line conditionals. :)


https://reviews.llvm.org/D36429



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

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})
+#message("COMPILE_FLAGS: '${addedFlags}' '${editedFlags}'")
+

These debugging messages (this and the one below) should probably be stripped.



Comment at: examples/clang-interpreter/Invoke.h:10
+
+#pragma once
+

I believe you should use include guards in LLVM headers. Pragma once should 
work, but I don't think it's standard (and it doesn't seem to be used anywhere 
else in the codebase).



Comment at: examples/clang-interpreter/Manager.h:10
+
+#pragma once
+

Ditto here.


https://reviews.llvm.org/D35103



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits