[PATCH] D155716: [clang][CodeGen] Introduce `-frecord-command-line` for MachO

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz accepted this revision.
sgraenitz added a comment.
This revision is now accepted and ready to land.

Great! This LGTM. Let's keep the review open for a few days and see if there is 
more feedback.




Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:1424
+MCSection *TargetLoweringObjectFileMachO::getSectionForCommandLines() const {
+  return getContext().getMachOSection("__TEXT", "__command_line", 0,
+  SectionKind::getReadOnly());

antoniofrighetto wrote:
> sgraenitz wrote:
> > Can we put it in `__DATA`?
> > 
> > Also the [[ 
> > https://github.com/llvm/llvm-project/blob/release/16.x/llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp#L1160
> >  | ELF implementation notes ]] that it "attempts to mimic GCC's switch of 
> > the same name" and thus calls the section `.GCC.command.line`. I guess we 
> > cannot use the dots in MachO, but should we add a `gcc` prefix?
> Whilst I agree it should be better to distinguish this from executable data, 
> I think this should live as read-only data, which `__TEXT` is traditionally 
> for.
> 
> Following MachO conventions, I first tried `__gcc_command_line`, but the [[ 
> https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/BinaryFormat/MachO.h#L587
>  | section name ]] is restricted to 16 chars, and I'm not sure adding more 
> bytes for the name is worth the change (thus I thought we'd prefer 
> `__command_line` over `__gcc_cmd_line`).
> [...] the section name is restricted to 16 chars, and I'm not sure adding 
> more bytes for the name is worth the change

Ok, then I guess it's fine to keep this as is.

> I think this should live as read-only data, which `__TEXT` is traditionally 
> for.

Oh interesting, I had expected permissions to be `R-X` for everything in the 
text segment, but I might be wrong.  And yes, the command-line string should be 
`R--` (read-only).

So, I looked at a few similar cases in existing code and it confirms your 
statement. `__cstring` e.g. appears to in `__TEXT` as well even though objdump 
will show it as type DATA:
```
Idx Name Size VMA  Type
  0 __text   00014029  TEXT
  1 __StaticInit 005f 00014030 TEXT
  ...
  6 __data   0040 000148e0 DATA
  7 __cstring084b 00014920 DATA
```

I am probably lacking some MachO expertise to understand the details here. 
Since you do the same as we do for `__cstring` in other places, I think this is 
good.


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

https://reviews.llvm.org/D155716

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

[PATCH] D155716: [clang][CodeGen] Introduce `-frecord-command-line` for MachO

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

That's an excellent patch! I didn't expect it could be so compact. Thanks for 
working on this! Please find below 2 inline comments on details.
On July 25th release/17.x will branch away. It would be great to land this 
before and have it in the upcoming release.




Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:1424
+MCSection *TargetLoweringObjectFileMachO::getSectionForCommandLines() const {
+  return getContext().getMachOSection("__TEXT", "__command_line", 0,
+  SectionKind::getReadOnly());

Can we put it in `__DATA`?

Also the [[ 
https://github.com/llvm/llvm-project/blob/release/16.x/llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp#L1160
 | ELF implementation notes ]] that it "attempts to mimic GCC's switch of the 
same name" and thus calls the section `.GCC.command.line`. I guess we cannot 
use the dots in MachO, but should we add a `gcc` prefix?



Comment at: llvm/test/CodeGen/AArch64/arm64-command-line-metadata.ll:1
+; RUN: llc -mtriple=aarch64-apple-darwin < %s | FileCheck %s
+; Verify that llvm.commandline metadata is emitted to a

The triple doesn't match the name of the test: arm64-command-line-metadata.ll

Why not rename the file to `commandline-metadata.ll` (as in 
llvm/test/CodeGen/X86/commandline-metadata.ll) and maybe change the triple into 
the more common `arm64-apple`?

Optionally, we could also add RUN and CHECK lines for the existing ELF feature 
in AArch64 if there isn't one already.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155716

___
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

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

In D141215#4272538 , @lhames wrote:

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

Agree, that's probably the better solution. Just for the record: Values 
couldn't be temporaries and access must be synchronized with execution to avoid 
races. I guess both is easily acceptable.




Comment at: clang/include/clang/Interpreter/Value.h:160-162
+  // Interpreter, QualType are stored as void* to reduce dependencies.
+  void *Interp = nullptr;
+  void *OpaqueType = nullptr;

v.g.vassilev wrote:
> aaron.ballman wrote:
> > junaire wrote:
> > > aaron.ballman wrote:
> > > > v.g.vassilev wrote:
> > > > > aaron.ballman wrote:
> > > > > > v.g.vassilev wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > Why don't forward declares suffice if we're storing the 
> > > > > > > > information by pointer?
> > > > > > > This is a performance-critical class. We literally measure the 
> > > > > > > instruction count for it. We practically cannot include anything 
> > > > > > > in this header file because the class needs to included as part 
> > > > > > > of the interpreter runtime. For example:
> > > > > > > 
> > > > > > > ```
> > > > > > > #include 
> > > > > > > Value ResultV;
> > > > > > > gInterpreter->evaluate("float i = 12.3; i++", );
> > > > > > > printf("Value is %d\n", ResultV.castAs());
> > > > > > > ```
> > > > > > > 
> > > > > > > This is how you can do things in Cling. This not yet there but 
> > > > > > > that's our next step.
> > > > > > > 
> > > > > > > For performance reasons we have the `ValueKind` optimization 
> > > > > > > which allows us to perform most of the operations we need very 
> > > > > > > fast. There are some operations such as printing the concrete 
> > > > > > > type which need the actual `QualType` and so on but they are 
> > > > > > > outside of the performance critical paths and it is okay to 
> > > > > > > resort back to the real types providing the level of accuracy we 
> > > > > > > need.
> > > > > > > 
> > > > > > That sounds like it's going to lead to maintenance problems in the 
> > > > > > long term, right? I can't think of another header file we say 
> > > > > > "don't touch this because it may impact runtime performance", and 
> > > > > > so I can easily imagine someone breaking your expectation that this 
> > > > > > file can't include anything else.
> > > > > > 
> > > > > > Is there a long-term plan for addressing this?
> > > > > We have a few components like the Lexer that are extremely prone to 
> > > > > performance regressions.
> > > > > 
> > > > > In terms for a longer-term plan in addressing this there are some 
> > > > > steps could help IMO. First, this component is relatively standalone 
> > > > > and very few changes will be required over time, for these I am 
> > > > > hoping to be listed as a reviewer. Second, we can add a comment in 
> > > > > the include area, making a note that including anything here will 
> > > > > degrade the performance of almost all interpreted code. Third, we 
> > > > > will find out about this in our downstream use-cases as the things 
> > > > > get significantly slower.
> > > > > 
> > > > > Neither is silver bullet but that's probably the best we could do at 
> > > > > that time. Btw, we might be able to add a test that's part of LLVM's 
> > > > > performance analysis infrastructure.
> > > > > Neither is silver bullet but that's probably the best we could do at 
> > > > > that time. Btw, we might be able to add a test that's part of LLVM's 
> > > > > performance analysis infrastructure.
> > > > 
> > > > Yeah, we should probably consider doing that. But to make sure I 
> > > > understand the performance concerns... when we change functionality in 
> > > > the lexer, we (potentially) slow down the lexing phase of compilation. 
> > > > That's straightforward and unsurprising. But in this case, it sounds 
> > > > like the act of including another header file in this header file will 
> > > > cause a runtime performance concern, even if no other changes are made. 
> > > > If I'm correct, I can't think of anything else in the compiler that 
> > > > works like that.
> > > I believe what @v.g.vassilev means is that the repl itself might include 
> > > `Value.h` as a part of *runtime*, so if the header is heavy, you can 
> > > notice the repl is slowed down. (That's obvious) So keep in mind we're 
> > > breaking the boundary between the compiled code and interpreted code 
> > > (It's kinda confusing) here it actually impacts interpreted code.
> > > I believe what @v.g.vassilev means is that the repl itself might include 
> > > Value.h as a part of *runtime*, so if the header is heavy, you can notice 
> > > the repl 

[PATCH] D148992: [clang-repl] Fix dynamic library test to avoid cstdio and linker

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz accepted this revision.
sgraenitz added a comment.

Thanks. LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148992

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

[PATCH] D148992: [clang-repl] Fix dynamic library test to avoid cstdio and linker

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added inline comments.



Comment at: clang/test/Interpreter/dynamic-library.cpp:12
+//   return 5;
+// }
 

Should we wrap this in a `extern "C"` block? Otherwise, the shared library has 
a C++ interface, which is not stable. It won't matter in this simple case, but 
the test might become a pattern.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148992

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

[PATCH] D141824: [clang-repl] Add a command to load dynamic libraries

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

In D141824#4274461 , @argentite wrote:

> We should probably also address the lack of linker issue as well. Should we 
> go for a precompiled dynamic library file? There seems to be some "precedent" 
> of this in other tests.

Yes, since yaml2obj can't do it and the test only runs on Linux anyway, it 
seems reasonable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141824

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

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz accepted this revision.
sgraenitz added a comment.

LGTM


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] D141824: [clang-repl] Add a command to load dynamic libraries

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added inline comments.



Comment at: clang/test/Interpreter/dynamic-library.cpp:6
+
+#include 
+

bcain wrote:
> This test fails for me like below.  
> 
> 
> ```
> FAIL: Clang :: Interpreter/dynamic-library.cpp (1 of 17751)
>  TEST 'Clang :: Interpreter/dynamic-library.cpp' FAILED 
> 
> Script:
> --
> : 'RUN: at line 4';   /local/mnt/workspace/upstream/obj_ubuntu/bin/clang 
> -xc++ -o 
> /local/mnt/workspace/upstream/obj_ubuntu/tools/clang/test/Interpreter/Output/libdynamic-library-test.so
>  -fPIC -shared -DLIBRARY 
> /local/mnt/workspace/upstream/llvm-project/clang/test/Interpreter/Inputs/dynamic-library-test.cpp
> : 'RUN: at line 5';   cat 
> /local/mnt/workspace/upstream/llvm-project/clang/test/Interpreter/dynamic-library.cpp
>  | env 
> LD_LIBRARY_PATH=/local/mnt/workspace/upstream/obj_ubuntu/tools/clang/test/Interpreter/Output:$LD_LIBRARY_PATH
>  /local/mnt/workspace/upstream/obj_ubuntu/bin/clang-repl | 
> /local/mnt/workspace/upstream/obj_ubuntu/bin/FileCheck 
> /local/mnt/workspace/upstream/llvm-project/clang/test/Interpreter/dynamic-library.cpp
> --
> Exit Code: 1
> 
> Command Output (stderr):
> --
> In file included from <<< inputs >>>:1:
> input_line_6:1:10: fatal error: 'cstdio' file not found
> #include 
>  ^~~~
> error: Parsing failed.
> input_line_12:1:1: error: use of undeclared identifier 'printf'
> printf("Return value: %d\n", calculate_answer());
> ^
> error: Parsing failed.
> input_line_15:1:1: error: use of undeclared identifier 'printf'
> printf("Variable: %d\n", ultimate_answer);
> ^
> error: Parsing failed.
> /local/mnt/workspace/upstream/llvm-project/clang/test/Interpreter/dynamic-library.cpp:15:11:
>  error: CHECK: expected string not found in input
> // CHECK: Return value: 5
>   ^
> :1:1: note: scanning from here
> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> 
> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> 
> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> 
> clang-repl> clang-repl> 
> ^
> :1:231: note: possible intended match here
> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> 
> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> 
> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> 
> clang-repl> clang-repl> 
>   
>   
>   ^
> 
> Input file: 
> Check file: 
> /local/mnt/workspace/upstream/llvm-project/clang/test/Interpreter/dynamic-library.cpp
> 
> -dump-input=help explains the following input dump.
> 
> Input was:
> <<
> 1: clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> 
> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> 
> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> 
> clang-repl> clang-repl> clang-repl>  
> check:15'0 
> X
>  error: no match found
> check:15'1
>   
>   
>?   possible intended match
> >>
> 
> --
> 
> 
> 
> Failed Tests (1):
>   Clang :: Interpreter/dynamic-library.cpp
> 
> ```
Thanks for your note, sounds reasonable. @argentite It should be sufficient to 
forward declare `printf`right? While we are here, can we somehow check whether 
the symbol is non-null before calling it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141824

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

[PATCH] D148434: [clang-repl] JITTargetAddress --> ExecutorAddr

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz accepted this revision.
sgraenitz added a comment.
This revision is now accepted and ready to land.

Thanks. Maybe you can fix this one detail and add a description like "Most of 
Orc and JITLink are movinng away from JITTargetAddress and use ExecutorAddr 
instead"? Otherwise LGTM.




Comment at: clang/lib/Interpreter/IncrementalExecutor.cpp:88
 return Sym.takeError();
-  return Sym->getValue();
+  return llvm::orc::ExecutorAddr(Sym->getValue());
 }

I think `Sym` is a `Expected` already


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148434

___
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

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

This looks a lot better already than the implementation I know from Cling. Well 
done!

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?




Comment at: clang/include/clang/Interpreter/Interpreter.h:83
   /// mangled name.
   llvm::Expected getSymbolAddress(GlobalDecl GD) const;
 

Most of the Orc and JITLink moved away from `JITTargetAddress` already and uses 
`ExecutorAddr` nowadays. I think we should use `ExecutorAddr` from the 
beginning in this patch.



Comment at: clang/include/clang/Interpreter/Value.h:98
+  QualType getType() const;
+  Interpreter () const;
+

Can we make this private and try not to introduce more dependencies on the 
interpreter instance?

The inherent problem is that we will have to wire these through Orc RPC in the 
future once we want to support out-of-process execution.



Comment at: clang/lib/Interpreter/Interpreter.cpp:406
+"__InterpreterSetValueNoAlloc", "__InterpreterSetValueWithAlloc",
+"__InterpreterSetValueCopyArr"};
+

In Orc we are usually prefixing such functions with the namespace and class 
name from where they are used. While it can impact readability, it gives these 
names a structure and does prevent collisions, e.g.:
`__clang_Interpreter_SetValueNoAlloc`.



Comment at: clang/lib/Interpreter/Interpreter.cpp:534
+}
+  llvm_unreachable("Unknown runtime interface kind");
+}

Is this a leftover from an old `default` label?



Comment at: clang/lib/Interpreter/Value.cpp:91
+  static constexpr unsigned char Canary[8] = {0x4c, 0x37, 0xad, 0x8f,
+  0x2d, 0x23, 0x95, 0x91};
+};

Can we add a comment with an explanation of the magic numbers please?


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] D141824: [clang-repl] Add a command to load dynamic libraries

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

In D141824#4247891 , @zhuhan0 wrote:

> This seems to be the first clang test that involves actual linking.

Interesting, I think we all didn't have this on our radars.

> If the intention is to test loading a dynamic library, could you commit the 
> library in the Inputs directory instead of building it from source? cc 
> @ayermolo

Is that something other tests do? Otherwise, I am not sure it's a practical 
solution. We could also consider another requirement like 
`host-supports-linking` for the test. What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141824

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

[PATCH] D141824: [clang-repl] Add a command to load dynamic libraries

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz accepted this revision.
sgraenitz added a comment.
This revision is now accepted and ready to land.

Thanks! LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141824

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

[PATCH] D141824: [clang-repl] Add a command to load dynamic libraries

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

Thanks for the update




Comment at: clang/test/Interpreter/dynamic-library.cpp:1
+// RUN: head -n 7 %s | %clang -xc++ -o %T/libdynamic-library-test.so -fPIC 
-shared -
+int ultimate_answer = 0;

The use of `head` and `tail` here is a creative solution, but I wonder how well 
it scales for similar cases in the future in case this becomes a role model. We 
have this situation a lot in LLDB: compile an input file and use the output in 
a RUN line. It typically uses separate input files for such purposes (note that 
"Inputs" folders must be excluded from test discovery somewhere in lit config), 
e.g.:
https://github.com/llvm/llvm-project/blob/release/16.x/lldb/test/Shell/Breakpoint/jit-loader_jitlink_elf.test

What do you think?



Comment at: clang/test/Interpreter/dynamic-library.cpp:9
+
+// REQUIRES: host-supports-jit, system-linux
+// RUN: tail -n 16 %s | env LD_LIBRARY_PATH=%T:$LD_LIBRARY_PATH clang-repl | 
FileCheck %s

This requirement applies to the entire file right? The arrangement here may 
imply the opposite.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141824

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

[PATCH] D138122: Lift EHPersonalities from Analysis to IR (NFC)

[Stefan Gränitz via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG3b387d10707d: Lift EHPersonalities from Analysis to IR (NFC) 
(authored by sgraenitz).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138122

Files:
  clang/docs/tools/clang-formatted-files.txt
  llvm/include/llvm/Analysis/EHPersonalities.h
  llvm/include/llvm/Analysis/MustExecute.h
  llvm/include/llvm/CodeGen/MachineFunction.h
  llvm/include/llvm/IR/EHPersonalities.h
  llvm/lib/Analysis/CMakeLists.txt
  llvm/lib/Analysis/EHPersonalities.cpp
  llvm/lib/Analysis/ValueTracking.cpp
  llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
  llvm/lib/CodeGen/DwarfEHPrepare.cpp
  llvm/lib/CodeGen/MachineFunction.cpp
  llvm/lib/CodeGen/MachineVerifier.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
  llvm/lib/CodeGen/StackProtector.cpp
  llvm/lib/CodeGen/WinEHPrepare.cpp
  llvm/lib/IR/CMakeLists.txt
  llvm/lib/IR/EHPersonalities.cpp
  llvm/lib/Target/M68k/M68kCollapseMOVEMPass.cpp
  llvm/lib/Target/M68k/M68kExpandPseudo.cpp
  llvm/lib/Target/X86/X86ExpandPseudo.cpp
  llvm/lib/Target/X86/X86FrameLowering.cpp
  llvm/lib/Target/X86/X86ISelLowering.cpp
  llvm/lib/Target/X86/X86WinEHState.cpp
  llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
  llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
  llvm/lib/Transforms/ObjCARC/ObjCARC.h
  llvm/lib/Transforms/ObjCARC/ObjCARCContract.cpp
  llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
  llvm/lib/Transforms/Utils/EscapeEnumerator.cpp
  llvm/lib/Transforms/Utils/InlineFunction.cpp
  llvm/lib/Transforms/Utils/Local.cpp
  llvm/utils/gn/secondary/llvm/lib/Analysis/BUILD.gn
  llvm/utils/gn/secondary/llvm/lib/IR/BUILD.gn

Index: llvm/utils/gn/secondary/llvm/lib/IR/BUILD.gn
===
--- llvm/utils/gn/secondary/llvm/lib/IR/BUILD.gn
+++ llvm/utils/gn/secondary/llvm/lib/IR/BUILD.gn
@@ -34,6 +34,7 @@
 "DiagnosticInfo.cpp",
 "DiagnosticPrinter.cpp",
 "Dominators.cpp",
+"EHPersonalities.cpp",
 "FPEnv.cpp",
 "Function.cpp",
 "GCStrategy.cpp",
Index: llvm/utils/gn/secondary/llvm/lib/Analysis/BUILD.gn
===
--- llvm/utils/gn/secondary/llvm/lib/Analysis/BUILD.gn
+++ llvm/utils/gn/secondary/llvm/lib/Analysis/BUILD.gn
@@ -49,7 +49,6 @@
 "DomPrinter.cpp",
 "DomTreeUpdater.cpp",
 "DominanceFrontier.cpp",
-"EHPersonalities.cpp",
 "FunctionPropertiesAnalysis.cpp",
 "GlobalsModRef.cpp",
 "GuardUtils.cpp",
Index: llvm/lib/Transforms/Utils/Local.cpp
===
--- llvm/lib/Transforms/Utils/Local.cpp
+++ llvm/lib/Transforms/Utils/Local.cpp
@@ -25,7 +25,6 @@
 #include "llvm/Analysis/AssumeBundleQueries.h"
 #include "llvm/Analysis/ConstantFolding.h"
 #include "llvm/Analysis/DomTreeUpdater.h"
-#include "llvm/Analysis/EHPersonalities.h"
 #include "llvm/Analysis/InstructionSimplify.h"
 #include "llvm/Analysis/MemoryBuiltins.h"
 #include "llvm/Analysis/MemorySSAUpdater.h"
@@ -47,6 +46,7 @@
 #include "llvm/IR/DebugLoc.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/Dominators.h"
+#include "llvm/IR/EHPersonalities.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GetElementPtrTypeIterator.h"
 #include "llvm/IR/GlobalObject.h"
Index: llvm/lib/Transforms/Utils/InlineFunction.cpp
===
--- llvm/lib/Transforms/Utils/InlineFunction.cpp
+++ llvm/lib/Transforms/Utils/InlineFunction.cpp
@@ -23,7 +23,6 @@
 #include "llvm/Analysis/BlockFrequencyInfo.h"
 #include "llvm/Analysis/CallGraph.h"
 #include "llvm/Analysis/CaptureTracking.h"
-#include "llvm/Analysis/EHPersonalities.h"
 #include "llvm/Analysis/InstructionSimplify.h"
 #include "llvm/Analysis/MemoryProfileInfo.h"
 #include "llvm/Analysis/ObjCARCAnalysisUtils.h"
@@ -42,6 +41,7 @@
 #include "llvm/IR/DebugLoc.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/Dominators.h"
+#include "llvm/IR/EHPersonalities.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/InlineAsm.h"
Index: llvm/lib/Transforms/Utils/EscapeEnumerator.cpp
===
--- llvm/lib/Transforms/Utils/EscapeEnumerator.cpp
+++ llvm/lib/Transforms/Utils/EscapeEnumerator.cpp
@@ -13,7 +13,7 @@
 
 #include "llvm/Transforms/Utils/EscapeEnumerator.h"
 #include "llvm/ADT/Triple.h"
-#include "llvm/Analysis/EHPersonalities.h"
+#include "llvm/IR/EHPersonalities.h"
 #include "llvm/IR/Module.h"
 #include "llvm/Transforms/Utils/Local.h"
 
Index: llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp

[PATCH] D141824: [clang-repl] Add a command to load dynamic libraries

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added inline comments.



Comment at: clang/lib/Interpreter/Interpreter.cpp:223
 
+llvm::Error Interpreter::CreateExecutor() {
+  const clang::TargetInfo  =

argentite wrote:
> sgraenitz wrote:
> > Factoring out this function looks like an independent change. Is it related 
> > to the load-library command in any way?
> The `IncrementalExecutor` was being created on the first call to `Execute()`. 
> It is created lazily for `-fsyntax-only` support. It holds the Execution 
> Engine that we need for loading the library. So we need to be able to create 
> it in `getExecutionEngine()` as well because a library can be loaded before 
> any code is executed.
Of course, right. I had missed the second use. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141824

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

[PATCH] D137944: [ObjC][ARC] Teach the OptimizeSequences step of ObjCARCOpts about WinEH funclet tokens

[Stefan Gränitz via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
sgraenitz marked an inline comment as done.
Closed by commit rGd9eece916a8a: [ObjC][ARC] Teach the OptimizeSequences step 
of ObjCARCOpts about WinEH funclet… (authored by sgraenitz).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137944

Files:
  clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
  llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
  llvm/test/Transforms/ObjCARC/funclet-catchpad.ll

Index: llvm/test/Transforms/ObjCARC/funclet-catchpad.ll
===
--- /dev/null
+++ llvm/test/Transforms/ObjCARC/funclet-catchpad.ll
@@ -0,0 +1,40 @@
+; RUN: opt -mtriple=x86_64-windows-msvc -passes=objc-arc -S < %s | FileCheck %s
+
+; Check that funclet tokens are preserved
+;
+; CHECK-LABEL:  catch:
+; CHECK:  %1 = catchpad within %0
+; CHECK:  %2 = tail call ptr @llvm.objc.retain(ptr %exn) #0 [ "funclet"(token %1) ]
+; CHECK:  call void @llvm.objc.release(ptr %exn) #0 [ "funclet"(token %1) ]
+; CHECK:  catchret from %1 to label %eh.cont
+
+define void @try_catch_with_objc_intrinsic() personality ptr @__CxxFrameHandler3 {
+entry:
+  %exn.slot = alloca ptr, align 8
+  invoke void @may_throw(ptr null) to label %eh.cont unwind label %catch.dispatch
+
+catch.dispatch:   ; preds = %entry
+  %0 = catchswitch within none [label %catch] unwind to caller
+
+eh.cont:  ; preds = %catch, %entry
+  ret void
+
+catch:; preds = %catch.dispatch
+  %1 = catchpad within %0 [ptr null, i32 0, ptr %exn.slot]
+  br label %if.then
+
+if.then:  ; preds = %catch
+  %exn = load ptr, ptr null, align 8
+  %2 = call ptr @llvm.objc.retain(ptr %exn) [ "funclet"(token %1) ]
+  call void @may_throw(ptr %exn)
+  call void @llvm.objc.release(ptr %exn) [ "funclet"(token %1) ]
+  catchret from %1 to label %eh.cont
+}
+
+declare void @may_throw(ptr)
+declare i32 @__CxxFrameHandler3(...)
+
+declare ptr @llvm.objc.retain(ptr) #0
+declare void @llvm.objc.release(ptr) #0
+
+attributes #0 = { nounwind }
Index: llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
===
--- llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
+++ llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
@@ -501,6 +501,8 @@
   /// is in fact used in the current function.
   unsigned UsedInThisFunction;
 
+  DenseMap BlockEHColors;
+
   bool OptimizeRetainRVCall(Function , Instruction *RetainRV);
   void OptimizeAutoreleaseRVCall(Function , Instruction *AutoreleaseRV,
  ARCInstKind );
@@ -508,17 +510,16 @@
 
   /// Optimize an individual call, optionally passing the
   /// GetArgRCIdentityRoot if it has already been computed.
-  void OptimizeIndividualCallImpl(
-  Function , DenseMap ,
-  Instruction *Inst, ARCInstKind Class, const Value *Arg);
+  void OptimizeIndividualCallImpl(Function , Instruction *Inst,
+  ARCInstKind Class, const Value *Arg);
 
   /// Try to optimize an AutoreleaseRV with a RetainRV or UnsafeClaimRV.  If the
   /// optimization occurs, returns true to indicate that the caller should
   /// assume the instructions are dead.
-  bool OptimizeInlinedAutoreleaseRVCall(
-  Function , DenseMap ,
-  Instruction *Inst, const Value *, ARCInstKind Class,
-  Instruction *AutoreleaseRV, const Value *);
+  bool OptimizeInlinedAutoreleaseRVCall(Function , Instruction *Inst,
+const Value *, ARCInstKind Class,
+Instruction *AutoreleaseRV,
+const Value *);
 
   void CheckForCFGHazards(const BasicBlock *BB,
   DenseMap ,
@@ -566,12 +567,46 @@
 
   void OptimizeReturns(Function );
 
+  Instruction *cloneCallInstForBB(CallInst , BasicBlock ) {
+SmallVector OpBundles;
+for (unsigned I = 0, E = CI.getNumOperandBundles(); I != E; ++I) {
+  auto Bundle = CI.getOperandBundleAt(I);
+  // Funclets will be reassociated in the future.
+  if (Bundle.getTagID() == LLVMContext::OB_funclet)
+continue;
+  OpBundles.emplace_back(Bundle);
+}
+
+if (!BlockEHColors.empty()) {
+  const ColorVector  = BlockEHColors.find()->second;
+  assert(CV.size() > 0 && "non-unique color for block!");
+  Instruction *EHPad = CV.front()->getFirstNonPHI();
+  if (EHPad->isEHPad())
+OpBundles.emplace_back("funclet", EHPad);
+}
+
+return CallInst::Create(, OpBundles);
+  }
+
+  void addOpBundleForFunclet(BasicBlock *BB,
+ SmallVectorImpl ) {
+if (!BlockEHColors.empty()) {
+  const ColorVector  = 

[PATCH] D137944: [ObjC][ARC] Teach the OptimizeSequences step of ObjCARCOpts about WinEH funclet tokens

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz marked an inline comment as done.
sgraenitz added inline comments.



Comment at: llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp:597
+  assert(CV.size() > 0 && "Uncolored block");
+  for (BasicBlock *EHPadBB : CV)
+if (auto *EHPad = dyn_cast(EHPadBB->getFirstNonPHI())) 
{

ahatanak wrote:
> This piece of code does something similar to `cloneCallInstForBB`, but it's 
> slightly different from it. It iterates over the entries in `CV` whereas the 
> code above just looks at the first entry. 
> 
> Is this a bug that is fixed in https://reviews.llvm.org/D137945?
Yes, D137945 unifies the behavior so that we will always iterate. The unique 
color invariant only holds after WinEHPrepare. So the old assertion in 
`cloneCallInstForBB()` was formally wrong, but I only ever observed cases of 
non-unique coloring in incorrect IR, i.e. dangling funclet tokens before 
D137939. Iteration is the safe solution, especially since we may not be able to 
bail out on dangling funclet tokens in the verifier (see 
https://reviews.llvm.org/D138123#4067201).

FYI: We discussed it this in a previous round of this review 
https://reviews.llvm.org/D137944?id=475132#inline-1334823



Comment at: llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp:751
-const ColorVector  = BlockColors.find()->second;
-assert(CV.size() == 1 && "non-unique color for block!");
-Instruction *EHPad = CV.front()->getFirstNonPHI();

The old code here assumed all EH coloring to be unique.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137944

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

[PATCH] D141824: [clang-repl] Add a command to load dynamic libraries

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

Thanks for working on this! It looks like this is going to be the clang-repl 
equivalent for Cling's `.L` command. Do you think we can get test coverage for 
it?

So far, the only tests I found for `libclangInterpreter` are unit tests in 
https://github.com/llvm/llvm-project/tree/main/clang/unittests/Interpreter -- 
e.g. there is a test for the `%undo` command. I guess the `%lib` command would 
be a good candidate for a LIT test, because it requires a dynamic library to 
load from disk. The tests for `.L` in Cling may give you a hint on how this 
could look like: 
https://github.com/root-project/cling/tree/master/test/LibraryCall




Comment at: clang/include/clang/Interpreter/Interpreter.h:62
   const CompilerInstance *getCompilerInstance() const;
-  const llvm::orc::LLJIT *getExecutionEngine() const;
+  llvm::Expected GetExecutionEngine();
+

Do we really want these functions capitalized? IIRC in the clang code-base they 
are used to highlight core API functions. It looks like these two functions are 
only used as helpers inside `Interpreter` itself.



Comment at: clang/include/clang/Interpreter/Interpreter.h:78
 
+  /// Link a dyanmic library
+  llvm::Error LoadDynamicLibrary(const char *name);

Typo `dynamic`



Comment at: clang/lib/Interpreter/Interpreter.cpp:223
 
+llvm::Error Interpreter::CreateExecutor() {
+  const clang::TargetInfo  =

Factoring out this function looks like an independent change. Is it related to 
the load-library command in any way?



Comment at: clang/tools/clang-repl/ClangRepl.cpp:126
   }
+  if (Line->rfind(R"(%lib )", 0) == 0) {
+if (auto Err = Interp->LoadDynamicLibrary(Line->data() + 5)) {

I see this is analog to existing code, but is there a good reason for using the 
multi-line string syntax here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141824

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

[PATCH] D137944: [ObjC][ARC] Teach the OptimizeSequences step of ObjCARCOpts about WinEH funclet tokens

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

This review belongs to a series of patches. I am planning to land  it towards 
the end of next week, so that it's ready for the next release branching in 
February. If you have any more remarks, please let me know soon.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137944

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

[PATCH] D137944: [ObjC][ARC] Teach the OptimizeSequences step of ObjCARCOpts about WinEH funclet tokens

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137944

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

[PATCH] D137944: [ObjC][ARC] Teach the OptimizeSequences step of ObjCARCOpts about WinEH funclet tokens

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

Thanks for taking a look. That' really useful feedback! Yes, the coloring can 
be expensive and we shouldn't run it more than once.

and there's more places indeed

until it either stops making changes or

  // no retain+release pair nesting is detected




Comment at: llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp:767
+const ColorVector  = BlockColors.find(BB)->second;
+assert(CV.size() == 1 && "non-unique color for block!");
+BasicBlock *EHPadBB = CV.front();

ahatanak wrote:
> sgraenitz wrote:
> > rnk wrote:
> > > I don't think the verifier changes you made guarantee this. I would 
> > > consider strengthening this to `report_fatal_error`, since valid IR 
> > > transforms can probably reach this code.
> > Right, I guess the Verifier change is correct and this should change to 
> > work for multi-color BBs?
> > ```
> >   assert(CV.size() > 0 && "Uncolored block");
> >   for (BasicBlock *EHPadBB : CV)
> > if (auto *EHPad = 
> > dyn_cast(ColorFirstBB->getFirstNonPHI())) {
> >   OpBundles.emplace_back("funclet", EHPad);
> >   break;
> > }
> > ```
> > 
> > There is no test that covers this case, but it appears that the unique 
> > color invariant only holds **after** WinEHPrepare. [The assertion 
> > here](https://github.com/llvm/llvm-project/blob/release/15.x/llvm/lib/CodeGen/WinEHPrepare.cpp#L185)
> >  seems to imply it:
> > ```
> > assert(BBColors.size() == 1 && "multi-color BB not removed by preparation");
> > ```
> > 
> > Since it would be equivalent to the Verifier check, I'd stick with the 
> > assertion and not report a fatal error.
> Is the assertion `assert(CV.size() == 1 && "non-unique color for block!");` 
> in `CloneCallInstForBB` incorrect too?
The code path disappears with the subsequent patch D137945. But yes, it's 
formally incorrect and I adjusted the assertion in the last iteration.



Comment at: llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp:2040-2041
+  DenseMap BlockColors;
+  if (F.hasPersonalityFn() &&
+  isScopedEHPersonality(classifyEHPersonality(F.getPersonalityFn(
+BlockColors = colorEHFunclets(F);

ahatanak wrote:
> sgraenitz wrote:
> > rnk wrote:
> > > I think this code snippet probably deserves a Function helper, 
> > > `F->hasScopedEHPersonality()`. Another part of me thinks we should cache 
> > > the personality enum similar to the way we cache intrinsic ids, but I 
> > > wouldn't want to increase Function memory usage, so that seems premature. 
> > > "No action required", I guess.
> > It doesn't really fit the the scope of this patch but I am happy to add the 
> > helper function in a follow-up (for now without personality enum caching).
> `OptimizeIndividualCall` calls `colorEHFunclets` too. Is it possible to just 
> call it once? Or we don't care because it's not expensive?
Good catch! Right, we shouldn't do it multiple times. Having moved funclet 
coloring into `ObjCARCOpt::init()` it now runs only once.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137944

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

[PATCH] D137944: [ObjC][ARC] Teach the OptimizeSequences step of ObjCARCOpts about WinEH funclet tokens

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz updated this revision to Diff 483745.
sgraenitz marked 2 inline comments as done.
sgraenitz added a comment.

Address feedback


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137944

Files:
  clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
  llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
  llvm/test/Transforms/ObjCARC/funclet-catchpad.ll

Index: llvm/test/Transforms/ObjCARC/funclet-catchpad.ll
===
--- /dev/null
+++ llvm/test/Transforms/ObjCARC/funclet-catchpad.ll
@@ -0,0 +1,40 @@
+; RUN: opt -mtriple=x86_64-windows-msvc -passes=objc-arc -S < %s | FileCheck %s
+
+; Check that funclet tokens are preserved
+;
+; CHECK-LABEL:  catch:
+; CHECK:  %1 = catchpad within %0
+; CHECK:  %2 = tail call ptr @llvm.objc.retain(ptr %exn) #0 [ "funclet"(token %1) ]
+; CHECK:  call void @llvm.objc.release(ptr %exn) #0 [ "funclet"(token %1) ]
+; CHECK:  catchret from %1 to label %eh.cont
+
+define void @try_catch_with_objc_intrinsic() personality ptr @__CxxFrameHandler3 {
+entry:
+  %exn.slot = alloca ptr, align 8
+  invoke void @may_throw(ptr null) to label %eh.cont unwind label %catch.dispatch
+
+catch.dispatch:   ; preds = %entry
+  %0 = catchswitch within none [label %catch] unwind to caller
+
+eh.cont:  ; preds = %catch, %entry
+  ret void
+
+catch:; preds = %catch.dispatch
+  %1 = catchpad within %0 [ptr null, i32 0, ptr %exn.slot]
+  br label %if.then
+
+if.then:  ; preds = %catch
+  %exn = load ptr, ptr null, align 8
+  %2 = call ptr @llvm.objc.retain(ptr %exn) [ "funclet"(token %1) ]
+  call void @may_throw(ptr %exn)
+  call void @llvm.objc.release(ptr %exn) [ "funclet"(token %1) ]
+  catchret from %1 to label %eh.cont
+}
+
+declare void @may_throw(ptr)
+declare i32 @__CxxFrameHandler3(...)
+
+declare ptr @llvm.objc.retain(ptr) #0
+declare void @llvm.objc.release(ptr) #0
+
+attributes #0 = { nounwind }
Index: llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
===
--- llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
+++ llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
@@ -502,6 +502,8 @@
   /// is in fact used in the current function.
   unsigned UsedInThisFunction;
 
+  DenseMap BlockEHColors;
+
   bool OptimizeRetainRVCall(Function , Instruction *RetainRV);
   void OptimizeAutoreleaseRVCall(Function , Instruction *AutoreleaseRV,
  ARCInstKind );
@@ -509,17 +511,16 @@
 
   /// Optimize an individual call, optionally passing the
   /// GetArgRCIdentityRoot if it has already been computed.
-  void OptimizeIndividualCallImpl(
-  Function , DenseMap ,
-  Instruction *Inst, ARCInstKind Class, const Value *Arg);
+  void OptimizeIndividualCallImpl(Function , Instruction *Inst,
+  ARCInstKind Class, const Value *Arg);
 
   /// Try to optimize an AutoreleaseRV with a RetainRV or UnsafeClaimRV.  If the
   /// optimization occurs, returns true to indicate that the caller should
   /// assume the instructions are dead.
-  bool OptimizeInlinedAutoreleaseRVCall(
-  Function , DenseMap ,
-  Instruction *Inst, const Value *, ARCInstKind Class,
-  Instruction *AutoreleaseRV, const Value *);
+  bool OptimizeInlinedAutoreleaseRVCall(Function , Instruction *Inst,
+const Value *, ARCInstKind Class,
+Instruction *AutoreleaseRV,
+const Value *);
 
   void CheckForCFGHazards(const BasicBlock *BB,
   DenseMap ,
@@ -567,12 +568,46 @@
 
   void OptimizeReturns(Function );
 
+  Instruction *cloneCallInstForBB(CallInst , BasicBlock ) {
+SmallVector OpBundles;
+for (unsigned I = 0, E = CI.getNumOperandBundles(); I != E; ++I) {
+  auto Bundle = CI.getOperandBundleAt(I);
+  // Funclets will be reassociated in the future.
+  if (Bundle.getTagID() == LLVMContext::OB_funclet)
+continue;
+  OpBundles.emplace_back(Bundle);
+}
+
+if (!BlockEHColors.empty()) {
+  const ColorVector  = BlockEHColors.find()->second;
+  assert(CV.size() > 0 && "non-unique color for block!");
+  Instruction *EHPad = CV.front()->getFirstNonPHI();
+  if (EHPad->isEHPad())
+OpBundles.emplace_back("funclet", EHPad);
+}
+
+return CallInst::Create(, OpBundles);
+  }
+
+  void addOpBundleForFunclet(BasicBlock *BB,
+ SmallVectorImpl ) {
+if (!BlockEHColors.empty()) {
+  const ColorVector  = BlockEHColors.find(BB)->second;
+  assert(CV.size() > 0 && "Uncolored block");
+  for (BasicBlock *EHPadBB : CV)
+if (auto *EHPad = 

[PATCH] D137944: [ObjC][ARC] Teach the OptimizeSequences step of ObjCARCOpts about WinEH funclet tokens

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

This is ready to land and I'd appreciate feedback from one of the authors. 
Anyone else I should add for review?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137944

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

[PATCH] D137944: [ObjC][ARC] Teach the OptimizeSequences step of ObjCARCOpts about WinEH funclet tokens

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz updated this revision to Diff 477471.
sgraenitz added a comment.

Handle potential multi-color basic blocks in addOpBundleForFunclet()


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137944

Files:
  clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
  llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
  llvm/test/Transforms/ObjCARC/funclet-catchpad.ll

Index: llvm/test/Transforms/ObjCARC/funclet-catchpad.ll
===
--- /dev/null
+++ llvm/test/Transforms/ObjCARC/funclet-catchpad.ll
@@ -0,0 +1,40 @@
+; RUN: opt -mtriple=x86_64-windows-msvc -passes=objc-arc -S < %s | FileCheck %s
+
+; Check that funclet tokens are preserved
+;
+; CHECK-LABEL:  catch:
+; CHECK:  %1 = catchpad within %0
+; CHECK:  %2 = tail call ptr @llvm.objc.retain(ptr %exn) #0 [ "funclet"(token %1) ]
+; CHECK:  call void @llvm.objc.release(ptr %exn) #0 [ "funclet"(token %1) ]
+; CHECK:  catchret from %1 to label %eh.cont
+
+define void @try_catch_with_objc_intrinsic() personality ptr @__CxxFrameHandler3 {
+entry:
+  %exn.slot = alloca ptr, align 8
+  invoke void @may_throw(ptr null) to label %eh.cont unwind label %catch.dispatch
+
+catch.dispatch:   ; preds = %entry
+  %0 = catchswitch within none [label %catch] unwind to caller
+
+eh.cont:  ; preds = %catch, %entry
+  ret void
+
+catch:; preds = %catch.dispatch
+  %1 = catchpad within %0 [ptr null, i32 0, ptr %exn.slot]
+  br label %if.then
+
+if.then:  ; preds = %catch
+  %exn = load ptr, ptr null, align 8
+  %2 = call ptr @llvm.objc.retain(ptr %exn) [ "funclet"(token %1) ]
+  call void @may_throw(ptr %exn)
+  call void @llvm.objc.release(ptr %exn) [ "funclet"(token %1) ]
+  catchret from %1 to label %eh.cont
+}
+
+declare void @may_throw(ptr)
+declare i32 @__CxxFrameHandler3(...)
+
+declare ptr @llvm.objc.retain(ptr) #0
+declare void @llvm.objc.release(ptr) #0
+
+attributes #0 = { nounwind }
Index: llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
===
--- llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
+++ llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
@@ -546,7 +546,9 @@
   void MoveCalls(Value *Arg, RRInfo , RRInfo ,
  BlotMapVector ,
  DenseMap ,
- SmallVectorImpl , Module *M);
+ SmallVectorImpl ,
+ const DenseMap ,
+ Module *M);
 
   bool PairUpRetainsAndReleases(DenseMap ,
 BlotMapVector ,
@@ -559,7 +561,7 @@
 
   bool PerformCodePlacement(DenseMap ,
 BlotMapVector ,
-DenseMap , Module *M);
+DenseMap , Function );
 
   void OptimizeWeakCalls(Function );
 
@@ -756,6 +758,20 @@
 
   return CallInst::Create(, OpBundles);
 }
+
+void addOpBundleForFunclet(BasicBlock *BB,
+   const DenseMap ,
+   SmallVectorImpl ) {
+  if (!BlockColors.empty()) {
+const ColorVector  = BlockColors.find(BB)->second;
+assert(CV.size() > 0 && "Uncolored block");
+for (BasicBlock *EHPadBB : CV)
+  if (auto *EHPad = dyn_cast(EHPadBB->getFirstNonPHI())) {
+OpBundles.emplace_back("funclet", EHPad);
+return;
+  }
+  }
+}
 }
 
 /// Visit each call, one at a time, and make simplifications without doing any
@@ -1757,12 +1773,12 @@
 }
 
 /// Move the calls in RetainsToMove and ReleasesToMove.
-void ObjCARCOpt::MoveCalls(Value *Arg, RRInfo ,
-   RRInfo ,
-   BlotMapVector ,
-   DenseMap ,
-   SmallVectorImpl ,
-   Module *M) {
+void ObjCARCOpt::MoveCalls(
+Value *Arg, RRInfo , RRInfo ,
+BlotMapVector ,
+DenseMap ,
+SmallVectorImpl ,
+const DenseMap , Module *M) {
   Type *ArgTy = Arg->getType();
   Type *ParamTy = PointerType::getUnqual(Type::getInt8Ty(ArgTy->getContext()));
 
@@ -1773,7 +1789,9 @@
 Value *MyArg = ArgTy == ParamTy ? Arg :
new BitCastInst(Arg, ParamTy, "", InsertPt);
 Function *Decl = EP.get(ARCRuntimeEntryPointKind::Retain);
-CallInst *Call = CallInst::Create(Decl, MyArg, "", InsertPt);
+SmallVector BundleList;
+addOpBundleForFunclet(InsertPt->getParent(), BlockColors, BundleList);
+CallInst *Call = CallInst::Create(Decl, MyArg, BundleList, "", InsertPt);
 Call->setDoesNotThrow();
 Call->setTailCall();
 
@@ -1786,7 +1804,9 @@
 Value *MyArg = ArgTy == ParamTy ? Arg :
new BitCastInst(Arg, ParamTy, "", InsertPt);
 Function *Decl = EP.get(ARCRuntimeEntryPointKind::Release);
-CallInst *Call = CallInst::Create(Decl, 

[PATCH] D137944: [ObjC][ARC] Teach the OptimizeSequences step of ObjCARCOpts about WinEH funclet tokens

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz updated this revision to Diff 477470.
sgraenitz marked an inline comment as done.
sgraenitz added a comment.

Rebase and update check-lines after D137939 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137944

Files:
  clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
  llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
  llvm/test/Transforms/ObjCARC/funclet-catchpad.ll

Index: llvm/test/Transforms/ObjCARC/funclet-catchpad.ll
===
--- /dev/null
+++ llvm/test/Transforms/ObjCARC/funclet-catchpad.ll
@@ -0,0 +1,40 @@
+; RUN: opt -mtriple=x86_64-windows-msvc -passes=objc-arc -S < %s | FileCheck %s
+
+; Check that funclet tokens are preserved
+;
+; CHECK-LABEL:  catch:
+; CHECK:  %1 = catchpad within %0
+; CHECK:  %2 = tail call ptr @llvm.objc.retain(ptr %exn) #0 [ "funclet"(token %1) ]
+; CHECK:  call void @llvm.objc.release(ptr %exn) #0 [ "funclet"(token %1) ]
+; CHECK:  catchret from %1 to label %eh.cont
+
+define void @try_catch_with_objc_intrinsic() personality ptr @__CxxFrameHandler3 {
+entry:
+  %exn.slot = alloca ptr, align 8
+  invoke void @may_throw(ptr null) to label %eh.cont unwind label %catch.dispatch
+
+catch.dispatch:   ; preds = %entry
+  %0 = catchswitch within none [label %catch] unwind to caller
+
+eh.cont:  ; preds = %catch, %entry
+  ret void
+
+catch:; preds = %catch.dispatch
+  %1 = catchpad within %0 [ptr null, i32 0, ptr %exn.slot]
+  br label %if.then
+
+if.then:  ; preds = %catch
+  %exn = load ptr, ptr null, align 8
+  %2 = call ptr @llvm.objc.retain(ptr %exn) [ "funclet"(token %1) ]
+  call void @may_throw(ptr %exn)
+  call void @llvm.objc.release(ptr %exn) [ "funclet"(token %1) ]
+  catchret from %1 to label %eh.cont
+}
+
+declare void @may_throw(ptr)
+declare i32 @__CxxFrameHandler3(...)
+
+declare ptr @llvm.objc.retain(ptr) #0
+declare void @llvm.objc.release(ptr) #0
+
+attributes #0 = { nounwind }
Index: llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
===
--- llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
+++ llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
@@ -546,7 +546,9 @@
   void MoveCalls(Value *Arg, RRInfo , RRInfo ,
  BlotMapVector ,
  DenseMap ,
- SmallVectorImpl , Module *M);
+ SmallVectorImpl ,
+ const DenseMap ,
+ Module *M);
 
   bool PairUpRetainsAndReleases(DenseMap ,
 BlotMapVector ,
@@ -559,7 +561,7 @@
 
   bool PerformCodePlacement(DenseMap ,
 BlotMapVector ,
-DenseMap , Module *M);
+DenseMap , Function );
 
   void OptimizeWeakCalls(Function );
 
@@ -756,6 +758,18 @@
 
   return CallInst::Create(, OpBundles);
 }
+
+void addOpBundleForFunclet(BasicBlock *BB,
+   const DenseMap ,
+   SmallVectorImpl ) {
+  if (!BlockColors.empty()) {
+const ColorVector  = BlockColors.find(BB)->second;
+assert(CV.size() == 1 && "non-unique color for block!");
+BasicBlock *EHPadBB = CV.front();
+if (auto *EHPad = dyn_cast(EHPadBB->getFirstNonPHI()))
+  OpBundles.emplace_back("funclet", EHPad);
+  }
+}
 }
 
 /// Visit each call, one at a time, and make simplifications without doing any
@@ -1757,12 +1771,12 @@
 }
 
 /// Move the calls in RetainsToMove and ReleasesToMove.
-void ObjCARCOpt::MoveCalls(Value *Arg, RRInfo ,
-   RRInfo ,
-   BlotMapVector ,
-   DenseMap ,
-   SmallVectorImpl ,
-   Module *M) {
+void ObjCARCOpt::MoveCalls(
+Value *Arg, RRInfo , RRInfo ,
+BlotMapVector ,
+DenseMap ,
+SmallVectorImpl ,
+const DenseMap , Module *M) {
   Type *ArgTy = Arg->getType();
   Type *ParamTy = PointerType::getUnqual(Type::getInt8Ty(ArgTy->getContext()));
 
@@ -1773,7 +1787,9 @@
 Value *MyArg = ArgTy == ParamTy ? Arg :
new BitCastInst(Arg, ParamTy, "", InsertPt);
 Function *Decl = EP.get(ARCRuntimeEntryPointKind::Retain);
-CallInst *Call = CallInst::Create(Decl, MyArg, "", InsertPt);
+SmallVector BundleList;
+addOpBundleForFunclet(InsertPt->getParent(), BlockColors, BundleList);
+CallInst *Call = CallInst::Create(Decl, MyArg, BundleList, "", InsertPt);
 Call->setDoesNotThrow();
 Call->setTailCall();
 
@@ -1786,7 +1802,9 @@
 Value *MyArg = ArgTy == ParamTy ? Arg :
new BitCastInst(Arg, ParamTy, "", InsertPt);
 Function *Decl = EP.get(ARCRuntimeEntryPointKind::Release);
-

[PATCH] D138122: Lift EHPersonalities from Analysis to IR (NFC)

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz updated this revision to Diff 477433.
sgraenitz added a comment.

clang-format includes and sort clang-formatted-files.txt


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138122

Files:
  clang/docs/tools/clang-formatted-files.txt
  llvm/include/llvm/Analysis/EHPersonalities.h
  llvm/include/llvm/Analysis/MustExecute.h
  llvm/include/llvm/CodeGen/MachineFunction.h
  llvm/include/llvm/IR/EHPersonalities.h
  llvm/lib/Analysis/CMakeLists.txt
  llvm/lib/Analysis/EHPersonalities.cpp
  llvm/lib/Analysis/ValueTracking.cpp
  llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
  llvm/lib/CodeGen/DwarfEHPrepare.cpp
  llvm/lib/CodeGen/MachineFunction.cpp
  llvm/lib/CodeGen/MachineVerifier.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
  llvm/lib/CodeGen/StackProtector.cpp
  llvm/lib/CodeGen/WinEHPrepare.cpp
  llvm/lib/IR/CMakeLists.txt
  llvm/lib/IR/EHPersonalities.cpp
  llvm/lib/Target/M68k/M68kCollapseMOVEMPass.cpp
  llvm/lib/Target/M68k/M68kExpandPseudo.cpp
  llvm/lib/Target/X86/X86ExpandPseudo.cpp
  llvm/lib/Target/X86/X86FrameLowering.cpp
  llvm/lib/Target/X86/X86ISelLowering.cpp
  llvm/lib/Target/X86/X86WinEHState.cpp
  llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
  llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
  llvm/lib/Transforms/ObjCARC/ObjCARC.h
  llvm/lib/Transforms/ObjCARC/ObjCARCContract.cpp
  llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
  llvm/lib/Transforms/Utils/EscapeEnumerator.cpp
  llvm/lib/Transforms/Utils/InlineFunction.cpp
  llvm/lib/Transforms/Utils/Local.cpp
  llvm/utils/gn/secondary/llvm/lib/Analysis/BUILD.gn
  llvm/utils/gn/secondary/llvm/lib/IR/BUILD.gn

Index: llvm/utils/gn/secondary/llvm/lib/IR/BUILD.gn
===
--- llvm/utils/gn/secondary/llvm/lib/IR/BUILD.gn
+++ llvm/utils/gn/secondary/llvm/lib/IR/BUILD.gn
@@ -33,6 +33,7 @@
 "DiagnosticInfo.cpp",
 "DiagnosticPrinter.cpp",
 "Dominators.cpp",
+"EHPersonalities.cpp",
 "FPEnv.cpp",
 "Function.cpp",
 "GCStrategy.cpp",
Index: llvm/utils/gn/secondary/llvm/lib/Analysis/BUILD.gn
===
--- llvm/utils/gn/secondary/llvm/lib/Analysis/BUILD.gn
+++ llvm/utils/gn/secondary/llvm/lib/Analysis/BUILD.gn
@@ -50,7 +50,6 @@
 "DomPrinter.cpp",
 "DomTreeUpdater.cpp",
 "DominanceFrontier.cpp",
-"EHPersonalities.cpp",
 "FunctionPropertiesAnalysis.cpp",
 "GlobalsModRef.cpp",
 "GuardUtils.cpp",
Index: llvm/lib/Transforms/Utils/Local.cpp
===
--- llvm/lib/Transforms/Utils/Local.cpp
+++ llvm/lib/Transforms/Utils/Local.cpp
@@ -27,7 +27,6 @@
 #include "llvm/Analysis/AssumeBundleQueries.h"
 #include "llvm/Analysis/ConstantFolding.h"
 #include "llvm/Analysis/DomTreeUpdater.h"
-#include "llvm/Analysis/EHPersonalities.h"
 #include "llvm/Analysis/InstructionSimplify.h"
 #include "llvm/Analysis/MemoryBuiltins.h"
 #include "llvm/Analysis/MemorySSAUpdater.h"
@@ -49,6 +48,7 @@
 #include "llvm/IR/DebugLoc.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/Dominators.h"
+#include "llvm/IR/EHPersonalities.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GetElementPtrTypeIterator.h"
 #include "llvm/IR/GlobalObject.h"
Index: llvm/lib/Transforms/Utils/InlineFunction.cpp
===
--- llvm/lib/Transforms/Utils/InlineFunction.cpp
+++ llvm/lib/Transforms/Utils/InlineFunction.cpp
@@ -25,7 +25,6 @@
 #include "llvm/Analysis/BlockFrequencyInfo.h"
 #include "llvm/Analysis/CallGraph.h"
 #include "llvm/Analysis/CaptureTracking.h"
-#include "llvm/Analysis/EHPersonalities.h"
 #include "llvm/Analysis/InstructionSimplify.h"
 #include "llvm/Analysis/MemoryProfileInfo.h"
 #include "llvm/Analysis/ObjCARCAnalysisUtils.h"
@@ -44,6 +43,7 @@
 #include "llvm/IR/DebugLoc.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/Dominators.h"
+#include "llvm/IR/EHPersonalities.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/InlineAsm.h"
Index: llvm/lib/Transforms/Utils/EscapeEnumerator.cpp
===
--- llvm/lib/Transforms/Utils/EscapeEnumerator.cpp
+++ llvm/lib/Transforms/Utils/EscapeEnumerator.cpp
@@ -13,7 +13,7 @@
 
 #include "llvm/Transforms/Utils/EscapeEnumerator.h"
 #include "llvm/ADT/Triple.h"
-#include "llvm/Analysis/EHPersonalities.h"
+#include "llvm/IR/EHPersonalities.h"
 #include "llvm/IR/Module.h"
 #include "llvm/Transforms/Utils/Local.h"
 
Index: llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
===
--- llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
+++ 

[PATCH] D137942: [CGObjC] Add run line for release mode in test arc-exceptions-seh.mm (NFC)

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

The Buildbot has detected a failed build on builder 
sanitizer-aarch64-linux-bootstrap-ubsan while building clang,compiler-rt. I 
will investigate tomorrow and reverted the change in the meantime. Full error 
output:

   TEST 'Clang :: CodeGenObjCXX/arc-exceptions-seh.mm' 
FAILED 
  Script:
  --
  : 'RUN: at line 1';   
/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm_build_ubsan/bin/clang 
-cc1 -internal-isystem 
/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm_build_ubsan/lib/clang/16/include
 -nostdsysteminc -triple x86_64-pc-windows-msvc -emit-llvm -fobjc-arc 
-fexceptions -fobjc-exceptions -fobjc-arc-exceptions -fobjc-runtime=gnustep-2.0 
-o - 
/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
 | 
/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm_build_ubsan/bin/FileCheck 
/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
 --check-prefixes=CHECK,CHECK-O0
  : 'RUN: at line 2';   
/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm_build_ubsan/bin/clang 
-cc1 -internal-isystem 
/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm_build_ubsan/lib/clang/16/include
 -nostdsysteminc -O2 -triple x86_64-pc-windows-msvc -emit-llvm -fobjc-arc 
-fexceptions -fobjc-exceptions -fobjc-arc-exceptions -fobjc-runtime=gnustep-2.0 
-mllvm -enable-objc-arc-opts=false -o - 
/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
 | 
/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm_build_ubsan/bin/FileCheck 
/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
 --check-prefixes=CHECK,CHECK-O2
  --
  Exit Code: 2
  
  Command Output (stderr):
  --
  llvm-project/llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp:577:41: runtime 
error: load of value 180, which is not a valid value for type 'bool'
  #0 0xea30 in hasCFGChanged 
llvm-project/llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp:577:41
  #1 0xea30 in llvm::ObjCARCOptPass::run(llvm::Function&, 
llvm::AnalysisManager&) 
llvm-project/llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp:2494:26
  #2 0xe202100c in llvm::PassManager>::run(llvm::Function&, 
llvm::AnalysisManager&) 
llvm-project/llvm/include/llvm/IR/PassManager.h:517:40
  #3 0xe1557168 in 
llvm::CGSCCToFunctionPassAdaptor::run(llvm::LazyCallGraph::SCC&, 
llvm::AnalysisManager&, 
llvm::LazyCallGraph&, llvm::CGSCCUpdateResult&) 
llvm-project/llvm/lib/Analysis/CGSCCPassManager.cpp:541:38
  #4 0xe1552b10 in llvm::PassManager, 
llvm::LazyCallGraph&, llvm::CGSCCUpdateResult&>::run(llvm::LazyCallGraph::SCC&, 
llvm::AnalysisManager&, 
llvm::LazyCallGraph&, llvm::CGSCCUpdateResult&) 
llvm-project/llvm/lib/Analysis/CGSCCPassManager.cpp:87:38
  #5 0xe155587c in 
llvm::DevirtSCCRepeatedPass::run(llvm::LazyCallGraph::SCC&, 
llvm::AnalysisManager&, 
llvm::LazyCallGraph&, llvm::CGSCCUpdateResult&) 
llvm-project/llvm/lib/Analysis/CGSCCPassManager.cpp:409:38
  #6 0xe15540b8 in 
llvm::ModuleToPostOrderCGSCCPassAdaptor::run(llvm::Module&, 
llvm::AnalysisManager&) 
llvm-project/llvm/lib/Analysis/CGSCCPassManager.cpp:277:44
  #7 0xe20204a8 in llvm::PassManager>::run(llvm::Module&, 
llvm::AnalysisManager&) 
llvm-project/llvm/include/llvm/IR/PassManager.h:517:40
  #8 0xe21794cc in llvm::ModuleInlinerWrapperPass::run(llvm::Module&, 
llvm::AnalysisManager&) 
llvm-project/llvm/lib/Transforms/IPO/Inliner.cpp:1170:7
  #9 0xe20204a8 in llvm::PassManager>::run(llvm::Module&, 
llvm::AnalysisManager&) 
llvm-project/llvm/include/llvm/IR/PassManager.h:517:40
  #10 0xe33186bc in (anonymous 
namespace)::EmitAssemblyHelper::RunOptimizationPipeline(clang::BackendAction, 
std::__1::unique_ptr>&, 
std::__1::unique_ptr>&) 
llvm-project/clang/lib/CodeGen/BackendUtil.cpp:1028:9
  #11 0xe3312ed8 in EmitAssembly 
llvm-project/clang/lib/CodeGen/BackendUtil.cpp:1085:3
  #12 0xe3312ed8 in clang::EmitBackendOutput(clang::DiagnosticsEngine&, 
clang::HeaderSearchOptions const&, clang::CodeGenOptions const&, 
clang::TargetOptions const&, clang::LangOptions const&, llvm::StringRef, 
llvm::Module*, clang::BackendAction, 
std::__1::unique_ptr>) 
llvm-project/clang/lib/CodeGen/BackendUtil.cpp:1244:13
  #13 0xe370da78 in 
clang::BackendConsumer::HandleTranslationUnit(clang::ASTContext&) 
llvm-project/clang/lib/CodeGen/CodeGenAction.cpp:381:7
  #14 0xe4ee63f8 in clang::ParseAST(clang::Sema&, bool, bool) 
llvm-project/clang/lib/Parse/ParseAST.cpp:196:13
  #15 0xe3614e64 in clang::FrontendAction::Execute() 
llvm-project/clang/lib/Frontend/FrontendAction.cpp:1055:8
  #16 0xe35acff4 in 
clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) 
llvm-project/clang/lib/Frontend/CompilerInstance.cpp:1045:33
  #17 0xe3707c84 

[PATCH] D137942: [CGObjC] Add run line for release mode in test arc-exceptions-seh.mm (NFC)

[Stefan Gränitz via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG01023bfcd33f: [CGObjC] Add run line for release mode in test 
arc-exceptions-seh.mm (NFC) (authored by sgraenitz).

Changed prior to commit:
  https://reviews.llvm.org/D137942?vs=475127=477140#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137942

Files:
  clang/test/CodeGenObjCXX/arc-exceptions-seh.mm


Index: clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
===
--- clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
+++ clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -emit-llvm -fobjc-arc 
-fexceptions -fobjc-exceptions -fobjc-arc-exceptions -fobjc-runtime=gnustep-2.0 
-o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -emit-llvm -fobjc-arc 
-fexceptions -fobjc-exceptions -fobjc-arc-exceptions -fobjc-runtime=gnustep-2.0 
-o - %s | FileCheck %s --check-prefixes=CHECK,CHECK-O0
+// RUN: %clang_cc1 -O2 -triple x86_64-pc-windows-msvc -emit-llvm -fobjc-arc 
-fexceptions -fobjc-exceptions -fobjc-arc-exceptions -fobjc-runtime=gnustep-2.0 
-mllvm -enable-objc-arc-opts=false -o - %s | FileCheck %s 
--check-prefixes=CHECK,CHECK-O2
 
 // WinEH requires funclet tokens on nounwind intrinsics if they can lower to
 // regular function calls in the course of IR transformations.
@@ -40,18 +41,21 @@
 // CHECK: [ "funclet"(token [[CATCHPAD]]) ]
 // CHECK: unwind label %[[CLEANUP2]]
 // CHECK:   call
-// CHECK: @llvm.objc.storeStrong
+// CHECK-O0:  @llvm.objc.storeStrong
+// CHECK-O2:  @llvm.objc.release
 // CHECK: [ "funclet"(token [[CATCHPAD]]) ]
-// CHECK:   catchret from [[CATCHPAD]] to label %catchret.dest
+// CHECK-O0:catchret from [[CATCHPAD]] to label %catchret.dest
+// CHECK-O2:catchret from [[CATCHPAD]] to label %eh.cont
 //
-// This block exists and it's empty:
-// CHECK: catchret.dest:
-// CHECK-NEXT:  br label %eh.cont
+// In debug mode, this block exists and it's empty:
+// CHECK-O0:  catchret.dest:
+// CHECK-O0-NEXT:   br label %eh.cont
 //
 // CHECK: [[CLEANUP2]]:
 // CHECK-NEXT:  [[CLEANUPPAD2:%[0-9]+]] = cleanuppad within [[CATCHPAD]]
 // CHECK:   call
-// CHECK: @llvm.objc.storeStrong
+// CHECK-O0:  @llvm.objc.storeStrong
+// CHECK-O2:  @llvm.objc.release
 // CHECK: [ "funclet"(token [[CLEANUPPAD2]]) ]
 // CHECK:   cleanupret from [[CLEANUPPAD2]]
 // CHECK: unwind label %[[CLEANUP1]]
@@ -59,6 +63,7 @@
 // CHECK: [[CLEANUP1]]:
 // CHECK-NEXT:  [[CLEANUPPAD1:%[0-9]+]] = cleanuppad within none
 // CHECK:   call
-// CHECK: @llvm.objc.storeStrong
+// CHECK-O0:  @llvm.objc.storeStrong
+// CHECK-O2:  @llvm.objc.release
 // CHECK: [ "funclet"(token [[CLEANUPPAD1]]) ]
 // CHECK:   cleanupret from [[CLEANUPPAD1]] unwind to caller


Index: clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
===
--- clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
+++ clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -emit-llvm -fobjc-arc -fexceptions -fobjc-exceptions -fobjc-arc-exceptions -fobjc-runtime=gnustep-2.0 -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -emit-llvm -fobjc-arc -fexceptions -fobjc-exceptions -fobjc-arc-exceptions -fobjc-runtime=gnustep-2.0 -o - %s | FileCheck %s --check-prefixes=CHECK,CHECK-O0
+// RUN: %clang_cc1 -O2 -triple x86_64-pc-windows-msvc -emit-llvm -fobjc-arc -fexceptions -fobjc-exceptions -fobjc-arc-exceptions -fobjc-runtime=gnustep-2.0 -mllvm -enable-objc-arc-opts=false -o - %s | FileCheck %s --check-prefixes=CHECK,CHECK-O2
 
 // WinEH requires funclet tokens on nounwind intrinsics if they can lower to
 // regular function calls in the course of IR transformations.
@@ -40,18 +41,21 @@
 // CHECK: [ "funclet"(token [[CATCHPAD]]) ]
 // CHECK: unwind label %[[CLEANUP2]]
 // CHECK:   call
-// CHECK: @llvm.objc.storeStrong
+// CHECK-O0:  @llvm.objc.storeStrong
+// CHECK-O2:  @llvm.objc.release
 // CHECK: [ "funclet"(token [[CATCHPAD]]) ]
-// CHECK:   catchret from [[CATCHPAD]] to label %catchret.dest
+// CHECK-O0:catchret from [[CATCHPAD]] to label %catchret.dest
+// CHECK-O2:catchret from [[CATCHPAD]] to label %eh.cont
 //
-// This block exists and it's empty:
-// CHECK: catchret.dest:
-// CHECK-NEXT:  br label %eh.cont
+// In debug mode, this block exists and it's empty:
+// CHECK-O0:  catchret.dest:
+// 

[PATCH] D137939: [CGObjC] Open cleanup scope before SaveAndRestore CurrentFuncletPad and push CatchRetScope early

[Stefan Gränitz via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG9a9d636caeea: [CGObjC] Open cleanup scope before 
SaveAndRestore CurrentFuncletPad and push… (authored by sgraenitz).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137939

Files:
  clang/lib/CodeGen/CGObjCRuntime.cpp
  clang/test/CodeGenObjCXX/arc-exceptions-seh.mm

Index: clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
===
--- clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
+++ clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
@@ -1,29 +1,64 @@
-// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -emit-llvm -fobjc-arc -fexceptions -fobjc-exceptions -fobjc-runtime=gnustep-2.0 -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -emit-llvm -fobjc-arc -fexceptions -fobjc-exceptions -fobjc-arc-exceptions -fobjc-runtime=gnustep-2.0 -o - %s | FileCheck %s
 
 // WinEH requires funclet tokens on nounwind intrinsics if they can lower to
 // regular function calls in the course of IR transformations.
 //
 // This is the case for ObjC ARC runtime intrinsics. Test that clang emits the
-// funclet tokens for llvm.objc.retain and llvm.objc.storeStrong and that they
-// refer to their catchpad's SSA value.
+// funclet tokens for llvm.objc.* calls inside catch- and cleanup-pads and that
+// they refer to their pad's SSA value.
 
-@class Ety;
-void opaque(void);
-void test_catch_with_objc_intrinsic(void) {
+void do_something();
+void may_throw(id);
+
+void try_catch_with_objc_intrinsic() {
+  id ex;
   @try {
-opaque();
-  } @catch (Ety *ex) {
-// Destroy ex when leaving catchpad. This emits calls to intrinsic functions
-// llvm.objc.retain and llvm.objc.storeStrong
+may_throw(ex);
+  } @catch (id ex_caught) {
+do_something();
+may_throw(ex_caught);
   }
 }
 
-// CHECK-LABEL: define{{.*}} void {{.*}}test_catch_with_objc_intrinsic
-//...
-// CHECK:   catch.dispatch:
-// CHECK-NEXT:[[CATCHSWITCH:%[0-9]+]] = catchswitch within none
-//...
-// CHECK:   catch:
-// CHECK-NEXT:[[CATCHPAD:%[0-9]+]] = catchpad within [[CATCHSWITCH]]
-// CHECK: {{%[0-9]+}} = call {{.*}} @llvm.objc.retain{{.*}} [ "funclet"(token [[CATCHPAD]]) ]
-// CHECK: call {{.*}} @llvm.objc.storeStrong{{.*}} [ "funclet"(token [[CATCHPAD]]) ]
+// CHECK-LABEL:   try_catch_with_objc_intrinsic
+//
+// CHECK: catch.dispatch:
+// CHECK-NEXT:  [[CATCHSWITCH:%[0-9]+]] = catchswitch within none [label %catch] unwind label %[[CLEANUP1:.*]]
+//
+// All calls within a catchpad must have funclet tokens that refer to it:
+// CHECK: catch:
+// CHECK-NEXT:  [[CATCHPAD:%[0-9]+]] = catchpad within [[CATCHSWITCH]]
+// CHECK:   call
+// CHECK: @llvm.objc.retain
+// CHECK: [ "funclet"(token [[CATCHPAD]]) ]
+// CHECK:   invoke
+// CHECK: do_something
+// CHECK: [ "funclet"(token [[CATCHPAD]]) ]
+// CHECK: unwind label %[[CLEANUP2:.*]]
+// CHECK:   invoke
+// CHECK: may_throw
+// CHECK: [ "funclet"(token [[CATCHPAD]]) ]
+// CHECK: unwind label %[[CLEANUP2]]
+// CHECK:   call
+// CHECK: @llvm.objc.storeStrong
+// CHECK: [ "funclet"(token [[CATCHPAD]]) ]
+// CHECK:   catchret from [[CATCHPAD]] to label %catchret.dest
+//
+// This block exists and it's empty:
+// CHECK: catchret.dest:
+// CHECK-NEXT:  br label %eh.cont
+//
+// CHECK: [[CLEANUP2]]:
+// CHECK-NEXT:  [[CLEANUPPAD2:%[0-9]+]] = cleanuppad within [[CATCHPAD]]
+// CHECK:   call
+// CHECK: @llvm.objc.storeStrong
+// CHECK: [ "funclet"(token [[CLEANUPPAD2]]) ]
+// CHECK:   cleanupret from [[CLEANUPPAD2]]
+// CHECK: unwind label %[[CLEANUP1]]
+//
+// CHECK: [[CLEANUP1]]:
+// CHECK-NEXT:  [[CLEANUPPAD1:%[0-9]+]] = cleanuppad within none
+// CHECK:   call
+// CHECK: @llvm.objc.storeStrong
+// CHECK: [ "funclet"(token [[CLEANUPPAD1]]) ]
+// CHECK:   cleanupret from [[CLEANUPPAD1]] unwind to caller
Index: clang/lib/CodeGen/CGObjCRuntime.cpp
===
--- clang/lib/CodeGen/CGObjCRuntime.cpp
+++ clang/lib/CodeGen/CGObjCRuntime.cpp
@@ -22,6 +22,7 @@
 #include "clang/AST/StmtObjC.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
 #include "clang/CodeGen/CodeGenABITypes.h"
+#include "llvm/IR/Instruction.h"
 #include "llvm/Support/SaveAndRestore.h"
 
 using namespace clang;
@@ -227,13 +228,18 @@
 CatchHandler  = Handlers[I];
 
 CGF.EmitBlock(Handler.Block);
-llvm::CatchPadInst *CPI = nullptr;
-SaveAndRestore RestoreCurrentFuncletPad(CGF.CurrentFuncletPad);
-if 

[PATCH] D137944: [ObjC][ARC] Teach the OptimizeSequences step of ObjCARCOpts about WinEH funclet tokens

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

Thanks for your feedback @rnk! I hope some of the ObjCARCOpts authors will 
share their opinions as well.




Comment at: llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp:767
+const ColorVector  = BlockColors.find(BB)->second;
+assert(CV.size() == 1 && "non-unique color for block!");
+BasicBlock *EHPadBB = CV.front();

rnk wrote:
> I don't think the verifier changes you made guarantee this. I would consider 
> strengthening this to `report_fatal_error`, since valid IR transforms can 
> probably reach this code.
Right, I guess the Verifier change is correct and this should change to work 
for multi-color BBs?
```
  assert(CV.size() > 0 && "Uncolored block");
  for (BasicBlock *EHPadBB : CV)
if (auto *EHPad = 
dyn_cast(ColorFirstBB->getFirstNonPHI())) {
  OpBundles.emplace_back("funclet", EHPad);
  break;
}
```

There is no test that covers this case, but it appears that the unique color 
invariant only holds **after** WinEHPrepare. [The assertion 
here](https://github.com/llvm/llvm-project/blob/release/15.x/llvm/lib/CodeGen/WinEHPrepare.cpp#L185)
 seems to imply it:
```
assert(BBColors.size() == 1 && "multi-color BB not removed by preparation");
```

Since it would be equivalent to the Verifier check, I'd stick with the 
assertion and not report a fatal error.



Comment at: llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp:2040-2041
+  DenseMap BlockColors;
+  if (F.hasPersonalityFn() &&
+  isScopedEHPersonality(classifyEHPersonality(F.getPersonalityFn(
+BlockColors = colorEHFunclets(F);

rnk wrote:
> I think this code snippet probably deserves a Function helper, 
> `F->hasScopedEHPersonality()`. Another part of me thinks we should cache the 
> personality enum similar to the way we cache intrinsic ids, but I wouldn't 
> want to increase Function memory usage, so that seems premature. "No action 
> required", I guess.
It doesn't really fit the the scope of this patch but I am happy to add the 
helper function in a follow-up (for now without personality enum caching).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137944

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

[PATCH] D137939: [CGObjC] Open cleanup scope before SaveAndRestore CurrentFuncletPad and push CatchRetScope early

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added inline comments.



Comment at: clang/test/CodeGenObjCXX/arc-exceptions-seh.mm:36
+// CHECK:   call
+// CHECK:   do_something
+// CHECK:   [ "funclet"(token [[CATCHPAD]]) ]

rnk wrote:
> Don't we need an exceptional cleanup here to release the exception if 
> `do_something` or `may_throw` throw?
Thanks, that's a very good point! `-fobjc-arc-exceptions` tells clang to 
generate invokes here instead of calls and then we can check funclet tokens in 
cleanup pads too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137939

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

[PATCH] D137939: [CGObjC] Open cleanup scope before SaveAndRestore CurrentFuncletPad and push CatchRetScope early

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz updated this revision to Diff 476150.
sgraenitz marked an inline comment as done.
sgraenitz added a comment.

Run test with -fobjc-arc-exceptions and check cleanup pads as well


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137939

Files:
  clang/lib/CodeGen/CGObjCRuntime.cpp
  clang/test/CodeGenObjCXX/arc-exceptions-seh.mm

Index: clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
===
--- clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
+++ clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
@@ -1,29 +1,64 @@
-// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -emit-llvm -fobjc-arc -fexceptions -fobjc-exceptions -fobjc-runtime=gnustep-2.0 -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -emit-llvm -fobjc-arc -fexceptions -fobjc-exceptions -fobjc-arc-exceptions -fobjc-runtime=gnustep-2.0 -o - %s | FileCheck %s
 
 // WinEH requires funclet tokens on nounwind intrinsics if they can lower to
 // regular function calls in the course of IR transformations.
 //
 // This is the case for ObjC ARC runtime intrinsics. Test that clang emits the
-// funclet tokens for llvm.objc.retain and llvm.objc.storeStrong and that they
-// refer to their catchpad's SSA value.
+// funclet tokens for llvm.objc.* calls inside catch- and cleanup-pads and that
+// they refer to their pad's SSA value.
 
-@class Ety;
-void opaque(void);
-void test_catch_with_objc_intrinsic(void) {
+void do_something();
+void may_throw(id);
+
+void try_catch_with_objc_intrinsic() {
+  id ex;
   @try {
-opaque();
-  } @catch (Ety *ex) {
-// Destroy ex when leaving catchpad. This emits calls to intrinsic functions
-// llvm.objc.retain and llvm.objc.storeStrong
+may_throw(ex);
+  } @catch (id ex_caught) {
+do_something();
+may_throw(ex_caught);
   }
 }
 
-// CHECK-LABEL: define{{.*}} void {{.*}}test_catch_with_objc_intrinsic
-//...
-// CHECK:   catch.dispatch:
-// CHECK-NEXT:[[CATCHSWITCH:%[0-9]+]] = catchswitch within none
-//...
-// CHECK:   catch:
-// CHECK-NEXT:[[CATCHPAD:%[0-9]+]] = catchpad within [[CATCHSWITCH]]
-// CHECK: {{%[0-9]+}} = call {{.*}} @llvm.objc.retain{{.*}} [ "funclet"(token [[CATCHPAD]]) ]
-// CHECK: call {{.*}} @llvm.objc.storeStrong{{.*}} [ "funclet"(token [[CATCHPAD]]) ]
+// CHECK-LABEL:   try_catch_with_objc_intrinsic
+//
+// CHECK: catch.dispatch:
+// CHECK-NEXT:  [[CATCHSWITCH:%[0-9]+]] = catchswitch within none [label %catch] unwind label %[[CLEANUP1:.*]]
+//
+// All calls within a catchpad must have funclet tokens that refer to it:
+// CHECK: catch:
+// CHECK-NEXT:  [[CATCHPAD:%[0-9]+]] = catchpad within [[CATCHSWITCH]]
+// CHECK:   call
+// CHECK: @llvm.objc.retain
+// CHECK: [ "funclet"(token [[CATCHPAD]]) ]
+// CHECK:   invoke
+// CHECK: do_something
+// CHECK: [ "funclet"(token [[CATCHPAD]]) ]
+// CHECK: unwind label %[[CLEANUP2:.*]]
+// CHECK:   invoke
+// CHECK: may_throw
+// CHECK: [ "funclet"(token [[CATCHPAD]]) ]
+// CHECK: unwind label %[[CLEANUP2]]
+// CHECK:   call
+// CHECK: @llvm.objc.storeStrong
+// CHECK: [ "funclet"(token [[CATCHPAD]]) ]
+// CHECK:   catchret from [[CATCHPAD]] to label %catchret.dest
+//
+// This block exists and it's empty:
+// CHECK: catchret.dest:
+// CHECK-NEXT:  br label %eh.cont
+//
+// CHECK: [[CLEANUP2]]:
+// CHECK-NEXT:  [[CLEANUPPAD2:%[0-9]+]] = cleanuppad within [[CATCHPAD]]
+// CHECK:   call
+// CHECK: @llvm.objc.storeStrong
+// CHECK: [ "funclet"(token [[CLEANUPPAD2]]) ]
+// CHECK:   cleanupret from [[CLEANUPPAD2]]
+// CHECK: unwind label %[[CLEANUP1]]
+//
+// CHECK: [[CLEANUP1]]:
+// CHECK-NEXT:  [[CLEANUPPAD1:%[0-9]+]] = cleanuppad within none
+// CHECK:   call
+// CHECK: @llvm.objc.storeStrong
+// CHECK: [ "funclet"(token [[CLEANUPPAD1]]) ]
+// CHECK:   cleanupret from [[CLEANUPPAD1]] unwind to caller
Index: clang/lib/CodeGen/CGObjCRuntime.cpp
===
--- clang/lib/CodeGen/CGObjCRuntime.cpp
+++ clang/lib/CodeGen/CGObjCRuntime.cpp
@@ -22,6 +22,7 @@
 #include "clang/AST/StmtObjC.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
 #include "clang/CodeGen/CodeGenABITypes.h"
+#include "llvm/IR/Instruction.h"
 #include "llvm/Support/SaveAndRestore.h"
 
 using namespace clang;
@@ -227,13 +228,18 @@
 CatchHandler  = Handlers[I];
 
 CGF.EmitBlock(Handler.Block);
-llvm::CatchPadInst *CPI = nullptr;
-SaveAndRestore RestoreCurrentFuncletPad(CGF.CurrentFuncletPad);
-if (useFunclets)
-  if ((CPI = dyn_cast_or_null(Handler.Block->getFirstNonPHI( {

[PATCH] D138122: Lift EHPersonalities from Analysis to IR (NFC)

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz created this revision.
sgraenitz added reviewers: rnk, efriedma, dblaikie, compnerd.
Herald added subscribers: Enna1, wenlei, pengfei, hiraditya.
Herald added a project: All.
sgraenitz requested review of this revision.
Herald added subscribers: cfe-commits, aheejin.
Herald added projects: clang, LLVM.

Computing EH-related information was only relevant for analysis passes so far. 
Lifting it to IR will allow the IR Verifier to calculate EH funclet coloring 
and validate funclet operand bundles in a follow-up step.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D138122

Files:
  clang/docs/tools/clang-formatted-files.txt
  llvm/include/llvm/Analysis/EHPersonalities.h
  llvm/include/llvm/Analysis/MustExecute.h
  llvm/include/llvm/CodeGen/MachineFunction.h
  llvm/include/llvm/IR/EHPersonalities.h
  llvm/lib/Analysis/CMakeLists.txt
  llvm/lib/Analysis/EHPersonalities.cpp
  llvm/lib/Analysis/ValueTracking.cpp
  llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
  llvm/lib/CodeGen/DwarfEHPrepare.cpp
  llvm/lib/CodeGen/MachineFunction.cpp
  llvm/lib/CodeGen/MachineVerifier.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
  llvm/lib/CodeGen/StackProtector.cpp
  llvm/lib/CodeGen/WinEHPrepare.cpp
  llvm/lib/IR/CMakeLists.txt
  llvm/lib/IR/EHPersonalities.cpp
  llvm/lib/Target/M68k/M68kCollapseMOVEMPass.cpp
  llvm/lib/Target/M68k/M68kExpandPseudo.cpp
  llvm/lib/Target/X86/X86ExpandPseudo.cpp
  llvm/lib/Target/X86/X86FrameLowering.cpp
  llvm/lib/Target/X86/X86ISelLowering.cpp
  llvm/lib/Target/X86/X86WinEHState.cpp
  llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
  llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
  llvm/lib/Transforms/ObjCARC/ObjCARC.h
  llvm/lib/Transforms/ObjCARC/ObjCARCContract.cpp
  llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
  llvm/lib/Transforms/Utils/EscapeEnumerator.cpp
  llvm/lib/Transforms/Utils/InlineFunction.cpp
  llvm/lib/Transforms/Utils/Local.cpp
  llvm/utils/gn/secondary/llvm/lib/Analysis/BUILD.gn
  llvm/utils/gn/secondary/llvm/lib/IR/BUILD.gn

Index: llvm/utils/gn/secondary/llvm/lib/IR/BUILD.gn
===
--- llvm/utils/gn/secondary/llvm/lib/IR/BUILD.gn
+++ llvm/utils/gn/secondary/llvm/lib/IR/BUILD.gn
@@ -33,6 +33,7 @@
 "DiagnosticInfo.cpp",
 "DiagnosticPrinter.cpp",
 "Dominators.cpp",
+"EHPersonalities.cpp",
 "FPEnv.cpp",
 "Function.cpp",
 "GCStrategy.cpp",
Index: llvm/utils/gn/secondary/llvm/lib/Analysis/BUILD.gn
===
--- llvm/utils/gn/secondary/llvm/lib/Analysis/BUILD.gn
+++ llvm/utils/gn/secondary/llvm/lib/Analysis/BUILD.gn
@@ -50,7 +50,6 @@
 "DomPrinter.cpp",
 "DomTreeUpdater.cpp",
 "DominanceFrontier.cpp",
-"EHPersonalities.cpp",
 "FunctionPropertiesAnalysis.cpp",
 "GlobalsModRef.cpp",
 "GuardUtils.cpp",
Index: llvm/lib/Transforms/Utils/Local.cpp
===
--- llvm/lib/Transforms/Utils/Local.cpp
+++ llvm/lib/Transforms/Utils/Local.cpp
@@ -27,7 +27,7 @@
 #include "llvm/Analysis/AssumeBundleQueries.h"
 #include "llvm/Analysis/ConstantFolding.h"
 #include "llvm/Analysis/DomTreeUpdater.h"
-#include "llvm/Analysis/EHPersonalities.h"
+#include "llvm/IR/EHPersonalities.h"
 #include "llvm/Analysis/InstructionSimplify.h"
 #include "llvm/Analysis/MemoryBuiltins.h"
 #include "llvm/Analysis/MemorySSAUpdater.h"
Index: llvm/lib/Transforms/Utils/InlineFunction.cpp
===
--- llvm/lib/Transforms/Utils/InlineFunction.cpp
+++ llvm/lib/Transforms/Utils/InlineFunction.cpp
@@ -25,7 +25,7 @@
 #include "llvm/Analysis/BlockFrequencyInfo.h"
 #include "llvm/Analysis/CallGraph.h"
 #include "llvm/Analysis/CaptureTracking.h"
-#include "llvm/Analysis/EHPersonalities.h"
+#include "llvm/IR/EHPersonalities.h"
 #include "llvm/Analysis/InstructionSimplify.h"
 #include "llvm/Analysis/MemoryProfileInfo.h"
 #include "llvm/Analysis/ObjCARCAnalysisUtils.h"
Index: llvm/lib/Transforms/Utils/EscapeEnumerator.cpp
===
--- llvm/lib/Transforms/Utils/EscapeEnumerator.cpp
+++ llvm/lib/Transforms/Utils/EscapeEnumerator.cpp
@@ -13,7 +13,7 @@
 
 #include "llvm/Transforms/Utils/EscapeEnumerator.h"
 #include "llvm/ADT/Triple.h"
-#include "llvm/Analysis/EHPersonalities.h"
+#include "llvm/IR/EHPersonalities.h"
 #include "llvm/IR/Module.h"
 #include "llvm/Transforms/Utils/Local.h"
 
Index: llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
===
--- llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
+++ llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
@@ -37,7 +37,7 @@
 #include "llvm/ADT/SmallVector.h"
 #include 

[PATCH] D137944: [ObjC][ARC] Teach the OptimizeSequences step of ObjCARCOpts about WinEH funclet tokens

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

There doesn't seem to be a 1-to-1 relationship between deleted and inserted 
retain/release calls and thus we don't attempt to preserve bundle operands in 
general.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137944

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

[PATCH] D137944: [ObjC][ARC] Teach the OptimizeSequences step of ObjCARCOpts about WinEH funclet tokens

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

There was a similar effort for CleanupPads in the past: 
https://reviews.llvm.org/rG8b342680bf62722e5099074e8bd23491c71d92b3


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137944

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

[PATCH] D137944: [ObjC][ARC] Teach the OptimizeSequences step of ObjCARCOpts about WinEH funclet tokens

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz created this revision.
sgraenitz added reviewers: gottesmm, rjmccall, rnk.
Herald added a subscriber: hiraditya.
Herald added a project: All.
sgraenitz requested review of this revision.
Herald added projects: clang, LLVM.
Herald added a subscriber: cfe-commits.

When optimizing retain-release-sequences we insert (and delete) ObjC runtime 
calls. These calls need a funclet operand bundle that refers to the enclosing 
funclet pad whenever they are inserted in a WinEH funclet. WinEH funclets can 
contain multiple basic blocks. In order to find the enclosing funclet pad, we 
have to calculate the funclet coloring first.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D137944

Files:
  clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
  llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
  llvm/test/Transforms/ObjCARC/funclet-catchpad.ll

Index: llvm/test/Transforms/ObjCARC/funclet-catchpad.ll
===
--- /dev/null
+++ llvm/test/Transforms/ObjCARC/funclet-catchpad.ll
@@ -0,0 +1,40 @@
+; RUN: opt -mtriple=x86_64-windows-msvc -passes=objc-arc -S < %s | FileCheck %s
+
+; Check that funclet tokens are preserved
+;
+; CHECK-LABEL:  catch:
+; CHECK:  %1 = catchpad within %0
+; CHECK:  %2 = tail call ptr @llvm.objc.retain(ptr %exn) #0 [ "funclet"(token %1) ]
+; CHECK:  call void @llvm.objc.release(ptr %exn) #0 [ "funclet"(token %1) ]
+; CHECK:  catchret from %1 to label %eh.cont
+
+define void @try_catch_with_objc_intrinsic() personality ptr @__CxxFrameHandler3 {
+entry:
+  %exn.slot = alloca ptr, align 8
+  invoke void @may_throw(ptr null) to label %eh.cont unwind label %catch.dispatch
+
+catch.dispatch:   ; preds = %entry
+  %0 = catchswitch within none [label %catch] unwind to caller
+
+eh.cont:  ; preds = %catch, %entry
+  ret void
+
+catch:; preds = %catch.dispatch
+  %1 = catchpad within %0 [ptr null, i32 0, ptr %exn.slot]
+  br label %if.then
+
+if.then:  ; preds = %catch
+  %exn = load ptr, ptr null, align 8
+  %2 = call ptr @llvm.objc.retain(ptr %exn) [ "funclet"(token %1) ]
+  call void @may_throw(ptr %exn)
+  call void @llvm.objc.release(ptr %exn) [ "funclet"(token %1) ]
+  catchret from %1 to label %eh.cont
+}
+
+declare void @may_throw(ptr)
+declare i32 @__CxxFrameHandler3(...)
+
+declare ptr @llvm.objc.retain(ptr) #0
+declare void @llvm.objc.release(ptr) #0
+
+attributes #0 = { nounwind }
Index: llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
===
--- llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
+++ llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
@@ -546,7 +546,9 @@
   void MoveCalls(Value *Arg, RRInfo , RRInfo ,
  BlotMapVector ,
  DenseMap ,
- SmallVectorImpl , Module *M);
+ SmallVectorImpl ,
+ const DenseMap ,
+ Module *M);
 
   bool PairUpRetainsAndReleases(DenseMap ,
 BlotMapVector ,
@@ -559,7 +561,7 @@
 
   bool PerformCodePlacement(DenseMap ,
 BlotMapVector ,
-DenseMap , Module *M);
+DenseMap , Function );
 
   void OptimizeWeakCalls(Function );
 
@@ -756,6 +758,18 @@
 
   return CallInst::Create(, OpBundles);
 }
+
+void addOpBundleForFunclet(BasicBlock *BB,
+   const DenseMap ,
+   SmallVectorImpl ) {
+  if (!BlockColors.empty()) {
+const ColorVector  = BlockColors.find(BB)->second;
+assert(CV.size() == 1 && "non-unique color for block!");
+BasicBlock *EHPadBB = CV.front();
+if (auto *EHPad = dyn_cast(EHPadBB->getFirstNonPHI()))
+  OpBundles.emplace_back("funclet", EHPad);
+  }
+}
 }
 
 /// Visit each call, one at a time, and make simplifications without doing any
@@ -1757,12 +1771,12 @@
 }
 
 /// Move the calls in RetainsToMove and ReleasesToMove.
-void ObjCARCOpt::MoveCalls(Value *Arg, RRInfo ,
-   RRInfo ,
-   BlotMapVector ,
-   DenseMap ,
-   SmallVectorImpl ,
-   Module *M) {
+void ObjCARCOpt::MoveCalls(
+Value *Arg, RRInfo , RRInfo ,
+BlotMapVector ,
+DenseMap ,
+SmallVectorImpl ,
+const DenseMap , Module *M) {
   Type *ArgTy = Arg->getType();
   Type *ParamTy = PointerType::getUnqual(Type::getInt8Ty(ArgTy->getContext()));
 
@@ -1773,7 +1787,9 @@
 Value *MyArg = ArgTy == ParamTy ? Arg :
new BitCastInst(Arg, ParamTy, "", InsertPt);
 Function *Decl = EP.get(ARCRuntimeEntryPointKind::Retain);
-CallInst *Call = CallInst::Create(Decl, MyArg, "", InsertPt);
+SmallVector BundleList;
+addOpBundleForFunclet(InsertPt->getParent(), 

[PATCH] D137942: [CGObjC] Add run line for release mode in test arc-exceptions-seh.mm (NFC)

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz created this revision.
Herald added a project: All.
sgraenitz requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

In release mode the updated `arc-exceptions-seh.mm` test fails and needs 
`-enable-objc-arc-opts=false` to skip ObjC ARC optimizations.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D137942

Files:
  clang/test/CodeGenObjCXX/arc-exceptions-seh.mm


Index: clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
===
--- clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
+++ clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
@@ -1,11 +1,12 @@
-// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -emit-llvm -fobjc-arc 
-fexceptions -fobjc-exceptions -fobjc-runtime=gnustep-2.0 -o - %s | FileCheck %s
+// RUN: %clang_cc1 -O0 -triple x86_64-pc-windows-msvc -emit-llvm -fobjc-arc 
-fexceptions -fobjc-exceptions -fobjc-runtime=gnustep-2.0 -o - %s | FileCheck 
%s --check-prefixes=CHECK,CHECK-O0
+// RUN: %clang_cc1 -O2 -triple x86_64-pc-windows-msvc -emit-llvm -fobjc-arc 
-fexceptions -fobjc-exceptions -fobjc-runtime=gnustep-2.0 -mllvm 
-enable-objc-arc-opts=false -o - %s | FileCheck %s 
--check-prefixes=CHECK,CHECK-O2
 
 // WinEH requires funclet tokens on nounwind intrinsics if they can lower to
 // regular function calls in the course of IR transformations.
 //
 // This is the case for ObjC ARC runtime intrinsics. Test that clang emits the
-// funclet tokens for llvm.objc.retain and llvm.objc.storeStrong and that they
-// refer to their catchpad's SSA value.
+// funclet tokens for llvm.objc.* calls inside catchpads and that they refer to
+// their catchpad's SSA value.
 
 void do_something();
 void may_throw(id);
@@ -39,10 +40,12 @@
 // CHECK:   may_throw
 // CHECK:   [ "funclet"(token [[CATCHPAD]]) ]
 // CHECK:   call
-// CHECK:   @llvm.objc.storeStrong
+// CHECK-O0:@llvm.objc.storeStrong
+// CHECK-O2:@llvm.objc.release
 // CHECK:   [ "funclet"(token [[CATCHPAD]]) ]
-// CHECK:   catchret from [[CATCHPAD]] to label %catchret.dest
+// CHECK-O0:catchret from [[CATCHPAD]] to label %catchret.dest
+// CHECK-O2:catchret from [[CATCHPAD]] to label %eh.cont
 //
-// This block exists and it's empty:
-// CHECK: catchret.dest:
-// CHECK-NEXT:  br label %eh.cont
+// In debug mode, this block exists and it's empty:
+// CHECK-O0:  catchret.dest:
+// CHECK-O0-NEXT:   br label %eh.cont


Index: clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
===
--- clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
+++ clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
@@ -1,11 +1,12 @@
-// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -emit-llvm -fobjc-arc -fexceptions -fobjc-exceptions -fobjc-runtime=gnustep-2.0 -o - %s | FileCheck %s
+// RUN: %clang_cc1 -O0 -triple x86_64-pc-windows-msvc -emit-llvm -fobjc-arc -fexceptions -fobjc-exceptions -fobjc-runtime=gnustep-2.0 -o - %s | FileCheck %s --check-prefixes=CHECK,CHECK-O0
+// RUN: %clang_cc1 -O2 -triple x86_64-pc-windows-msvc -emit-llvm -fobjc-arc -fexceptions -fobjc-exceptions -fobjc-runtime=gnustep-2.0 -mllvm -enable-objc-arc-opts=false -o - %s | FileCheck %s --check-prefixes=CHECK,CHECK-O2
 
 // WinEH requires funclet tokens on nounwind intrinsics if they can lower to
 // regular function calls in the course of IR transformations.
 //
 // This is the case for ObjC ARC runtime intrinsics. Test that clang emits the
-// funclet tokens for llvm.objc.retain and llvm.objc.storeStrong and that they
-// refer to their catchpad's SSA value.
+// funclet tokens for llvm.objc.* calls inside catchpads and that they refer to
+// their catchpad's SSA value.
 
 void do_something();
 void may_throw(id);
@@ -39,10 +40,12 @@
 // CHECK:   may_throw
 // CHECK:   [ "funclet"(token [[CATCHPAD]]) ]
 // CHECK:   call
-// CHECK:   @llvm.objc.storeStrong
+// CHECK-O0:@llvm.objc.storeStrong
+// CHECK-O2:@llvm.objc.release
 // CHECK:   [ "funclet"(token [[CATCHPAD]]) ]
-// CHECK:   catchret from [[CATCHPAD]] to label %catchret.dest
+// CHECK-O0:catchret from [[CATCHPAD]] to label %catchret.dest
+// CHECK-O2:catchret from [[CATCHPAD]] to label %eh.cont
 //
-// This block exists and it's empty:
-// CHECK: catchret.dest:
-// CHECK-NEXT:  br label %eh.cont
+// In debug mode, this block exists and it's empty:
+// CHECK-O0:  catchret.dest:
+// CHECK-O0-NEXT:   br label %eh.cont
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137939: [CGObjC] Open cleanup scope before SaveAndRestore CurrentFuncletPad and push CatchRetScope early

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

The approach follows the C++ try/catch implementation in 
clang/lib/CodeGen/CGException.cpp 
.
 I think this is the correct solution for what I attempted to fix with D134866 
 in the backend before.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137939

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

[PATCH] D137939: [CGObjC] Open cleanup scope before SaveAndRestore CurrentFuncletPad and push CatchRetScope early

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz created this revision.
sgraenitz added reviewers: theraven, rnk.
Herald added a project: All.
sgraenitz requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Pushing the `CatchRetScope` early causes cleanups for catch parameters to be 
emitted in the basic block of the catch handler instead of the `catchret.dest` 
block. This is important because the latter is not part of the catchpad and 
this caused code truncations due to ARC PreISel intrinsics in WinEHPrepare.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D137939

Files:
  clang/lib/CodeGen/CGObjCRuntime.cpp
  clang/test/CodeGenObjCXX/arc-exceptions-seh.mm

Index: clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
===
--- clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
+++ clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
@@ -7,23 +7,42 @@
 // funclet tokens for llvm.objc.retain and llvm.objc.storeStrong and that they
 // refer to their catchpad's SSA value.
 
-@class Ety;
-void opaque(void);
-void test_catch_with_objc_intrinsic(void) {
+void do_something();
+void may_throw(id);
+
+void try_catch_with_objc_intrinsic() {
+  id ex;
   @try {
-opaque();
-  } @catch (Ety *ex) {
-// Destroy ex when leaving catchpad. This emits calls to intrinsic functions
-// llvm.objc.retain and llvm.objc.storeStrong
+may_throw(ex);
+  } @catch (id ex_caught) {
+do_something();
+may_throw(ex_caught);
   }
 }
 
-// CHECK-LABEL: define{{.*}} void {{.*}}test_catch_with_objc_intrinsic
-//...
-// CHECK:   catch.dispatch:
-// CHECK-NEXT:[[CATCHSWITCH:%[0-9]+]] = catchswitch within none
-//...
-// CHECK:   catch:
-// CHECK-NEXT:[[CATCHPAD:%[0-9]+]] = catchpad within [[CATCHSWITCH]]
-// CHECK: {{%[0-9]+}} = call {{.*}} @llvm.objc.retain{{.*}} [ "funclet"(token [[CATCHPAD]]) ]
-// CHECK: call {{.*}} @llvm.objc.storeStrong{{.*}} [ "funclet"(token [[CATCHPAD]]) ]
+// CHECK-LABEL:   try_catch_with_objc_intrinsic
+//
+// CHECK: catch.dispatch:
+// CHECK-NEXT:  [[CATCHSWITCH:%[0-9]+]] = catchswitch within none [label %catch]
+//
+// All calls within a catchpad must have funclet tokens that refer to the
+// enclosing catchpad:
+// CHECK: catch:
+// CHECK-NEXT:  [[CATCHPAD:%[0-9]+]] = catchpad within [[CATCHSWITCH]]
+// CHECK:   call
+// CHECK:   @llvm.objc.retain
+// CHECK:   [ "funclet"(token [[CATCHPAD]]) ]
+// CHECK:   call
+// CHECK:   do_something
+// CHECK:   [ "funclet"(token [[CATCHPAD]]) ]
+// CHECK:   call
+// CHECK:   may_throw
+// CHECK:   [ "funclet"(token [[CATCHPAD]]) ]
+// CHECK:   call
+// CHECK:   @llvm.objc.storeStrong
+// CHECK:   [ "funclet"(token [[CATCHPAD]]) ]
+// CHECK:   catchret from [[CATCHPAD]] to label %catchret.dest
+//
+// This block exists and it's empty:
+// CHECK: catchret.dest:
+// CHECK-NEXT:  br label %eh.cont
Index: clang/lib/CodeGen/CGObjCRuntime.cpp
===
--- clang/lib/CodeGen/CGObjCRuntime.cpp
+++ clang/lib/CodeGen/CGObjCRuntime.cpp
@@ -22,6 +22,7 @@
 #include "clang/AST/StmtObjC.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
 #include "clang/CodeGen/CodeGenABITypes.h"
+#include "llvm/IR/Instruction.h"
 #include "llvm/Support/SaveAndRestore.h"
 
 using namespace clang;
@@ -227,13 +228,18 @@
 CatchHandler  = Handlers[I];
 
 CGF.EmitBlock(Handler.Block);
-llvm::CatchPadInst *CPI = nullptr;
-SaveAndRestore RestoreCurrentFuncletPad(CGF.CurrentFuncletPad);
-if (useFunclets)
-  if ((CPI = dyn_cast_or_null(Handler.Block->getFirstNonPHI( {
+
+CodeGenFunction::LexicalScope Cleanups(CGF, Handler.Body->getSourceRange());
+SaveAndRestore RevertAfterScope(CGF.CurrentFuncletPad);
+if (useFunclets) {
+  llvm::Instruction *CPICandidate = Handler.Block->getFirstNonPHI();
+  if (auto *CPI = dyn_cast_or_null(CPICandidate)) {
 CGF.CurrentFuncletPad = CPI;
 CPI->setOperand(2, CGF.getExceptionSlot().getPointer());
+CGF.EHStack.pushCleanup(NormalCleanup, CPI);
   }
+}
+
 llvm::Value *RawExn = CGF.getExceptionFromSlot();
 
 // Enter the catch.
@@ -241,8 +247,6 @@
 if (beginCatchFn)
   Exn = CGF.EmitNounwindRuntimeCall(beginCatchFn, RawExn, "exn.adjusted");
 
-CodeGenFunction::LexicalScope cleanups(CGF, Handler.Body->getSourceRange());
-
 if (endCatchFn) {
   // Add a cleanup to leave the catch.
   bool EndCatchMightThrow = (Handler.Variable == nullptr);
@@ -260,15 +264,13 @@
   CGF.EmitAutoVarDecl(*CatchParam);
   EmitInitOfCatchParam(CGF, CastExn, CatchParam);
 }
-if (CPI)
-CGF.EHStack.pushCleanup(NormalCleanup, CPI);
 
 CGF.ObjCEHValueStack.push_back(Exn);
 CGF.EmitStmt(Handler.Body);
 

[PATCH] D134441: [ObjC][ARC] Fix target register for call expanded from CALL_RVMARKER on Windows

[Stefan Gränitz via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGed8409dfa0a9: [ObjC][ARC] Fix target register for call 
expanded from CALL_RVMARKER on Windows (authored by sgraenitz).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134441

Files:
  llvm/lib/Target/X86/X86ExpandPseudo.cpp
  llvm/test/CodeGen/X86/call-rv-marker.ll


Index: llvm/test/CodeGen/X86/call-rv-marker.ll
===
--- llvm/test/CodeGen/X86/call-rv-marker.ll
+++ llvm/test/CodeGen/X86/call-rv-marker.ll
@@ -1,4 +1,5 @@
 ; RUN: llc -mtriple=x86_64-apple-macosx -verify-machineinstrs -o - %s | 
FileCheck --check-prefix=CHECK %s
+; RUN: llc -mtriple=x86_64-windows-msvc -verify-machineinstrs -o - %s | 
FileCheck --check-prefix=WINABI %s
 
 ; TODO: support marker generation with GlobalISel
 target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
@@ -33,6 +34,12 @@
 ; CHECK-NEXT:popq%rcx
 ; CHECK-NEXT:retq
 ;
+; WINABI-LABEL: rv_marker_1_retain:
+; WINABI:callq   foo1
+; WINABI-NEXT:   movq%rax, %rcx
+; WINABI-NEXT:   callq   objc_retainAutoreleasedReturnValue
+; WINABI-NEXT:   nop
+;
 entry:
   %call = call ptr @foo1() [ "clang.arc.attachedcall"(ptr 
@objc_retainAutoreleasedReturnValue) ]
   ret ptr %call
Index: llvm/lib/Target/X86/X86ExpandPseudo.cpp
===
--- llvm/lib/Target/X86/X86ExpandPseudo.cpp
+++ llvm/lib/Target/X86/X86ExpandPseudo.cpp
@@ -224,9 +224,11 @@
   // Emit marker "movq %rax, %rdi".  %rdi is not callee-saved, so it cannot be
   // live across the earlier call. The call to the ObjC runtime function 
returns
   // the first argument, so the value of %rax is unchanged after the ObjC
-  // runtime call.
+  // runtime call. On Windows targets, the runtime call follows the regular
+  // x64 calling convention and expects the first argument in %rcx.
+  auto TargetReg = STI->getTargetTriple().isOSWindows() ? X86::RCX : X86::RDI;
   auto *Marker = BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(X86::MOV64rr))
- .addReg(X86::RDI, RegState::Define)
+ .addReg(TargetReg, RegState::Define)
  .addReg(X86::RAX)
  .getInstr();
   if (MI.shouldUpdateCallSiteInfo())


Index: llvm/test/CodeGen/X86/call-rv-marker.ll
===
--- llvm/test/CodeGen/X86/call-rv-marker.ll
+++ llvm/test/CodeGen/X86/call-rv-marker.ll
@@ -1,4 +1,5 @@
 ; RUN: llc -mtriple=x86_64-apple-macosx -verify-machineinstrs -o - %s | FileCheck --check-prefix=CHECK %s
+; RUN: llc -mtriple=x86_64-windows-msvc -verify-machineinstrs -o - %s | FileCheck --check-prefix=WINABI %s
 
 ; TODO: support marker generation with GlobalISel
 target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
@@ -33,6 +34,12 @@
 ; CHECK-NEXT:popq%rcx
 ; CHECK-NEXT:retq
 ;
+; WINABI-LABEL: rv_marker_1_retain:
+; WINABI:callq   foo1
+; WINABI-NEXT:   movq%rax, %rcx
+; WINABI-NEXT:   callq   objc_retainAutoreleasedReturnValue
+; WINABI-NEXT:   nop
+;
 entry:
   %call = call ptr @foo1() [ "clang.arc.attachedcall"(ptr @objc_retainAutoreleasedReturnValue) ]
   ret ptr %call
Index: llvm/lib/Target/X86/X86ExpandPseudo.cpp
===
--- llvm/lib/Target/X86/X86ExpandPseudo.cpp
+++ llvm/lib/Target/X86/X86ExpandPseudo.cpp
@@ -224,9 +224,11 @@
   // Emit marker "movq %rax, %rdi".  %rdi is not callee-saved, so it cannot be
   // live across the earlier call. The call to the ObjC runtime function returns
   // the first argument, so the value of %rax is unchanged after the ObjC
-  // runtime call.
+  // runtime call. On Windows targets, the runtime call follows the regular
+  // x64 calling convention and expects the first argument in %rcx.
+  auto TargetReg = STI->getTargetTriple().isOSWindows() ? X86::RCX : X86::RDI;
   auto *Marker = BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(X86::MOV64rr))
- .addReg(X86::RDI, RegState::Define)
+ .addReg(TargetReg, RegState::Define)
  .addReg(X86::RAX)
  .getInstr();
   if (MI.shouldUpdateCallSiteInfo())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D134441: [ObjC][ARC] Fix target register for call expanded from CALL_RVMARKER on Windows

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

In D134441#3814794 , @fhahn wrote:

> LGTM, thanks! You might want to extend the windows check lines to the other 
> tests as well.

Thanks. Yes thought about that, but not all of them match the typical IR output 
on Windows (gxx_personality, cfi, etc.). The test case that I extended is 
exactly the scenario we were running into and for the moment it seems good 
enough to make sure we don't regress again. I will keep the review open for a 
day or two before landing this -- in case there is more feedback.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134441

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

[PATCH] D134441: [ObjC][ARC] Don't use operand bundle "clang.arc.attachedcall" in codegen for Windows

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz updated this revision to Diff 462871.
sgraenitz added a comment.
Herald added a subscriber: hiraditya.
Herald added a project: LLVM.

Follow Reid's advice from 
https://github.com/llvm/llvm-project/issues/56952#issuecomment-122565 
and fix the instruction sequence expanded from CALL_RVMARKER on Windows (use 
RCX as target 
register and not RDI). This way we stay as close as possible to Apple's ABI and 
GNUstep has the 
option to implement detection of optimized callers for autorelease 
optimizations in the future.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134441

Files:
  llvm/lib/Target/X86/X86ExpandPseudo.cpp
  llvm/test/CodeGen/X86/call-rv-marker.ll


Index: llvm/test/CodeGen/X86/call-rv-marker.ll
===
--- llvm/test/CodeGen/X86/call-rv-marker.ll
+++ llvm/test/CodeGen/X86/call-rv-marker.ll
@@ -1,4 +1,5 @@
 ; RUN: llc -mtriple=x86_64-apple-macosx -verify-machineinstrs -o - %s | 
FileCheck --check-prefix=CHECK %s
+; RUN: llc -mtriple=x86_64-windows-msvc -verify-machineinstrs -o - %s | 
FileCheck --check-prefix=WINABI %s
 
 ; TODO: support marker generation with GlobalISel
 target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
@@ -33,6 +34,12 @@
 ; CHECK-NEXT:popq%rcx
 ; CHECK-NEXT:retq
 ;
+; WINABI-LABEL: rv_marker_1_retain:
+; WINABI:callq   foo1
+; WINABI-NEXT:   movq%rax, %rcx
+; WINABI-NEXT:   callq   objc_retainAutoreleasedReturnValue
+; WINABI-NEXT:   nop
+;
 entry:
   %call = call ptr @foo1() [ "clang.arc.attachedcall"(ptr 
@objc_retainAutoreleasedReturnValue) ]
   ret ptr %call
Index: llvm/lib/Target/X86/X86ExpandPseudo.cpp
===
--- llvm/lib/Target/X86/X86ExpandPseudo.cpp
+++ llvm/lib/Target/X86/X86ExpandPseudo.cpp
@@ -224,9 +224,11 @@
   // Emit marker "movq %rax, %rdi".  %rdi is not callee-saved, so it cannot be
   // live across the earlier call. The call to the ObjC runtime function 
returns
   // the first argument, so the value of %rax is unchanged after the ObjC
-  // runtime call.
+  // runtime call. On Windows targets, the runtime call follows the regular
+  // x64 calling convention and expects the first argument in %rcx.
+  auto TargetReg = STI->getTargetTriple().isOSWindows() ? X86::RCX : X86::RDI;
   auto *Marker = BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(X86::MOV64rr))
- .addReg(X86::RDI, RegState::Define)
+ .addReg(TargetReg, RegState::Define)
  .addReg(X86::RAX)
  .getInstr();
   if (MI.shouldUpdateCallSiteInfo())


Index: llvm/test/CodeGen/X86/call-rv-marker.ll
===
--- llvm/test/CodeGen/X86/call-rv-marker.ll
+++ llvm/test/CodeGen/X86/call-rv-marker.ll
@@ -1,4 +1,5 @@
 ; RUN: llc -mtriple=x86_64-apple-macosx -verify-machineinstrs -o - %s | FileCheck --check-prefix=CHECK %s
+; RUN: llc -mtriple=x86_64-windows-msvc -verify-machineinstrs -o - %s | FileCheck --check-prefix=WINABI %s
 
 ; TODO: support marker generation with GlobalISel
 target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
@@ -33,6 +34,12 @@
 ; CHECK-NEXT:popq%rcx
 ; CHECK-NEXT:retq
 ;
+; WINABI-LABEL: rv_marker_1_retain:
+; WINABI:callq   foo1
+; WINABI-NEXT:   movq%rax, %rcx
+; WINABI-NEXT:   callq   objc_retainAutoreleasedReturnValue
+; WINABI-NEXT:   nop
+;
 entry:
   %call = call ptr @foo1() [ "clang.arc.attachedcall"(ptr @objc_retainAutoreleasedReturnValue) ]
   ret ptr %call
Index: llvm/lib/Target/X86/X86ExpandPseudo.cpp
===
--- llvm/lib/Target/X86/X86ExpandPseudo.cpp
+++ llvm/lib/Target/X86/X86ExpandPseudo.cpp
@@ -224,9 +224,11 @@
   // Emit marker "movq %rax, %rdi".  %rdi is not callee-saved, so it cannot be
   // live across the earlier call. The call to the ObjC runtime function returns
   // the first argument, so the value of %rax is unchanged after the ObjC
-  // runtime call.
+  // runtime call. On Windows targets, the runtime call follows the regular
+  // x64 calling convention and expects the first argument in %rcx.
+  auto TargetReg = STI->getTargetTriple().isOSWindows() ? X86::RCX : X86::RDI;
   auto *Marker = BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(X86::MOV64rr))
- .addReg(X86::RDI, RegState::Define)
+ .addReg(TargetReg, RegState::Define)
  .addReg(X86::RAX)
  .getInstr();
   if (MI.shouldUpdateCallSiteInfo())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D134441: [ObjC][ARC] Don't use operand bundle "clang.arc.attachedcall" in codegen for Windows

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

Thanks for your feedback! For the moment, I continued the discussion in the 
GitHub ticket. My gut feeling is that I should fix the X86ExpandPseudo code to 
emit the right register-register move instead of preventing the use of the 
operand bundle here in CodeGen, because the inliner may generate it as well. I 
tested it like this and it works as expected:
https://github.com/weliveindetail/llvm-project/commit/c36eeefb3917c66ad75c7dd1dbcda29645d05aed

I'd like to wait for David's response on whether or not GNUstep is checking for 
optimized callers (or wants to do it in the future) before deciding for one way 
or the other.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134441

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

[PATCH] D134441: [ObjC][ARC] Don't use operand bundle "clang.arc.attachedcall" in codegen for Windows

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

The symptom is that Clang emits `movq %rax, %rdi` instead of `movq %rax, %rcx` 
while `objc_retainAutoreleasedReturnValue()` still expects the value in `%rcx`.
It appears related to D94597 . Is this a 
calling convention issue?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134441

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

[PATCH] D134441: [ObjC][ARC] Don't use operand bundle "clang.arc.attachedcall" in codegen for Windows

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

Bisectioning showed that the regression started with this patch: 
https://reviews.llvm.org/D111331


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134441

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

[PATCH] D134441: [ObjC][ARC] Don't use operand bundle "clang.arc.attachedcall" in codegen for Windows

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

I assume is that this can only be a temporary fix as it probably has an impact 
on performance of compiled output. I am eager to fix this properly. The FIXME 
comment says that we need specific support in the target backend. Any ideas 
what the x86_64 backend for Windows might be missing to handle the operand 
bundle correctly? Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134441

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

[PATCH] D134441: [ObjC][ARC] Don't use operand bundle "clang.arc.attachedcall" in codegen for Windows

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz created this revision.
sgraenitz added reviewers: ahatanak, rjmccall, theraven, rnk.
Herald added a subscriber: pengfei.
Herald added a project: All.
sgraenitz requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This patch is an attempt to fix regression 
https://github.com/llvm/llvm-project/issues/56952 for Clang 
CodeGen on Windows. It appears that the operand bundle `clang.arc.attachedcall` 
is not fully supported
in the x86_64 backend for Windows. This patch prevents Clang from emitting it 
in the first place.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D134441

Files:
  clang/lib/CodeGen/CGObjC.cpp


Index: clang/lib/CodeGen/CGObjC.cpp
===
--- clang/lib/CodeGen/CGObjC.cpp
+++ clang/lib/CodeGen/CGObjC.cpp
@@ -2359,7 +2359,8 @@
   // FIXME: Do this on all targets and at -O0 too. This can be enabled only if
   // the target backend knows how to handle the operand bundle.
   if (CGF.CGM.getCodeGenOpts().OptimizationLevel > 0 &&
-  (Arch == llvm::Triple::aarch64 || Arch == llvm::Triple::x86_64)) {
+  (Arch == llvm::Triple::aarch64 || Arch == llvm::Triple::x86_64) &&
+  !CGF.CGM.getTarget().getTriple().isOSWindows()) {
 llvm::Value *bundleArgs[] = {EP};
 llvm::OperandBundleDef OB("clang.arc.attachedcall", bundleArgs);
 auto *oldCall = cast(value);


Index: clang/lib/CodeGen/CGObjC.cpp
===
--- clang/lib/CodeGen/CGObjC.cpp
+++ clang/lib/CodeGen/CGObjC.cpp
@@ -2359,7 +2359,8 @@
   // FIXME: Do this on all targets and at -O0 too. This can be enabled only if
   // the target backend knows how to handle the operand bundle.
   if (CGF.CGM.getCodeGenOpts().OptimizationLevel > 0 &&
-  (Arch == llvm::Triple::aarch64 || Arch == llvm::Triple::x86_64)) {
+  (Arch == llvm::Triple::aarch64 || Arch == llvm::Triple::x86_64) &&
+  !CGF.CGM.getTarget().getTriple().isOSWindows()) {
 llvm::Value *bundleArgs[] = {EP};
 llvm::OperandBundleDef OB("clang.arc.attachedcall", bundleArgs);
 auto *oldCall = cast(value);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128190: [WinEH] Apply funclet operand bundles to nounwind intrinsics that lower to function calls

[Stefan Gränitz via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG1e308204838b: [WinEH] Apply funclet operand bundles to 
nounwind intrinsics that lower to… (authored by sgraenitz).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128190

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
  llvm/include/llvm/IR/IntrinsicInst.h
  llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
  llvm/lib/IR/IntrinsicInst.cpp
  llvm/lib/Transforms/Utils/InlineFunction.cpp
  llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll
  llvm/test/Feature/OperandBundles/inliner-funclet-wineh.ll

Index: llvm/test/Feature/OperandBundles/inliner-funclet-wineh.ll
===
--- /dev/null
+++ llvm/test/Feature/OperandBundles/inliner-funclet-wineh.ll
@@ -0,0 +1,51 @@
+; RUN: opt -S -always-inline -mtriple=x86_64-windows-msvc < %s | FileCheck %s
+
+; WinEH requires funclet tokens on nounwind intrinsics if they can lower to
+; regular function calls in the course of IR transformations.
+;
+; Test that the inliner propagates funclet tokens to such intrinsics when
+; inlining into EH funclets, i.e.: llvm.objc.storeStrong inherits the "funclet"
+; token from inlined_fn.
+
+define void @inlined_fn(ptr %ex) #1 {
+entry:
+  call void @llvm.objc.storeStrong(ptr %ex, ptr null)
+  ret void
+}
+
+define void @test_catch_with_inline() personality ptr @__CxxFrameHandler3 {
+entry:
+  %exn.slot = alloca ptr, align 8
+  %ex = alloca ptr, align 8
+  invoke void @opaque() to label %invoke.cont unwind label %catch.dispatch
+
+catch.dispatch:
+  %0 = catchswitch within none [label %catch] unwind to caller
+
+invoke.cont:
+  unreachable
+
+catch:
+  %1 = catchpad within %0 [ptr null, i32 64, ptr %exn.slot]
+  call void @inlined_fn(ptr %ex) [ "funclet"(token %1) ]
+  catchret from %1 to label %catchret.dest
+
+catchret.dest:
+  ret void
+}
+
+declare void @opaque()
+declare void @llvm.objc.storeStrong(ptr, ptr) #0
+declare i32 @__CxxFrameHandler3(...)
+
+attributes #0 = { nounwind }
+attributes #1 = { alwaysinline }
+
+; After inlining, llvm.objc.storeStrong inherited the "funclet" token:
+;
+;   CHECK-LABEL:  define void @test_catch_with_inline()
+;   ...
+;   CHECK:catch:
+;   CHECK-NEXT: %1 = catchpad within %0 [ptr null, i32 64, ptr %exn.slot]
+;   CHECK-NEXT: call void @llvm.objc.storeStrong(ptr %ex, ptr null) [ "funclet"(token %1) ]
+;   CHECK-NEXT: catchret from %1 to label %catchret.dest
Index: llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll
@@ -0,0 +1,77 @@
+; RUN: llc -mtriple=x86_64-windows-msvc < %s | FileCheck %s
+
+; WinEH requires funclet tokens on nounwind intrinsics if they can lower to
+; regular function calls in the course of IR transformations.
+;
+; Test that the code generator will emit the function call and not consider it
+; an "implausible instruciton". In the past this silently truncated code on
+; exception paths and caused crashes at runtime.
+;
+; Reduced IR generated from ObjC++ source:
+;
+; @class Ety;
+; void opaque(void);
+; void test_catch_with_objc_intrinsic(void) {
+;   @try {
+; opaque();
+;   } @catch (Ety *ex) {
+; // Destroy ex when leaving catchpad. This would emit calls to two
+; // intrinsic functions: llvm.objc.retain and llvm.objc.storeStrong
+;   }
+; }
+;
+; llvm.objc.retain and llvm.objc.storeStrong both lower into regular function
+; calls before ISel. We only need one of them to trigger funclet truncation
+; during codegen:
+
+define void @test_catch_with_objc_intrinsic() personality ptr @__CxxFrameHandler3 {
+entry:
+  %exn.slot = alloca ptr, align 8
+  %ex2 = alloca ptr, align 8
+  invoke void @opaque() to label %invoke.cont unwind label %catch.dispatch
+
+catch.dispatch:
+  %0 = catchswitch within none [label %catch] unwind to caller
+
+invoke.cont:
+  unreachable
+
+catch:
+  %1 = catchpad within %0 [ptr null, i32 64, ptr %exn.slot]
+  call void @llvm.objc.storeStrong(ptr %ex2, ptr null) [ "funclet"(token %1) ]
+  catchret from %1 to label %catchret.dest
+
+catchret.dest:
+  ret void
+}
+
+declare void @opaque()
+declare void @llvm.objc.storeStrong(ptr, ptr) #0
+declare i32 @__CxxFrameHandler3(...)
+
+attributes #0 = { nounwind }
+
+; EH catchpad with SEH prologue:
+; CHECK: # %catch
+; CHECK: pushq   %rbp
+; CHECK: .seh_pushreg %rbp
+;...
+; CHECK: .seh_endprologue
+;
+; At this point the code used to be truncated (and sometimes terminated with an
+; int3 opcode):
+; CHECK-NOT: int3
+;
+; Instead, the call to objc_storeStrong should be emitted:
+; CHECK: leaq	-24(%rbp), %rcx

[PATCH] D128190: [WinEH] Apply funclet operand bundles to nounwind intrinsics that lower to function calls

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

Pre-merge checks passed on Windows. Debian failures are unrelated. Locally on 
Ubuntu and macOS all tests pass. Getting ready to land this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128190

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

[PATCH] D128190: [WinEH] Apply funclet operand bundles to nounwind intrinsics that lower to function calls

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

Hi @theraven thanks for reviewing!




Comment at: llvm/lib/IR/IntrinsicInst.cpp:61
+  case Intrinsic::objc_sync_enter:
+  case Intrinsic::objc_sync_exit:
+return true;

theraven wrote:
> I'm curious why memcpy and friends don't need to be on this list.  Do they 
> get expanded at a different point?  Or is it that the front end inserts a 
> memcpy call and the optimiser that turns them into intrinsics and back 
> preserves operand bundles?  It would be a good idea to have a comment 
> explaining why these specific ones are on the list and not other libc (or 
> math library) functions that may appear in cleanup blocks.  From the code in 
> `InlineFunction.cpp`, it looks as if this is a problem only for intrinsics 
> that may throw, which excludes most of the C standard ones?
The name `mayLowerToFunctionCall` was proposed in a precursor to this review: 
https://reviews.llvm.org/D127962#inline-1227485. I agree that the term might 
appear overly general at first. I still followed the advice because:
(1) This controls assignment and propagation of operand bundles, which is a 
very general concept.
(2) From the perspective of IR transformations, only a few intrinsics actually 
lower to function calls (the majority remains intrinsics).
(3) The approach might be applied to other intrinsics, e.g. statepoints

However, this is not obvious and because other readers might have the same 
question, I extended my comment in the header like this:
```
Check if the intrinsic might lower into a regular function call in the course 
of IR transformations
```

Do you think that makes it more clear?

**For additional context**
Right now the list only contains ObjC ARC intrinsics. These are subject to 
`PreISelIntrinsicLowering` which means they are lowered to regular function 
calls before we run exception handling passes:
https://github.com/llvm/llvm-project/blob/c73adbad6a9964e0700865b7c786cc6885898c68/llvm/lib/CodeGen/TargetPassConfig.cpp#L1114-L1118

WinEHPrepare is an exception handling pass and it accepts call instructions in 
EH funclets only if they: 
* target nounwind intrinsics or inline assembly
* have a matching funclet bundle operand

It declares other calls as "implausible instructions" and marks them 
unreachable, which caused the truncation bug that this patch aims to fix.

All ObjC ARC intrinsics have the `nounwind` attribute. My understanding is that 
the only user-code that might run in turn of an invocation can be destructors 
-- and they don't throw per definition.

The reason libc functions like memcpy are not in this list is that they are 
lowered at a later point in time. In the context of WinEHPrepare (and opt in 
general) they are always intrinsics. 




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128190

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

[PATCH] D128190: [WinEH] Apply funclet operand bundles to nounwind intrinsics that lower to function calls

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz updated this revision to Diff 447667.
sgraenitz marked 2 inline comments as done.
sgraenitz added a comment.

Rebase and polish comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128190

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
  llvm/include/llvm/IR/IntrinsicInst.h
  llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
  llvm/lib/IR/IntrinsicInst.cpp
  llvm/lib/Transforms/Utils/InlineFunction.cpp
  llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll
  llvm/test/Feature/OperandBundles/inliner-funclet-wineh.ll

Index: llvm/test/Feature/OperandBundles/inliner-funclet-wineh.ll
===
--- /dev/null
+++ llvm/test/Feature/OperandBundles/inliner-funclet-wineh.ll
@@ -0,0 +1,51 @@
+; RUN: opt -S -always-inline -mtriple=x86_64-windows-msvc < %s | FileCheck %s
+
+; WinEH requires funclet tokens on nounwind intrinsics if they can lower to
+; regular function calls in the course of IR transformations.
+;
+; Test that the inliner propagates funclet tokens to such intrinsics when
+; inlining into EH funclets, i.e.: llvm.objc.storeStrong inherits the "funclet"
+; token from inlined_fn.
+
+define void @inlined_fn(ptr %ex) #1 {
+entry:
+  call void @llvm.objc.storeStrong(ptr %ex, ptr null)
+  ret void
+}
+
+define void @test_catch_with_inline() personality ptr @__CxxFrameHandler3 {
+entry:
+  %exn.slot = alloca ptr, align 8
+  %ex = alloca ptr, align 8
+  invoke void @opaque() to label %invoke.cont unwind label %catch.dispatch
+
+catch.dispatch:
+  %0 = catchswitch within none [label %catch] unwind to caller
+
+invoke.cont:
+  unreachable
+
+catch:
+  %1 = catchpad within %0 [ptr null, i32 64, ptr %exn.slot]
+  call void @inlined_fn(ptr %ex) [ "funclet"(token %1) ]
+  catchret from %1 to label %catchret.dest
+
+catchret.dest:
+  ret void
+}
+
+declare void @opaque()
+declare void @llvm.objc.storeStrong(ptr, ptr) #0
+declare i32 @__CxxFrameHandler3(...)
+
+attributes #0 = { nounwind }
+attributes #1 = { alwaysinline }
+
+; After inlining, llvm.objc.storeStrong inherited the "funclet" token:
+;
+;   CHECK-LABEL:  define void @test_catch_with_inline()
+;   ...
+;   CHECK:catch:
+;   CHECK-NEXT: %1 = catchpad within %0 [ptr null, i32 64, ptr %exn.slot]
+;   CHECK-NEXT: call void @llvm.objc.storeStrong(ptr %ex, ptr null) [ "funclet"(token %1) ]
+;   CHECK-NEXT: catchret from %1 to label %catchret.dest
Index: llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll
@@ -0,0 +1,77 @@
+; RUN: llc -mtriple=x86_64-windows-msvc < %s | FileCheck %s
+
+; WinEH requires funclet tokens on nounwind intrinsics if they can lower to
+; regular function calls in the course of IR transformations.
+;
+; Test that the code generator will emit the function call and not consider it
+; an "implausible instruciton". In the past this silently truncated code on
+; exception paths and caused crashes at runtime.
+;
+; Reduced IR generated from ObjC++ source:
+;
+; @class Ety;
+; void opaque(void);
+; void test_catch_with_objc_intrinsic(void) {
+;   @try {
+; opaque();
+;   } @catch (Ety *ex) {
+; // Destroy ex when leaving catchpad. This would emit calls to two
+; // intrinsic functions: llvm.objc.retain and llvm.objc.storeStrong
+;   }
+; }
+;
+; llvm.objc.retain and llvm.objc.storeStrong both lower into regular function
+; calls before ISel. We only need one of them to trigger funclet truncation
+; during codegen:
+
+define void @test_catch_with_objc_intrinsic() personality ptr @__CxxFrameHandler3 {
+entry:
+  %exn.slot = alloca ptr, align 8
+  %ex2 = alloca ptr, align 8
+  invoke void @opaque() to label %invoke.cont unwind label %catch.dispatch
+
+catch.dispatch:
+  %0 = catchswitch within none [label %catch] unwind to caller
+
+invoke.cont:
+  unreachable
+
+catch:
+  %1 = catchpad within %0 [ptr null, i32 64, ptr %exn.slot]
+  call void @llvm.objc.storeStrong(ptr %ex2, ptr null) [ "funclet"(token %1) ]
+  catchret from %1 to label %catchret.dest
+
+catchret.dest:
+  ret void
+}
+
+declare void @opaque()
+declare void @llvm.objc.storeStrong(ptr, ptr) #0
+declare i32 @__CxxFrameHandler3(...)
+
+attributes #0 = { nounwind }
+
+; EH catchpad with SEH prologue:
+; CHECK: # %catch
+; CHECK: pushq   %rbp
+; CHECK: .seh_pushreg %rbp
+;...
+; CHECK: .seh_endprologue
+;
+; At this point the code used to be truncated (and sometimes terminated with an
+; int3 opcode):
+; CHECK-NOT: int3
+;
+; Instead, the call to objc_storeStrong should be emitted:
+; CHECK: leaq	-24(%rbp), %rcx
+; CHECK: xorl	%edx, %edx
+; CHECK: callq	objc_storeStrong
+;...
+;
+; This is the end of the 

[PATCH] D124762: [WinEHPrepare] Avoid truncation of EH funclets with GNUstep ObjC runtime

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz abandoned this revision.
sgraenitz added a comment.

Close in favor of the D128190 , which covers 
both cases at once: emit (was D124762 ) and 
inline (was D127962 )


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124762

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

[PATCH] D128190: [WinEH] Apply funclet operand bundles to nounwind intrinsics that lower to function calls

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

Running `check-llvm` and `check-clang` locally found no more failing tests. The 
issue is not limited to pre-ISel intrinsics (anymore) and I have to re-phrase a 
few comments. Apart from that, is there any more feedback? Thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128190

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

[PATCH] D128190: [WinEH] Apply funclet operand bundles to nounwind intrinsics that lower to function calls

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz updated this revision to Diff 438425.
sgraenitz added a comment.

Add missing `objc_sync_exit` to fix failing test 
Transforms/PreISelIntrinsicLowering/objc-arc.ll


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128190

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
  llvm/include/llvm/IR/IntrinsicInst.h
  llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
  llvm/lib/IR/IntrinsicInst.cpp
  llvm/lib/Transforms/Utils/InlineFunction.cpp
  llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll
  llvm/test/Feature/OperandBundles/inliner-funclet-wineh.ll

Index: llvm/test/Feature/OperandBundles/inliner-funclet-wineh.ll
===
--- /dev/null
+++ llvm/test/Feature/OperandBundles/inliner-funclet-wineh.ll
@@ -0,0 +1,55 @@
+; RUN: opt -S -always-inline -mtriple=x86_64-windows-msvc < %s | FileCheck %s
+
+; WinEH doesn't require funclet tokens on nounwind intrinsics per se. ObjC++ ARC
+; intrinsics are a special case, because they are subject to pre-ISel lowering.
+; They appear as regular function calls for subsequent passes, like WinEHPrepare
+; which would consider them implausible instrucitons and mark them unreachable.
+; Affected EH funclets would get truncated silently, which causes unpredictable
+; crashes at runtime.
+;
+; Thus, when we target WinEH and generate calls to pre-ISel intrinsics from EH
+; funclets, we emit funclet tokens explicitly.
+;
+; The inliner has to propagate funclet tokens to such intrinsics, if they get
+; inlined into EH funclets.
+
+define void @inlined_fn(ptr %ex) #1 {
+entry:
+  call void @llvm.objc.storeStrong(ptr %ex, ptr null)
+  ret void
+}
+
+define void @test_catch_with_inline() personality ptr @__CxxFrameHandler3 {
+entry:
+  %exn.slot = alloca ptr, align 8
+  %ex = alloca ptr, align 8
+  invoke void @opaque() to label %invoke.cont unwind label %catch.dispatch
+
+catch.dispatch:
+  %0 = catchswitch within none [label %catch] unwind to caller
+
+invoke.cont:
+  unreachable
+
+catch:
+  %1 = catchpad within %0 [ptr null, i32 64, ptr %exn.slot]
+  call void @inlined_fn(ptr %ex) [ "funclet"(token %1) ]
+  catchret from %1 to label %catchret.dest
+
+catchret.dest:
+  ret void
+}
+
+declare void @opaque()
+declare void @llvm.objc.storeStrong(ptr, ptr) #0
+declare i32 @__CxxFrameHandler3(...)
+
+attributes #0 = { nounwind }
+attributes #1 = { alwaysinline }
+
+; CHECK-LABEL:  define void @test_catch_with_inline()
+; ...
+; CHECK:catch:
+; CHECK-NEXT: %1 = catchpad within %0 [ptr null, i32 64, ptr %exn.slot]
+; CHECK-NEXT: call void @llvm.objc.storeStrong(ptr %ex, ptr null) [ "funclet"(token %1) ]
+; CHECK-NEXT: catchret from %1 to label %catchret.dest
Index: llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll
@@ -0,0 +1,68 @@
+; RUN: llc -mtriple=x86_64-windows-msvc < %s | FileCheck %s
+
+; Reduced IR generated from ObjC++ source:
+;
+; @class Ety;
+; void opaque(void);
+; void test_catch(void) {
+;   @try {
+; opaque();
+;   } @catch (Ety *ex) {
+; // Destroy ex when leaving catchpad. This emits calls to two intrinsic
+; // functions, llvm.objc.retain and llvm.objc.storeStrong, but only one
+; // is required to trigger the funclet truncation.
+;   }
+; }
+
+define void @test_catch() personality ptr @__CxxFrameHandler3 {
+entry:
+  %exn.slot = alloca ptr, align 8
+  %ex2 = alloca ptr, align 8
+  invoke void @opaque() to label %invoke.cont unwind label %catch.dispatch
+
+catch.dispatch:
+  %0 = catchswitch within none [label %catch] unwind to caller
+
+invoke.cont:
+  unreachable
+
+catch:
+  %1 = catchpad within %0 [ptr null, i32 64, ptr %exn.slot]
+  call void @llvm.objc.storeStrong(ptr %ex2, ptr null) [ "funclet"(token %1) ]
+  catchret from %1 to label %catchret.dest
+
+catchret.dest:
+  ret void
+}
+
+declare void @opaque()
+declare void @llvm.objc.storeStrong(ptr, ptr) #0
+declare i32 @__CxxFrameHandler3(...)
+
+attributes #0 = { nounwind }
+
+; llvm.objc.storeStrong is a Pre-ISel intrinsic, which used to cause truncations
+; when it occurred in SEH funclets like catchpads:
+; CHECK: # %catch
+; CHECK: pushq   %rbp
+; CHECK: .seh_pushreg %rbp
+;...
+; CHECK: .seh_endprologue
+;
+; At this point the code used to be truncated (and sometimes terminated with an
+; int3 opcode):
+; CHECK-NOT: int3
+;
+; Instead, the call to objc_storeStrong should be emitted:
+; CHECK: leaq	-24(%rbp), %rcx
+; CHECK: xorl	%edx, %edx
+; CHECK: callq	objc_storeStrong
+;...
+; 
+; This is the end of the funclet:
+; CHECK: popq	%rbp
+; CHECK: retq# CATCHRET
+;...
+;  

[PATCH] D128190: [WinEH] Apply funclet operand bundles to nounwind intrinsics that lower to function calls

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz created this revision.
sgraenitz added reviewers: rnk, theraven, DHowett-MSFT.
Herald added subscribers: jsji, pengfei, hiraditya.
Herald added a project: All.
sgraenitz requested review of this revision.
Herald added projects: clang, LLVM.
Herald added a subscriber: cfe-commits.

WinEHPrepare marks any function call from EH funclets as unreachable, if it's 
not a nounwind intrinsic and has no proper funclet bundle operand. This affects 
ARC intrinsics on Windows, because they are lowered to regular function calls 
in the PreISelIntrinsicLowering pass. It caused silent binary truncations and 
crashes during unwinding with the GNUstep ObjC runtime: 
https://github.com/gnustep/libobjc2/issues/222

This patch adds a new function `llvm::IntrinsicInst::mayLowerToFunctionCall()` 
that aims to collect all affected intrinsic IDs.

- Clang CodeGen uses it to determine whether or not it must emit a funclet 
bundle operand.
- PreISelIntrinsicLowering asserts that the function returns true for all ObjC 
runtime calls it lowers.
- LLVM uses it to determine whether or not a funclet bundle operand must be 
propagated to inlined call sites.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D128190

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
  llvm/include/llvm/IR/IntrinsicInst.h
  llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
  llvm/lib/IR/IntrinsicInst.cpp
  llvm/lib/Transforms/Utils/InlineFunction.cpp
  llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll
  llvm/test/Feature/OperandBundles/inliner-funclet-wineh.ll

Index: llvm/test/Feature/OperandBundles/inliner-funclet-wineh.ll
===
--- /dev/null
+++ llvm/test/Feature/OperandBundles/inliner-funclet-wineh.ll
@@ -0,0 +1,55 @@
+; RUN: opt -S -always-inline -mtriple=x86_64-windows-msvc < %s | FileCheck %s
+
+; WinEH doesn't require funclet tokens on nounwind intrinsics per se. ObjC++ ARC
+; intrinsics are a special case, because they are subject to pre-ISel lowering.
+; They appear as regular function calls for subsequent passes, like WinEHPrepare
+; which would consider them implausible instrucitons and mark them unreachable.
+; Affected EH funclets would get truncated silently, which causes unpredictable
+; crashes at runtime.
+;
+; Thus, when we target WinEH and generate calls to pre-ISel intrinsics from EH
+; funclets, we emit funclet tokens explicitly.
+;
+; The inliner has to propagate funclet tokens to such intrinsics, if they get
+; inlined into EH funclets.
+
+define void @inlined_fn(ptr %ex) #1 {
+entry:
+  call void @llvm.objc.storeStrong(ptr %ex, ptr null)
+  ret void
+}
+
+define void @test_catch_with_inline() personality ptr @__CxxFrameHandler3 {
+entry:
+  %exn.slot = alloca ptr, align 8
+  %ex = alloca ptr, align 8
+  invoke void @opaque() to label %invoke.cont unwind label %catch.dispatch
+
+catch.dispatch:
+  %0 = catchswitch within none [label %catch] unwind to caller
+
+invoke.cont:
+  unreachable
+
+catch:
+  %1 = catchpad within %0 [ptr null, i32 64, ptr %exn.slot]
+  call void @inlined_fn(ptr %ex) [ "funclet"(token %1) ]
+  catchret from %1 to label %catchret.dest
+
+catchret.dest:
+  ret void
+}
+
+declare void @opaque()
+declare void @llvm.objc.storeStrong(ptr, ptr) #0
+declare i32 @__CxxFrameHandler3(...)
+
+attributes #0 = { nounwind }
+attributes #1 = { alwaysinline }
+
+; CHECK-LABEL:  define void @test_catch_with_inline()
+; ...
+; CHECK:catch:
+; CHECK-NEXT: %1 = catchpad within %0 [ptr null, i32 64, ptr %exn.slot]
+; CHECK-NEXT: call void @llvm.objc.storeStrong(ptr %ex, ptr null) [ "funclet"(token %1) ]
+; CHECK-NEXT: catchret from %1 to label %catchret.dest
Index: llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll
@@ -0,0 +1,68 @@
+; RUN: llc -mtriple=x86_64-windows-msvc < %s | FileCheck %s
+
+; Reduced IR generated from ObjC++ source:
+;
+; @class Ety;
+; void opaque(void);
+; void test_catch(void) {
+;   @try {
+; opaque();
+;   } @catch (Ety *ex) {
+; // Destroy ex when leaving catchpad. This emits calls to two intrinsic
+; // functions, llvm.objc.retain and llvm.objc.storeStrong, but only one
+; // is required to trigger the funclet truncation.
+;   }
+; }
+
+define void @test_catch() personality ptr @__CxxFrameHandler3 {
+entry:
+  %exn.slot = alloca ptr, align 8
+  %ex2 = alloca ptr, align 8
+  invoke void @opaque() to label %invoke.cont unwind label %catch.dispatch
+
+catch.dispatch:
+  %0 = catchswitch within none [label %catch] unwind to caller
+
+invoke.cont:
+  unreachable
+
+catch:
+  %1 = catchpad within %0 [ptr null, i32 64, ptr %exn.slot]
+  call void @llvm.objc.storeStrong(ptr %ex2, ptr null) [ "funclet"(token %1) ]
+  catchret from %1 to label 

[PATCH] D124762: [WinEHPrepare] Avoid truncation of EH funclets with GNUstep ObjC runtime

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz updated this revision to Diff 437180.
sgraenitz added a comment.

Drop GNUstep assertion from getBundlesForFunclet(). Indeed a similar issue was 
observed before. See: D44640 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124762

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
  llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
  llvm/lib/CodeGen/WinEHPrepare.cpp
  llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll

Index: llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll
@@ -0,0 +1,68 @@
+; RUN: llc -mtriple=x86_64-windows-msvc < %s | FileCheck %s
+
+; Reduced IR generated from ObjC++ source:
+;
+; @class Ety;
+; void opaque(void);
+; void test_catch(void) {
+;   @try {
+; opaque();
+;   } @catch (Ety *ex) {
+; // Destroy ex when leaving catchpad. This emits calls to two intrinsic
+; // functions, llvm.objc.retain and llvm.objc.storeStrong, but only one
+; // is required to trigger the funclet truncation.
+;   }
+; }
+
+define void @test_catch() personality ptr @__CxxFrameHandler3 {
+entry:
+  %exn.slot = alloca ptr, align 8
+  %ex2 = alloca ptr, align 8
+  invoke void @opaque() to label %invoke.cont unwind label %catch.dispatch
+
+catch.dispatch:
+  %0 = catchswitch within none [label %catch] unwind to caller
+
+invoke.cont:
+  unreachable
+
+catch:
+  %1 = catchpad within %0 [ptr null, i32 64, ptr %exn.slot]
+  call void @llvm.objc.storeStrong(ptr %ex2, ptr null) [ "funclet"(token %1) ]
+  catchret from %1 to label %catchret.dest
+
+catchret.dest:
+  ret void
+}
+
+declare void @opaque()
+declare void @llvm.objc.storeStrong(ptr, ptr) #0
+declare i32 @__CxxFrameHandler3(...)
+
+attributes #0 = { nounwind }
+
+; llvm.objc.storeStrong is a Pre-ISel intrinsic, which used to cause truncations
+; when it occurred in SEH funclets like catchpads:
+; CHECK: # %catch
+; CHECK: pushq   %rbp
+; CHECK: .seh_pushreg %rbp
+;...
+; CHECK: .seh_endprologue
+;
+; At this point the code used to be truncated (and sometimes terminated with an
+; int3 opcode):
+; CHECK-NOT: int3
+;
+; Instead, the call to objc_storeStrong should be emitted:
+; CHECK: leaq	-24(%rbp), %rcx
+; CHECK: xorl	%edx, %edx
+; CHECK: callq	objc_storeStrong
+;...
+; 
+; This is the end of the funclet:
+; CHECK: popq	%rbp
+; CHECK: retq# CATCHRET
+;...
+; CHECK: .seh_handlerdata
+;...
+; CHECK: .seh_endproc
Index: llvm/lib/CodeGen/WinEHPrepare.cpp
===
--- llvm/lib/CodeGen/WinEHPrepare.cpp
+++ llvm/lib/CodeGen/WinEHPrepare.cpp
@@ -963,7 +963,7 @@
 if (auto BU = CB->getOperandBundle(LLVMContext::OB_funclet))
   FuncletBundleOperand = BU->Inputs.front();
 
-if (FuncletBundleOperand == FuncletPad)
+if (!FuncletPad || FuncletBundleOperand == FuncletPad)
   continue;
 
 // Skip call sites which are nounwind intrinsics or inline asm.
Index: llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
===
--- llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
+++ llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
@@ -107,7 +107,9 @@
 
 IRBuilder<> Builder(CI->getParent(), CI->getIterator());
 SmallVector Args(CI->args());
-CallInst *NewCI = Builder.CreateCall(FCache, Args);
+SmallVector BundleList;
+CI->getOperandBundlesAsDefs(BundleList);
+CallInst *NewCI = Builder.CreateCall(FCache, Args, BundleList);
 NewCI->setName(CI->getName());
 
 // Try to set the most appropriate TailCallKind based on both the current
Index: clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
===
--- /dev/null
+++ clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -emit-llvm -fobjc-arc -fexceptions -fobjc-exceptions -fobjc-runtime=gnustep-2.0 -o - %s | FileCheck %s
+
+@class Ety;
+void opaque(void);
+void test_catch_preisel_intrinsic(void) {
+  @try {
+opaque();
+  } @catch (Ety *ex) {
+// Destroy ex when leaving catchpad. Emits calls to two intrinsic functions,
+// that should both have a "funclet" bundle operand that refers to the
+// catchpad's SSA value.
+  }
+}
+
+// CHECK-LABEL: define{{.*}} void {{.*}}test_catch_preisel_intrinsic
+//...
+// CHECK:   catch.dispatch:
+// CHECK-NEXT:[[CATCHSWITCH:%[0-9]+]] = catchswitch within none
+//...
+// CHECK:   catch:
+// CHECK-NEXT:[[CATCHPAD:%[0-9]+]] = catchpad within 

[PATCH] D124762: [WinEHPrepare] Avoid truncation of EH funclets with GNUstep ObjC runtime

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz updated this revision to Diff 437179.
sgraenitz added a comment.

Fix accidental functional change that failed Clang test 
CodeGenCXX/microsoft-abi-eh-inlineasm.cpp


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124762

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
  llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
  llvm/lib/CodeGen/WinEHPrepare.cpp
  llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll

Index: llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll
@@ -0,0 +1,68 @@
+; RUN: llc -mtriple=x86_64-windows-msvc < %s | FileCheck %s
+
+; Reduced IR generated from ObjC++ source:
+;
+; @class Ety;
+; void opaque(void);
+; void test_catch(void) {
+;   @try {
+; opaque();
+;   } @catch (Ety *ex) {
+; // Destroy ex when leaving catchpad. This emits calls to two intrinsic
+; // functions, llvm.objc.retain and llvm.objc.storeStrong, but only one
+; // is required to trigger the funclet truncation.
+;   }
+; }
+
+define void @test_catch() personality ptr @__CxxFrameHandler3 {
+entry:
+  %exn.slot = alloca ptr, align 8
+  %ex2 = alloca ptr, align 8
+  invoke void @opaque() to label %invoke.cont unwind label %catch.dispatch
+
+catch.dispatch:
+  %0 = catchswitch within none [label %catch] unwind to caller
+
+invoke.cont:
+  unreachable
+
+catch:
+  %1 = catchpad within %0 [ptr null, i32 64, ptr %exn.slot]
+  call void @llvm.objc.storeStrong(ptr %ex2, ptr null) [ "funclet"(token %1) ]
+  catchret from %1 to label %catchret.dest
+
+catchret.dest:
+  ret void
+}
+
+declare void @opaque()
+declare void @llvm.objc.storeStrong(ptr, ptr) #0
+declare i32 @__CxxFrameHandler3(...)
+
+attributes #0 = { nounwind }
+
+; llvm.objc.storeStrong is a Pre-ISel intrinsic, which used to cause truncations
+; when it occurred in SEH funclets like catchpads:
+; CHECK: # %catch
+; CHECK: pushq   %rbp
+; CHECK: .seh_pushreg %rbp
+;...
+; CHECK: .seh_endprologue
+;
+; At this point the code used to be truncated (and sometimes terminated with an
+; int3 opcode):
+; CHECK-NOT: int3
+;
+; Instead, the call to objc_storeStrong should be emitted:
+; CHECK: leaq	-24(%rbp), %rcx
+; CHECK: xorl	%edx, %edx
+; CHECK: callq	objc_storeStrong
+;...
+; 
+; This is the end of the funclet:
+; CHECK: popq	%rbp
+; CHECK: retq# CATCHRET
+;...
+; CHECK: .seh_handlerdata
+;...
+; CHECK: .seh_endproc
Index: llvm/lib/CodeGen/WinEHPrepare.cpp
===
--- llvm/lib/CodeGen/WinEHPrepare.cpp
+++ llvm/lib/CodeGen/WinEHPrepare.cpp
@@ -963,7 +963,7 @@
 if (auto BU = CB->getOperandBundle(LLVMContext::OB_funclet))
   FuncletBundleOperand = BU->Inputs.front();
 
-if (FuncletBundleOperand == FuncletPad)
+if (!FuncletPad || FuncletBundleOperand == FuncletPad)
   continue;
 
 // Skip call sites which are nounwind intrinsics or inline asm.
Index: llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
===
--- llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
+++ llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
@@ -107,7 +107,9 @@
 
 IRBuilder<> Builder(CI->getParent(), CI->getIterator());
 SmallVector Args(CI->args());
-CallInst *NewCI = Builder.CreateCall(FCache, Args);
+SmallVector BundleList;
+CI->getOperandBundlesAsDefs(BundleList);
+CallInst *NewCI = Builder.CreateCall(FCache, Args, BundleList);
 NewCI->setName(CI->getName());
 
 // Try to set the most appropriate TailCallKind based on both the current
Index: clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
===
--- /dev/null
+++ clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -emit-llvm -fobjc-arc -fexceptions -fobjc-exceptions -fobjc-runtime=gnustep-2.0 -o - %s | FileCheck %s
+
+@class Ety;
+void opaque(void);
+void test_catch_preisel_intrinsic(void) {
+  @try {
+opaque();
+  } @catch (Ety *ex) {
+// Destroy ex when leaving catchpad. Emits calls to two intrinsic functions,
+// that should both have a "funclet" bundle operand that refers to the
+// catchpad's SSA value.
+  }
+}
+
+// CHECK-LABEL: define{{.*}} void {{.*}}test_catch_preisel_intrinsic
+//...
+// CHECK:   catch.dispatch:
+// CHECK-NEXT:[[CATCHSWITCH:%[0-9]+]] = catchswitch within none
+//...
+// CHECK:   catch:
+// CHECK-NEXT:[[CATCHPAD:%[0-9]+]] = catchpad within [[CATCHSWITCH]]
+// CHECK: {{%[0-9]+}} = 

[PATCH] D124762: [WinEHPrepare] Avoid truncation of EH funclets with GNUstep ObjC runtime

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz updated this revision to Diff 436730.
sgraenitz added a comment.

Update LLVM CodeGen test for mainline opaque pointers


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124762

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
  llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
  llvm/lib/CodeGen/WinEHPrepare.cpp
  llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll

Index: llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll
@@ -0,0 +1,68 @@
+; RUN: llc -mtriple=x86_64-windows-msvc < %s | FileCheck %s
+
+; Reduced IR generated from ObjC++ source:
+;
+; @class Ety;
+; void opaque(void);
+; void test_catch(void) {
+;   @try {
+; opaque();
+;   } @catch (Ety *ex) {
+; // Destroy ex when leaving catchpad. This emits calls to two intrinsic
+; // functions, llvm.objc.retain and llvm.objc.storeStrong, but only one
+; // is required to trigger the funclet truncation.
+;   }
+; }
+
+define void @test_catch() personality ptr @__CxxFrameHandler3 {
+entry:
+  %exn.slot = alloca ptr, align 8
+  %ex2 = alloca ptr, align 8
+  invoke void @opaque() to label %invoke.cont unwind label %catch.dispatch
+
+catch.dispatch:
+  %0 = catchswitch within none [label %catch] unwind to caller
+
+invoke.cont:
+  unreachable
+
+catch:
+  %1 = catchpad within %0 [ptr null, i32 64, ptr %exn.slot]
+  call void @llvm.objc.storeStrong(ptr %ex2, ptr null) [ "funclet"(token %1) ]
+  catchret from %1 to label %catchret.dest
+
+catchret.dest:
+  ret void
+}
+
+declare void @opaque()
+declare void @llvm.objc.storeStrong(ptr, ptr) #0
+declare i32 @__CxxFrameHandler3(...)
+
+attributes #0 = { nounwind }
+
+; llvm.objc.storeStrong is a Pre-ISel intrinsic, which used to cause truncations
+; when it occurred in SEH funclets like catchpads:
+; CHECK: # %catch
+; CHECK: pushq   %rbp
+; CHECK: .seh_pushreg %rbp
+;...
+; CHECK: .seh_endprologue
+;
+; At this point the code used to be truncated (and sometimes terminated with an
+; int3 opcode):
+; CHECK-NOT: int3
+;
+; Instead, the call to objc_storeStrong should be emitted:
+; CHECK: leaq	-24(%rbp), %rcx
+; CHECK: xorl	%edx, %edx
+; CHECK: callq	objc_storeStrong
+;...
+; 
+; This is the end of the funclet:
+; CHECK: popq	%rbp
+; CHECK: retq# CATCHRET
+;...
+; CHECK: .seh_handlerdata
+;...
+; CHECK: .seh_endproc
Index: llvm/lib/CodeGen/WinEHPrepare.cpp
===
--- llvm/lib/CodeGen/WinEHPrepare.cpp
+++ llvm/lib/CodeGen/WinEHPrepare.cpp
@@ -963,7 +963,7 @@
 if (auto BU = CB->getOperandBundle(LLVMContext::OB_funclet))
   FuncletBundleOperand = BU->Inputs.front();
 
-if (FuncletBundleOperand == FuncletPad)
+if (!FuncletPad || FuncletBundleOperand == FuncletPad)
   continue;
 
 // Skip call sites which are nounwind intrinsics or inline asm.
Index: llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
===
--- llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
+++ llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
@@ -107,7 +107,9 @@
 
 IRBuilder<> Builder(CI->getParent(), CI->getIterator());
 SmallVector Args(CI->args());
-CallInst *NewCI = Builder.CreateCall(FCache, Args);
+SmallVector BundleList;
+CI->getOperandBundlesAsDefs(BundleList);
+CallInst *NewCI = Builder.CreateCall(FCache, Args, BundleList);
 NewCI->setName(CI->getName());
 
 // Try to set the most appropriate TailCallKind based on both the current
Index: clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
===
--- /dev/null
+++ clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -emit-llvm -fobjc-arc -fexceptions -fobjc-exceptions -fobjc-runtime=gnustep-2.0 -o - %s | FileCheck %s
+
+@class Ety;
+void opaque(void);
+void test_catch_preisel_intrinsic(void) {
+  @try {
+opaque();
+  } @catch (Ety *ex) {
+// Destroy ex when leaving catchpad. Emits calls to two intrinsic functions,
+// that should both have a "funclet" bundle operand that refers to the
+// catchpad's SSA value.
+  }
+}
+
+// CHECK-LABEL: define{{.*}} void {{.*}}test_catch_preisel_intrinsic
+//...
+// CHECK:   catch.dispatch:
+// CHECK-NEXT:[[CATCHSWITCH:%[0-9]+]] = catchswitch within none
+//...
+// CHECK:   catch:
+// CHECK-NEXT:[[CATCHPAD:%[0-9]+]] = catchpad within [[CATCHSWITCH]]
+// CHECK: {{%[0-9]+}} = call {{.*}} @llvm.objc.retain{{.*}} [ 

[PATCH] D124762: [WinEHPrepare] Avoid truncation of EH funclets with GNUstep ObjC runtime

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

For illustration, truncations look like this:

F23303573: Screenshot 2022-06-02 at 15.35.19.png 


F23303572: Screenshot 2022-06-02 at 19.06.41.png 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124762

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

[PATCH] D124762: [WinEHPrepare] Avoid truncation of EH funclets with GNUstep ObjC runtime

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz marked an inline comment as done.
sgraenitz added a comment.

My follow-up got delayed, because I hit another bug, which appears to be a 
regression in release 14. This is why I wrote the tests for release/13.x and I 
still have to port them back to mainline, so this is *not yet ready to land*. 
As I don't expect it to cause big changes, however, I though it might be good 
to update the review anyway.

Rebased on release/13.x I am running the tests like this and both are passing:

  > ninja llvm-config llvm-readobj llc FileCheck count not
  > bin\llvm-lit.py --filter=win64-funclet-preisel-intrinsics test\CodeGen\X86
  > ninja clang
  > bin\llvm-lit.py --filter=arc-exceptions-seh tools\clang\test

@rnk What do you think about implementation, naming, etc. Is that ok?

In D124762#3486288 , @rnk wrote:

> Lastly, I think the inliner needs to be taught that these intrinsics need to 
> carry operand bundles, otherwise this problem will arise after optimization. 
> The inliner has the same logic here:
> https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Utils/InlineFunction.cpp#L2311

Thanks for the note. Yes, I can reproduce that. I am not yet sure what's the 
best way to fix it though. Actually, I'd prefer to split it out into a separate 
review, so the two discussions can remain their individual focus. What do you 
think?




Comment at: clang/test/CodeGenObjCXX/arc-exceptions-seh.mm:1
+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -emit-llvm -fobjc-arc 
-fexceptions -fobjc-exceptions -fobjc-runtime=gnustep-2.0 -o - %s | FileCheck %s
+

Note: There is another test without the `-seh` postfix that checks very similar 
situations for CodeGen with the macOS runtime.



Comment at: llvm/lib/CodeGen/WinEHPrepare.cpp:966
 
-if (FuncletBundleOperand == FuncletPad)
+if (!FuncletPad || FuncletBundleOperand == FuncletPad)
   continue;

sgraenitz wrote:
> rnk wrote:
> > Is this change necessary? It seems like it shouldn't be after the clang and 
> > preisel pass changes.
> Yes, apparently it is necessary. There are cases, where 
> `CodeGenFunction::getBundlesForFunclet()` registers a "funclet" bundle 
> operand, but the `FuncletPad` here is null. My guess was it might be due to 
> optimizations?
Might that be due to the inliner issue then? I'd address it in the follow-up 
review if it's ok?



Comment at: llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll:53
+; At this point the code used to be truncated:
+; CHECK-NOT: int3
+;

Another side-note: Often times Clang just didn't emit anything here, so 
execution would run into whatever code comes next. The `int3` I sometimes got 
might as well have been "accidental" padding.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124762

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

[PATCH] D124762: [WinEHPrepare] Avoid truncation of EH funclets with GNUstep ObjC runtime

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz updated this revision to Diff 434024.
sgraenitz added a comment.

Clang frontend: check that 'funclet' bundle operands are emitted for Pre-ISel 
intrinsics


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124762

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
  llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
  llvm/lib/CodeGen/WinEHPrepare.cpp
  llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll

Index: llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll
@@ -0,0 +1,67 @@
+; RUN: llc -mtriple=x86_64-windows-msvc < %s | FileCheck %s
+
+; Reduced IR generated from ObjC++ source:
+;
+; @class Ety;
+; void opaque(void);
+; void test_catch(void) {
+;   @try {
+; opaque();
+;   } @catch (Ety *ex) {
+; // Destroy ex when leaving catchpad. This emits calls to two intrinsic
+; // functions, llvm.objc.retain and llvm.objc.storeStrong, but only one
+; // is required to trigger the funclet truncation.
+;   }
+; }
+
+define void @test_catch() personality i8* bitcast (i32 (...)* @__CxxFrameHandler3 to i8*) {
+entry:
+  %exn.slot = alloca i8*, align 8
+  %ex2 = alloca i8*, align 8
+  invoke void @opaque() to label %invoke.cont unwind label %catch.dispatch
+
+catch.dispatch:
+  %0 = catchswitch within none [label %catch] unwind to caller
+
+invoke.cont:
+  unreachable
+
+catch:
+  %1 = catchpad within %0 [i8* null, i32 64, i8** %exn.slot]
+  call void @llvm.objc.storeStrong(i8** %ex2, i8* null) [ "funclet"(token %1) ]
+  catchret from %1 to label %catchret.dest
+
+catchret.dest:
+  ret void
+}
+
+declare void @opaque()
+declare void @llvm.objc.storeStrong(i8**, i8*) #0
+declare i32 @__CxxFrameHandler3(...)
+
+attributes #0 = { nounwind }
+
+; llvm.objc.storeStrong is a Pre-ISel intrinsic, which used to cause truncations
+; when it occurred in SEH funclets like catchpads:
+; CHECK: # %catch
+; CHECK: pushq   %rbp
+; CHECK: .seh_pushreg %rbp
+;...
+; CHECK: .seh_endprologue
+;
+; At this point the code used to be truncated:
+; CHECK-NOT: int3
+;
+; Instead, the call to objc_storeStrong should be emitted:
+; CHECK: leaq	-24(%rbp), %rcx
+; CHECK: xorl	%edx, %edx
+; CHECK: callq	objc_storeStrong
+;...
+; 
+; This is the end of the funclet:
+; CHECK: popq	%rbp
+; CHECK: retq# CATCHRET
+;...
+; CHECK: .seh_handlerdata
+;...
+; CHECK: .seh_endproc
Index: llvm/lib/CodeGen/WinEHPrepare.cpp
===
--- llvm/lib/CodeGen/WinEHPrepare.cpp
+++ llvm/lib/CodeGen/WinEHPrepare.cpp
@@ -963,7 +963,7 @@
 if (auto BU = CB->getOperandBundle(LLVMContext::OB_funclet))
   FuncletBundleOperand = BU->Inputs.front();
 
-if (FuncletBundleOperand == FuncletPad)
+if (!FuncletPad || FuncletBundleOperand == FuncletPad)
   continue;
 
 // Skip call sites which are nounwind intrinsics or inline asm.
Index: llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
===
--- llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
+++ llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
@@ -107,7 +107,9 @@
 
 IRBuilder<> Builder(CI->getParent(), CI->getIterator());
 SmallVector Args(CI->args());
-CallInst *NewCI = Builder.CreateCall(FCache, Args);
+SmallVector BundleList;
+CI->getOperandBundlesAsDefs(BundleList);
+CallInst *NewCI = Builder.CreateCall(FCache, Args, BundleList);
 NewCI->setName(CI->getName());
 
 // Try to set the most appropriate TailCallKind based on both the current
Index: clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
===
--- /dev/null
+++ clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -emit-llvm -fobjc-arc -fexceptions -fobjc-exceptions -fobjc-runtime=gnustep-2.0 -o - %s | FileCheck %s
+
+@class Ety;
+void opaque(void);
+void test_catch_preisel_intrinsic(void) {
+  @try {
+opaque();
+  } @catch (Ety *ex) {
+// Destroy ex when leaving catchpad. Emits calls to two intrinsic functions,
+// that should both have a "funclet" bundle operand that refers to the
+// catchpad's SSA value.
+  }
+}
+
+// CHECK-LABEL: define{{.*}} void {{.*}}test_catch_preisel_intrinsic
+//...
+// CHECK:   catch.dispatch:
+// CHECK-NEXT:[[CATCHSWITCH:%[0-9]+]] = catchswitch within none
+//...
+// CHECK:   catch:
+// CHECK-NEXT:[[CATCHPAD:%[0-9]+]] = catchpad within [[CATCHSWITCH]]
+// CHECK: {{%[0-9]+}} = call {{.*}} 

[PATCH] D124762: [WinEHPrepare] Avoid truncation of EH funclets with GNUstep ObjC runtime

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz updated this revision to Diff 434023.
sgraenitz added a comment.
Herald added subscribers: jsji, pengfei.

LLVM CodeGen: check that presence of bundle operands avoids truncation


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124762

Files:
  clang/lib/CodeGen/CGCall.cpp
  llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
  llvm/lib/CodeGen/WinEHPrepare.cpp
  llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll

Index: llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll
@@ -0,0 +1,67 @@
+; RUN: llc -mtriple=x86_64-windows-msvc < %s | FileCheck %s
+
+; Reduced IR generated from ObjC++ source:
+;
+; @class Ety;
+; void opaque(void);
+; void test_catch(void) {
+;   @try {
+; opaque();
+;   } @catch (Ety *ex) {
+; // Destroy ex when leaving catchpad. This emits calls to two intrinsic
+; // functions, llvm.objc.retain and llvm.objc.storeStrong, but only one
+; // is required to trigger the funclet truncation.
+;   }
+; }
+
+define void @test_catch() personality i8* bitcast (i32 (...)* @__CxxFrameHandler3 to i8*) {
+entry:
+  %exn.slot = alloca i8*, align 8
+  %ex2 = alloca i8*, align 8
+  invoke void @opaque() to label %invoke.cont unwind label %catch.dispatch
+
+catch.dispatch:
+  %0 = catchswitch within none [label %catch] unwind to caller
+
+invoke.cont:
+  unreachable
+
+catch:
+  %1 = catchpad within %0 [i8* null, i32 64, i8** %exn.slot]
+  call void @llvm.objc.storeStrong(i8** %ex2, i8* null) [ "funclet"(token %1) ]
+  catchret from %1 to label %catchret.dest
+
+catchret.dest:
+  ret void
+}
+
+declare void @opaque()
+declare void @llvm.objc.storeStrong(i8**, i8*) #0
+declare i32 @__CxxFrameHandler3(...)
+
+attributes #0 = { nounwind }
+
+; llvm.objc.storeStrong is a Pre-ISel intrinsic, which used to cause truncations
+; when it occurred in SEH funclets like catchpads:
+; CHECK: # %catch
+; CHECK: pushq   %rbp
+; CHECK: .seh_pushreg %rbp
+;...
+; CHECK: .seh_endprologue
+;
+; At this point the code used to be truncated:
+; CHECK-NOT: int3
+;
+; Instead, the call to objc_storeStrong should be emitted:
+; CHECK: leaq	-24(%rbp), %rcx
+; CHECK: xorl	%edx, %edx
+; CHECK: callq	objc_storeStrong
+;...
+; 
+; This is the end of the funclet:
+; CHECK: popq	%rbp
+; CHECK: retq# CATCHRET
+;...
+; CHECK: .seh_handlerdata
+;...
+; CHECK: .seh_endproc
Index: llvm/lib/CodeGen/WinEHPrepare.cpp
===
--- llvm/lib/CodeGen/WinEHPrepare.cpp
+++ llvm/lib/CodeGen/WinEHPrepare.cpp
@@ -963,7 +963,7 @@
 if (auto BU = CB->getOperandBundle(LLVMContext::OB_funclet))
   FuncletBundleOperand = BU->Inputs.front();
 
-if (FuncletBundleOperand == FuncletPad)
+if (!FuncletPad || FuncletBundleOperand == FuncletPad)
   continue;
 
 // Skip call sites which are nounwind intrinsics or inline asm.
Index: llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
===
--- llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
+++ llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
@@ -107,7 +107,9 @@
 
 IRBuilder<> Builder(CI->getParent(), CI->getIterator());
 SmallVector Args(CI->args());
-CallInst *NewCI = Builder.CreateCall(FCache, Args);
+SmallVector BundleList;
+CI->getOperandBundlesAsDefs(BundleList);
+CallInst *NewCI = Builder.CreateCall(FCache, Args, BundleList);
 NewCI->setName(CI->getName());
 
 // Try to set the most appropriate TailCallKind based on both the current
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -25,11 +25,13 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/Basic/CodeGenOptions.h"
+#include "clang/Basic/ObjCRuntime.h"
 #include "clang/Basic/TargetBuiltins.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
 #include "clang/CodeGen/SwiftCallingConv.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/Analysis/ObjCARCInstKind.h"
 #include "llvm/Analysis/ValueTracking.h"
 #include "llvm/IR/Assumptions.h"
 #include "llvm/IR/Attributes.h"
@@ -4465,16 +4467,37 @@
 CodeGenFunction::getBundlesForFunclet(llvm::Value *Callee) {
   SmallVector BundleList;
   // There is no need for a funclet operand bundle if we aren't inside a
-  // funclet.
+  // funclet or the callee is not a function.
   if (!CurrentFuncletPad)
 return BundleList;
-
-  // Skip intrinsics which cannot throw.
   auto *CalleeFn = 

[PATCH] D124762: [WinEHPrepare] Avoid truncation of EH funclets with GNUstep ObjC runtime

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz updated this revision to Diff 434020.
sgraenitz added a comment.

Fix unchecked nullptr compiler crash and assertion


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124762

Files:
  clang/lib/CodeGen/CGCall.cpp
  llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
  llvm/lib/CodeGen/WinEHPrepare.cpp


Index: llvm/lib/CodeGen/WinEHPrepare.cpp
===
--- llvm/lib/CodeGen/WinEHPrepare.cpp
+++ llvm/lib/CodeGen/WinEHPrepare.cpp
@@ -963,7 +963,7 @@
 if (auto BU = CB->getOperandBundle(LLVMContext::OB_funclet))
   FuncletBundleOperand = BU->Inputs.front();
 
-if (FuncletBundleOperand == FuncletPad)
+if (!FuncletPad || FuncletBundleOperand == FuncletPad)
   continue;
 
 // Skip call sites which are nounwind intrinsics or inline asm.
Index: llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
===
--- llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
+++ llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
@@ -107,7 +107,9 @@
 
 IRBuilder<> Builder(CI->getParent(), CI->getIterator());
 SmallVector Args(CI->args());
-CallInst *NewCI = Builder.CreateCall(FCache, Args);
+SmallVector BundleList;
+CI->getOperandBundlesAsDefs(BundleList);
+CallInst *NewCI = Builder.CreateCall(FCache, Args, BundleList);
 NewCI->setName(CI->getName());
 
 // Try to set the most appropriate TailCallKind based on both the current
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -25,11 +25,13 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/Basic/CodeGenOptions.h"
+#include "clang/Basic/ObjCRuntime.h"
 #include "clang/Basic/TargetBuiltins.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
 #include "clang/CodeGen/SwiftCallingConv.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/Analysis/ObjCARCInstKind.h"
 #include "llvm/Analysis/ValueTracking.h"
 #include "llvm/IR/Assumptions.h"
 #include "llvm/IR/Attributes.h"
@@ -4465,16 +4467,37 @@
 CodeGenFunction::getBundlesForFunclet(llvm::Value *Callee) {
   SmallVector BundleList;
   // There is no need for a funclet operand bundle if we aren't inside a
-  // funclet.
+  // funclet or the callee is not a function.
   if (!CurrentFuncletPad)
 return BundleList;
-
-  // Skip intrinsics which cannot throw.
   auto *CalleeFn = dyn_cast(Callee->stripPointerCasts());
-  if (CalleeFn && CalleeFn->isIntrinsic() && CalleeFn->doesNotThrow())
+  if (!CalleeFn)
 return BundleList;
 
-  BundleList.emplace_back("funclet", CurrentFuncletPad);
+  // Skip intrinsics which cannot throw.
+  bool InsertFuncletOp = true;
+  if (CalleeFn->isIntrinsic() && CalleeFn->doesNotThrow())
+InsertFuncletOp = false;
+
+  // Most ObjC ARC intrinics are lowered in PreISelIntrinsicLowering. Thus,
+  // WinEHPrepare will see them as regular calls. We need to set the funclet
+  // operand explicitly in this case to avoid accidental truncation of EH
+  // funclets on Windows.
+  if (CalleeFn->isIntrinsic() && CalleeFn->doesNotThrow()) {
+if (CGM.getTarget().getTriple().isOSWindows()) {
+  assert(CGM.getLangOpts().ObjCRuntime.getKind() == ObjCRuntime::GNUstep &&
+ "Only reproduced with GNUstep so far, but likely applies to other 
"
+ "ObjC runtimes on Windows");
+  using namespace llvm::objcarc;
+  ARCInstKind CalleeKind = GetFunctionClass(CalleeFn);
+  if (!IsUser(CalleeKind) && CalleeKind != ARCInstKind::None)
+InsertFuncletOp = true;
+}
+  }
+
+  if (InsertFuncletOp)
+BundleList.emplace_back("funclet", CurrentFuncletPad);
+
   return BundleList;
 }
 


Index: llvm/lib/CodeGen/WinEHPrepare.cpp
===
--- llvm/lib/CodeGen/WinEHPrepare.cpp
+++ llvm/lib/CodeGen/WinEHPrepare.cpp
@@ -963,7 +963,7 @@
 if (auto BU = CB->getOperandBundle(LLVMContext::OB_funclet))
   FuncletBundleOperand = BU->Inputs.front();
 
-if (FuncletBundleOperand == FuncletPad)
+if (!FuncletPad || FuncletBundleOperand == FuncletPad)
   continue;
 
 // Skip call sites which are nounwind intrinsics or inline asm.
Index: llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
===
--- llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
+++ llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
@@ -107,7 +107,9 @@
 
 IRBuilder<> Builder(CI->getParent(), CI->getIterator());
 SmallVector Args(CI->args());
-CallInst *NewCI = Builder.CreateCall(FCache, Args);
+SmallVector BundleList;
+CI->getOperandBundlesAsDefs(BundleList);
+CallInst *NewCI = Builder.CreateCall(FCache, Args, 

[PATCH] D124762: [WinEHPrepare] Avoid truncation of EH funclets with GNUstep ObjC runtime

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

Thanks for your help! I will work on testing in calendar week 21.




Comment at: llvm/lib/CodeGen/WinEHPrepare.cpp:966
 
-if (FuncletBundleOperand == FuncletPad)
+if (!FuncletPad || FuncletBundleOperand == FuncletPad)
   continue;

rnk wrote:
> Is this change necessary? It seems like it shouldn't be after the clang and 
> preisel pass changes.
Yes, apparently it is necessary. There are cases, where 
`CodeGenFunction::getBundlesForFunclet()` registers a "funclet" bundle operand, 
but the `FuncletPad` here is null. My guess was it might be due to 
optimizations?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124762

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

[PATCH] D124762: [WinEHPrepare] Avoid truncation of EH funclets with GNUstep ObjC runtime

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

I guess testing must be split in two:

- Clang wants to make sure the "funclet" bundle operand gets emitted for ObjC 
ARC runtime calls on Windows. Maybe that fits into 
clang/test/CodeGenObjC/gnu-exceptions.m 
?
- LLVM wants to check that WinEHPrepare handles these runtime calls correctly. 
For that maybe I can use llvm/test/CodeGen/X86/win-funclet-cfi.ll 

 as a template and adjust it for the ObjC ARC case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124762

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

[PATCH] D124762: [WinEHPrepare] Avoid truncation of EH funclets with GNUstep ObjC runtime

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz created this revision.
sgraenitz added reviewers: rnk, theraven, DHowett-MSFT.
Herald added a subscriber: hiraditya.
Herald added a project: All.
sgraenitz requested review of this revision.
Herald added projects: clang, LLVM.
Herald added a subscriber: cfe-commits.

Unwinding ObjC code with automatic reference counting involves calls to 
intrinsics like `llvm.obj.retain` and `llvm.objc.destroyWeak`. Exception 
handling on Windows is based on funclets and WinEHPrepare only allows calls to 
`nounwind` intrinsics. This works just fine, except for ObjC `nounwind` 
intrinsics, because these are lowered into regular function calls in an earlier 
stage already (i.e. PreISelIntrinsicLoweringPass). Thus, WinEHPrepare 
accidentally drops them as implausible instructions and silently truncates 
certain funclets. Eventually, this causes crashes during unwinding on Windows 
with the GNUstep ObjC runtime: https://github.com/gnustep/libobjc2/issues/222

This patch forces the emission of a "funclet" bundle operand for calls to ObjC 
ARC intrinsics during codegen and lets PreISelIntrinsicLowering carry it over 
onto lowered replacement calls. This way all ObjC ARC calls survive 
WinEHPrepare through the FuncletBundleOperand check. The latter had to be 
adjusted in order to allow funclet pads to get optimized away.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124762

Files:
  clang/lib/CodeGen/CGCall.cpp
  llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
  llvm/lib/CodeGen/WinEHPrepare.cpp


Index: llvm/lib/CodeGen/WinEHPrepare.cpp
===
--- llvm/lib/CodeGen/WinEHPrepare.cpp
+++ llvm/lib/CodeGen/WinEHPrepare.cpp
@@ -963,7 +963,7 @@
 if (auto BU = CB->getOperandBundle(LLVMContext::OB_funclet))
   FuncletBundleOperand = BU->Inputs.front();
 
-if (FuncletBundleOperand == FuncletPad)
+if (!FuncletPad || FuncletBundleOperand == FuncletPad)
   continue;
 
 // Skip call sites which are nounwind intrinsics or inline asm.
Index: llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
===
--- llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
+++ llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
@@ -107,7 +107,9 @@
 
 IRBuilder<> Builder(CI->getParent(), CI->getIterator());
 SmallVector Args(CI->args());
-CallInst *NewCI = Builder.CreateCall(FCache, Args);
+SmallVector BundleList;
+CI->getOperandBundlesAsDefs(BundleList);
+CallInst *NewCI = Builder.CreateCall(FCache, Args, BundleList);
 NewCI->setName(CI->getName());
 
 // Try to set the most appropriate TailCallKind based on both the current
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -25,11 +25,13 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/Basic/CodeGenOptions.h"
+#include "clang/Basic/ObjCRuntime.h"
 #include "clang/Basic/TargetBuiltins.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
 #include "clang/CodeGen/SwiftCallingConv.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/Analysis/ObjCARCInstKind.h"
 #include "llvm/Analysis/ValueTracking.h"
 #include "llvm/IR/Assumptions.h"
 #include "llvm/IR/Attributes.h"
@@ -4470,11 +4472,30 @@
 return BundleList;
 
   // Skip intrinsics which cannot throw.
+  bool InsertFuncletOp = true;
   auto *CalleeFn = dyn_cast(Callee->stripPointerCasts());
   if (CalleeFn && CalleeFn->isIntrinsic() && CalleeFn->doesNotThrow())
-return BundleList;
+InsertFuncletOp = false;
+
+  // ObjC ARC intrinics are lowered in PreISelIntrinsicLowering. Thus,
+  // WinEHPrepare will see them as regular calls. We need to set the funclet
+  // operand explicitly in this case to avoid accidental truncation of EH
+  // funclets on Windows.
+  using namespace llvm::objcarc;
+  if (GetFunctionClass(CalleeFn) != ARCInstKind::None) {
+assert(CalleeFn->isIntrinsic() && CalleeFn->doesNotThrow() &&
+   "Right now these are nounwind intrinsics");
+if (CGM.getTarget().getTriple().isOSWindows()) {
+  assert(CGM.getLangOpts().ObjCRuntime.getKind() == ObjCRuntime::GNUstep &&
+ "Only reproduced with GNUstep so far, but likely applies to other 
"
+ "ObjC runtimes on Windows");
+  InsertFuncletOp = true;
+}
+  }
+
+  if (InsertFuncletOp)
+BundleList.emplace_back("funclet", CurrentFuncletPad);
 
-  BundleList.emplace_back("funclet", CurrentFuncletPad);
   return BundleList;
 }
 


Index: llvm/lib/CodeGen/WinEHPrepare.cpp
===
--- llvm/lib/CodeGen/WinEHPrepare.cpp
+++ llvm/lib/CodeGen/WinEHPrepare.cpp
@@ -963,7 +963,7 @@
 if (auto BU = CB->getOperandBundle(LLVMContext::OB_funclet))
   FuncletBundleOperand = 

[PATCH] D110484: [clang-repl] Allow loading of plugins in clang-repl.

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

Two minor comments. Otherwise LGTM.




Comment at: clang/include/clang/Frontend/CompilerInstance.h:223
+  void LoadRequestedPlugins();
+
   /// }

This could have a minimal doxygen comment now that it's part of the public 
interface.



Comment at: clang/test/Interpreter/plugins.cpp:5
+// RUN:'extern "C" int printf(const char*,...);' \
+// RUN:'auto r1 = printf("i = %d\n", i);' 2>&1 | FileCheck %s
+// REQUIRES: host-supports-jit, plugins, examples

You could write the code interleaved with the check-lines below and then do:
```
// RUN: cat %s | clang-repl | FileCheck %s
```

Is there a good reason not to do so?


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

https://reviews.llvm.org/D110484

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

[PATCH] D110260: [ORC] Minor renaming and typo fixes (NFC)

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

FYI Included one more typo fix in ExecutorAddress.h


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110260

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

[PATCH] D110260: [ORC] Minor renaming and typo fixes (NFC)

[Stefan Gränitz via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG767b328e506e: [ORC] Minor renaming and typo fixes (NFC) 
(authored by sgraenitz).

Changed prior to commit:
  https://reviews.llvm.org/D110260?vs=374251=374646#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110260

Files:
  clang/docs/ClangFormattedStatus.rst
  llvm/examples/OrcV2Examples/CMakeLists.txt
  llvm/examples/OrcV2Examples/LLJITWithExecutorProcessControl/CMakeLists.txt
  
llvm/examples/OrcV2Examples/LLJITWithExecutorProcessControl/LLJITWithExecutorProcessControl.cpp
  llvm/examples/OrcV2Examples/LLJITWithTargetProcessControl/CMakeLists.txt
  
llvm/examples/OrcV2Examples/LLJITWithTargetProcessControl/LLJITWithTargetProcessControl.cpp
  llvm/include/llvm/ExecutionEngine/Orc/Shared/ExecutorAddress.h
  llvm/include/llvm/ExecutionEngine/Orc/SimpleRemoteEPC.h
  llvm/lib/ExecutionEngine/Orc/Shared/SimpleRemoteEPCUtils.cpp


Index: llvm/lib/ExecutionEngine/Orc/Shared/SimpleRemoteEPCUtils.cpp
===
--- llvm/lib/ExecutionEngine/Orc/Shared/SimpleRemoteEPCUtils.cpp
+++ llvm/lib/ExecutionEngine/Orc/Shared/SimpleRemoteEPCUtils.cpp
@@ -223,7 +223,7 @@
 
 if (MsgSize < FDMsgHeader::Size) {
   Err = joinErrors(std::move(Err),
-   make_error("Mesasge size too small",
+   make_error("Message size too small",
inconvertibleErrorCode()));
   break;
 }
Index: llvm/include/llvm/ExecutionEngine/Orc/SimpleRemoteEPC.h
===
--- llvm/include/llvm/ExecutionEngine/Orc/SimpleRemoteEPC.h
+++ llvm/include/llvm/ExecutionEngine/Orc/SimpleRemoteEPC.h
@@ -15,7 +15,6 @@
 
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/FunctionExtras.h"
-#include "llvm/ExecutionEngine/Orc/EPCGenericJITLinkMemoryManager.h"
 #include "llvm/ExecutionEngine/Orc/EPCGenericMemoryAccess.h"
 #include "llvm/ExecutionEngine/Orc/ExecutorProcessControl.h"
 #include "llvm/ExecutionEngine/Orc/Shared/SimpleRemoteEPCUtils.h"
Index: llvm/include/llvm/ExecutionEngine/Orc/Shared/ExecutorAddress.h
===
--- llvm/include/llvm/ExecutionEngine/Orc/Shared/ExecutorAddress.h
+++ llvm/include/llvm/ExecutionEngine/Orc/Shared/ExecutorAddress.h
@@ -47,7 +47,7 @@
   }
 
   /// Cast this ExecutorAddress to a pointer of the given type.
-  /// Warning: This should only be esude when JITing in-process.
+  /// Warning: This should only be used when JITing in-process.
   template  T toPtr() const {
 static_assert(std::is_pointer::value, "T must be a pointer type");
 uintptr_t IntPtr = static_cast(Addr);
Index: 
llvm/examples/OrcV2Examples/LLJITWithExecutorProcessControl/CMakeLists.txt
===
--- llvm/examples/OrcV2Examples/LLJITWithExecutorProcessControl/CMakeLists.txt
+++ llvm/examples/OrcV2Examples/LLJITWithExecutorProcessControl/CMakeLists.txt
@@ -7,6 +7,6 @@
   nativecodegen
   )
 
-add_llvm_example(LLJITWithTargetProcessControl
-  LLJITWithTargetProcessControl.cpp
+add_llvm_example(LLJITWithExecutorProcessControl
+  LLJITWithExecutorProcessControl.cpp
   )
Index: llvm/examples/OrcV2Examples/CMakeLists.txt
===
--- llvm/examples/OrcV2Examples/CMakeLists.txt
+++ llvm/examples/OrcV2Examples/CMakeLists.txt
@@ -1,12 +1,12 @@
 add_subdirectory(LLJITDumpObjects)
 add_subdirectory(LLJITWithCustomObjectLinkingLayer)
+add_subdirectory(LLJITWithExecutorProcessControl)
 add_subdirectory(LLJITWithGDBRegistrationListener)
 add_subdirectory(LLJITWithInitializers)
 add_subdirectory(LLJITWithLazyReexports)
 add_subdirectory(LLJITWithObjectCache)
 add_subdirectory(LLJITWithObjectLinkingLayerPlugin)
 add_subdirectory(LLJITWithOptimizingIRTransform)
-add_subdirectory(LLJITWithTargetProcessControl)
 add_subdirectory(LLJITWithThinLTOSummaries)
 add_subdirectory(OrcV2CBindingsAddObjectFile)
 add_subdirectory(OrcV2CBindingsBasicUsage)
Index: clang/docs/ClangFormattedStatus.rst
===
--- clang/docs/ClangFormattedStatus.rst
+++ clang/docs/ClangFormattedStatus.rst
@@ -3704,7 +3704,7 @@
  - `3`
  - `0`
  - :good:`100%`
-   * - llvm/examples/OrcV2Examples/LLJITWithTargetProcessControl
+   * - llvm/examples/OrcV2Examples/LLJITWithExecutorProcessControl
  - `1`
  - `1`
  - `0`


Index: llvm/lib/ExecutionEngine/Orc/Shared/SimpleRemoteEPCUtils.cpp
===
--- llvm/lib/ExecutionEngine/Orc/Shared/SimpleRemoteEPCUtils.cpp
+++ llvm/lib/ExecutionEngine/Orc/Shared/SimpleRemoteEPCUtils.cpp
@@ -223,7 +223,7 @@
 
 if 

[PATCH] D110260: [ORC] Minor renaming and typo fixes (NFC)

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

In D110260#3018412 , @xgupta wrote:

> Yeah we need testcases, now it says
>
> JIT session error: Symbols not found: [ return1 ]
> JIT session error: Failed to materialize symbols: { (main, { foo_body }) }
> Unable to lazily compile function. Exiting.

Interesting, it does work for me now on macOS with both, shared libs on and 
off. Can you please check if you get something like this as well:

  ➜ nm bin/LLJITWithExecutorProcessControl | grep return1
  00014c10 T _return1


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110260

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

[PATCH] D110260: [ORC] Minor renaming and typo fixes (NFC)

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

Thanks @xgupta for your note! The parameter passed to 
`EPCIndirectionUtils::Create()` in the example was referencing a moved-from 
value. This caused the segfault. Unfortunately, the examples don't have good 
test coverage so far.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110260

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

[PATCH] D110260: [ORC] Minor renaming and typo fixes (NFC)

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz created this revision.
sgraenitz added a reviewer: lhames.
Herald added subscribers: hiraditya, mgorny.
sgraenitz requested review of this revision.
Herald added projects: clang, LLVM.
Herald added a subscriber: cfe-commits.

One typo, one unsused include and some leftovers from the TargetProcessControl 
-> ExecutorProcessControl renaming


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D110260

Files:
  clang/docs/ClangFormattedStatus.rst
  llvm/examples/OrcV2Examples/CMakeLists.txt
  llvm/examples/OrcV2Examples/LLJITWithExecutorProcessControl/CMakeLists.txt
  
llvm/examples/OrcV2Examples/LLJITWithExecutorProcessControl/LLJITWithExecutorProcessControl.cpp
  llvm/examples/OrcV2Examples/LLJITWithTargetProcessControl/CMakeLists.txt
  
llvm/examples/OrcV2Examples/LLJITWithTargetProcessControl/LLJITWithTargetProcessControl.cpp
  llvm/include/llvm/ExecutionEngine/Orc/SimpleRemoteEPC.h
  llvm/lib/ExecutionEngine/Orc/Shared/SimpleRemoteEPCUtils.cpp

Index: llvm/lib/ExecutionEngine/Orc/Shared/SimpleRemoteEPCUtils.cpp
===
--- llvm/lib/ExecutionEngine/Orc/Shared/SimpleRemoteEPCUtils.cpp
+++ llvm/lib/ExecutionEngine/Orc/Shared/SimpleRemoteEPCUtils.cpp
@@ -223,7 +223,7 @@
 
 if (MsgSize < FDMsgHeader::Size) {
   Err = joinErrors(std::move(Err),
-   make_error("Mesasge size too small",
+   make_error("Message size too small",
inconvertibleErrorCode()));
   break;
 }
Index: llvm/include/llvm/ExecutionEngine/Orc/SimpleRemoteEPC.h
===
--- llvm/include/llvm/ExecutionEngine/Orc/SimpleRemoteEPC.h
+++ llvm/include/llvm/ExecutionEngine/Orc/SimpleRemoteEPC.h
@@ -15,7 +15,6 @@
 
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/FunctionExtras.h"
-#include "llvm/ExecutionEngine/Orc/EPCGenericJITLinkMemoryManager.h"
 #include "llvm/ExecutionEngine/Orc/EPCGenericMemoryAccess.h"
 #include "llvm/ExecutionEngine/Orc/ExecutorProcessControl.h"
 #include "llvm/ExecutionEngine/Orc/Shared/SimpleRemoteEPCUtils.h"
Index: llvm/examples/OrcV2Examples/LLJITWithTargetProcessControl/LLJITWithTargetProcessControl.cpp
===
--- /dev/null
+++ llvm/examples/OrcV2Examples/LLJITWithTargetProcessControl/LLJITWithTargetProcessControl.cpp
@@ -1,197 +0,0 @@
-//===- LLJITWithExecutorProcessControl.cpp - LLJIT example with EPC utils -===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===--===//
-//
-// In this example we will use the lazy re-exports utility to lazily compile
-// IR modules. We will do this in seven steps:
-//
-// 1. Create an LLJIT instance.
-// 2. Install a transform so that we can see what is being compiled.
-// 3. Create an indirect stubs manager and lazy call-through manager.
-// 4. Add two modules that will be conditionally compiled, plus a main module.
-// 5. Add lazy-rexports of the symbols in the conditionally compiled modules.
-// 6. Dump the ExecutionSession state to see the symbol table prior to
-//executing any code.
-// 7. Verify that only modules containing executed code are compiled.
-//
-//===--===//
-
-#include "llvm/ADT/StringMap.h"
-#include "llvm/ExecutionEngine/JITLink/JITLinkMemoryManager.h"
-#include "llvm/ExecutionEngine/Orc/EPCDynamicLibrarySearchGenerator.h"
-#include "llvm/ExecutionEngine/Orc/EPCIndirectionUtils.h"
-#include "llvm/ExecutionEngine/Orc/ExecutorProcessControl.h"
-#include "llvm/ExecutionEngine/Orc/LLJIT.h"
-#include "llvm/ExecutionEngine/Orc/ObjectLinkingLayer.h"
-#include "llvm/ExecutionEngine/Orc/OrcABISupport.h"
-#include "llvm/Support/InitLLVM.h"
-#include "llvm/Support/TargetSelect.h"
-#include "llvm/Support/raw_ostream.h"
-
-#include "../ExampleModules.h"
-
-#include 
-
-using namespace llvm;
-using namespace llvm::orc;
-
-ExitOnError ExitOnErr;
-
-// Example IR modules.
-//
-// Note that in the conditionally compiled modules, FooMod and BarMod, functions
-// have been given an _body suffix. This is to ensure that their names do not
-// clash with their lazy-reexports.
-// For clients who do not wish to rename function bodies (e.g. because they want
-// to re-use cached objects between static and JIT compiles) techniques exist to
-// avoid renaming. See the lazy-reexports section of the ORCv2 design doc.
-
-const llvm::StringRef FooMod =
-R"(
-  declare i32 @return1()
-
-  define i32 @foo_body() {
-  entry:
-%0 = call i32 @return1()
-ret i32 %0
-  }
-)";
-
-const llvm::StringRef BarMod =
-R"(
-  declare i32 @return2()
-
-  define i32 @bar_body() {
-  entry:

[PATCH] D104898: [clang-repl] Allow passing in code as positional arguments.

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz accepted this revision.
sgraenitz added a comment.
This revision is now accepted and ready to land.

LGTM




Comment at: clang/test/Interpreter/execute.cpp:1
 // RUN: cat %s | clang-repl | FileCheck %s
+// RUN: clang-repl "int i = 10;" 'extern "C" int printf(const char*,...);' \

Maybe this could move down to line 8, so it's closer to the code that it works 
with?



Comment at: clang/tools/clang-repl/ClangRepl.cpp:90
 
+  if (!OptInputs.size()) {
+llvm::LineEditor LE("clang-repl");

`OptInputs.empty()` ?


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

https://reviews.llvm.org/D104898

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

[PATCH] D104918: [clang-repl] Implement partial translation units and error recovery.

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

A few minor notes




Comment at: clang/include/clang/Interpreter/Interpreter.h:63
+if (auto Err = ErrOrPTU.takeError())
   return Err;
+if (ErrOrPTU->TheModule)

`ErrorOr` has different semantics and is still in use, so the name could be 
confusing. Idiomatic usage for `Expected` would rather be like:
```
auto PTU = Parse(Code);
if (!PTU)
  return PTU.takeError();
```

The patch didn't introduce it, but it might be a good chance for improvement.



Comment at: clang/lib/Interpreter/IncrementalParser.cpp:178
+TranslationUnitDecl *PreviousTU = MostRecentTU->getPreviousDecl();
+assert(PreviousTU);
+TranslationUnitDecl *FirstTU = MostRecentTU->getFirstDecl();

Where does it point, if the very first TU fails? Maybe worth noting in the 
assert and/or adding a test case?



Comment at: clang/lib/Interpreter/IncrementalParser.cpp:263
+  if (auto Err = ErrOrPTU.takeError())
 return std::move(Err);
 

```
auto PTU = ParseOrWrapTopLevelDecl();
if (!PTU)
  return PTU.takeError();
```


Repository:
  rC Clang

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

https://reviews.llvm.org/D104918

___
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

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz accepted this revision.
sgraenitz added a comment.

Thanks. From my side this looks good now.


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

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

I think this makes good progress. I found two details in the test code that 
need attention. The stdout issue might be done now or in a follow-up patch some 
time soon. Otherwise, this seems to get ready to land.

@teemperor What about your notes. Are there any open issues remaining?




Comment at: clang/lib/Interpreter/Interpreter.cpp:93
+   "Initialization failed. "
+   "Unable to create diagnostics engine");
+

It looks like `clang-repl` always dumps errors to stdout currently. This is 
fine for the interactive use case, but it will be impractical for input/output 
tests. As a result unit tests e.g. dump:
```
Note: Google Test filter = InterpreterTest.Errors
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from InterpreterTest
[ RUN  ] InterpreterTest.Errors
In file included from :0:
input_line_0:1:1: error: unknown type name 'intentional_error'
intentional_error v1 = 42; 
^
[   OK ] InterpreterTest.Errors (9024 ms)
[--] 1 test from InterpreterTest (9024 ms total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (9025 ms total)
[  PASSED  ] 1 test.
```

It would be useful to have an option for streaming diagnostics to an in-memory 
buffer (and maybe append them to returned llvm::Error instances in the future). 
Instead of `createDiagnostics()` you could pass a TextDiagnosticPrinter via 
`setDiagnostics()` here to accomplish it.

Not insisting on having it in this review, but it would be a good follow-up 
task at least.



Comment at: clang/test/Interpreter/execute.c:9
+
+struct S { float f = 1.0; S *m = nullptr;} s;
+auto r2 = printf("S[f=%f, m=%p]\n", s.f, s.m);

*nit* this should be a cpp file right? Otherwise, you should write `struct S *m 
= nullptr;` here. Also, C doesn't understand the `extern "C"` above :)



Comment at: clang/test/Interpreter/execute.c:11
+auto r2 = printf("S[f=%f, m=%p]\n", s.f, s.m);
+// CHECK-NEXT: S[f=1.00, m=(nil)]
+

The `%p` format placeholder in printf is implementation-defined, so the output 
here varies. Maybe you can do something like this instead:

```
auto r2 = printf("S[f=%f, m=0x%llx]\n", s.f, (unsigned long long)s.m);
// CHECK-NEXT: S[f=1.00, m=(0x0)]
```

Or reinterpret_cast(s.m) if you go the C++ way.



Comment at: clang/test/lit.cfg.py:105
+config.available_features.add('host-supports-jit')
+
 if config.clang_staticanalyzer:

I couldn't test this on a host that doesn't support JIT, but it looks like a 
nice "duck typing" way of testing for the feature.


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

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a subscriber: anarazel.
sgraenitz added a comment.

Hi Vassil, thanks for upstreaming this! I think it goes into a good direction.

The last time I looked at the Cling sources downstream, it was based on LLVM 
release 5.0. The IncrementalJIT class was based on what we call OrcV1 today. 
OrcV1 is long deprecated and even though it's still in tree today, it will very 
likely be removed in the upcoming release cycle. So I guess, one of the 
challenges will be porting the Cling internals to OrcV2 -- a lot has changed, 
mostly to the better :) Not all of this is relevant for this patch, but maybe 
it's worth mentioning for upcoming additions.

OrcV2 works with Dylibs, basically symbol namespaces. When you add a module to 
a Dylib, all its symbols will be added. Respectively, if you want to remove 
something from a Dylib, you have to remove the symbols (for fine-tuning it you 
can reach for a Dylib's ResourceTracker). Symbols won't be materialized until 
you look them up. I guess for incremental compilation you would keep on adding 
symbols, one increment at a time.

  int var1 = 42; int f() { return var1; }
  int var2 = f();

Let's say these are two inputs. The first line only adds definitions for 
symbols `var1` and `f()` but won't materialize anything. The second line would 
have to lookup `f()`, execute it and emit a new definition for `var2`. I never 
got into Cling deep enough to find out how it works, but I assume it's 
high-level enough that it won't require large changes. One thing I'd recommend 
to double-check: if there is a third line that adds a static constructor, will 
LLJIT only run this one or will it run all previous static ctors again when 
calling `initialize()`? I assume the former but I wouldn't bet on it.

Another aspect is that downstream Cling is based on RuntimeDyld for linking 
Orc's output object files. I remember RemovableObjectLinkingLayer adding some 
object file removal code. Upstream OrcV2 grew it's own linker in the meantime. 
It's called JITLink and gets pulled into LLJIT via `ObjectLinkingLayer`. 
RuntimeDyld-based linking is still supported with the 
`RTDyldObjectLinkingLayer`. JITLink is not complete for all platforms yet. 
Thus, LLJITBuilder defaults to JITLink on macOS and RuntimeDyld otherwise. 
Chances are that JITLink gets good enough for ELF to enable it by default on 
Linux (at least x86-64). I guess that's your platform of concern? The related 
question is whether you are aiming for JITLink straight away or staying with 
RuntimeDyld for now.

For the moment, I added a few pointers inline. Some are referring to my general 
comments above.




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

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.



Comment at: clang/lib/Interpreter/IncrementalExecutor.h:36
+  llvm::Error addModule(std::unique_ptr M);
+  llvm::Error runCtors() const;
+};

v.g.vassilev wrote:
> teemperor wrote:
> > Should we maybe merge `runCtors` and `addModule`? Not sure if there is a 
> > use case for adding a Module but not 

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

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

@v.g.vassilev Great to see this getting upstreamed! @teemperor Thanks for 
adding, I will take a look in the next days.


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] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added inline comments.



Comment at: lldb/source/Symbol/ClangASTImporter.cpp:68
+  return *ret_or_error;
+} else {
+  Log *log =

aprantl wrote:
> The `else` is redundant.
Here it's necessary for the scope of `ret_or_error`. That's a bit unfortunate. 
Maybe invert the condition?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61438



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

[PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added inline comments.



Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp:1980
+  }
+  return *type_or_error;
 }

>>! In D61438#1490102, @jingham wrote:
> [...] include the contents of that error n the log message?
e.g:
```
if (auto type_owner = merger.ImporterForOrigin(from_context).Import(type)) {
  return *type_owner;
} else {
  Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS);
  LLDB_LOG_ERROR(log, type_owner.takeError(), "Couldn't import type: {0}")
  return QualType();
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61438



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

[PATCH] D57521: [CMake] External compiler-rt-configure requires LLVMTestingSupport when including tests

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

In D57521#1379837 , @beanz wrote:

> @phosek, does LLVM’s runtime directory do this correctly?
>
> At some point we should try and deprecate and remove this option in favor of 
> the LLVM directory because the LLVM approach is much more complete and robust.


My 2¢: ideally compiler-rt used imported targets. Please see my note added in 
D55891 .


Repository:
  rC Clang

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

https://reviews.llvm.org/D57521



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

[PATCH] D55891: [compiler-rt] [xray] [tests] Detect and handle missing LLVMTestingSupport gracefully

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

In case compiler-rt abandons `llvm-config` in favor of `find_package(LLVM)` one 
day, we could drop the `COMPILER_RT_HAS_LLVMTESTINGSUPPORT` logic here and use 
imported target properties instead. It seems a cleaner solution to me and 
avoids issues like D57521 .


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55891



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

[PATCH] D57521: [CMake] External compiler-rt-configure requires LLVMTestingSupport when including tests

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

FYI: Seems to be happening since D55891 


Repository:
  rC Clang

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

https://reviews.llvm.org/D57521



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

[PATCH] D57521: [CMake] External compiler-rt-configure requires LLVMTestingSupport when including tests

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz updated this revision to Diff 184531.
sgraenitz added a comment.

Polishing


Repository:
  rC Clang

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

https://reviews.llvm.org/D57521

Files:
  runtime/CMakeLists.txt


Index: runtime/CMakeLists.txt
===
--- runtime/CMakeLists.txt
+++ runtime/CMakeLists.txt
@@ -58,12 +58,16 @@
 endif()
   endforeach()
 
+  set(compiler_rt_configure_deps)
   if(TARGET cxx-headers)
-set(COMPILER_RT_LIBCXX_DEPENDENCY "cxx-headers")
+list(APPEND compiler_rt_configure_deps "cxx-headers")
+  endif()
+  if(LLVM_INCLUDE_TESTS)
+list(APPEND compiler_rt_configure_deps LLVMTestingSupport)
   endif()
 
   ExternalProject_Add(compiler-rt
-DEPENDS llvm-config clang ${COMPILER_RT_LIBCXX_DEPENDENCY}
+DEPENDS llvm-config clang ${compiler_rt_configure_deps}
 PREFIX ${COMPILER_RT_PREFIX}
 SOURCE_DIR ${COMPILER_RT_SRC_ROOT}
 STAMP_DIR ${STAMP_DIR}


Index: runtime/CMakeLists.txt
===
--- runtime/CMakeLists.txt
+++ runtime/CMakeLists.txt
@@ -58,12 +58,16 @@
 endif()
   endforeach()
 
+  set(compiler_rt_configure_deps)
   if(TARGET cxx-headers)
-set(COMPILER_RT_LIBCXX_DEPENDENCY "cxx-headers")
+list(APPEND compiler_rt_configure_deps "cxx-headers")
+  endif()
+  if(LLVM_INCLUDE_TESTS)
+list(APPEND compiler_rt_configure_deps LLVMTestingSupport)
   endif()
 
   ExternalProject_Add(compiler-rt
-DEPENDS llvm-config clang ${COMPILER_RT_LIBCXX_DEPENDENCY}
+DEPENDS llvm-config clang ${compiler_rt_configure_deps}
 PREFIX ${COMPILER_RT_PREFIX}
 SOURCE_DIR ${COMPILER_RT_SRC_ROOT}
 STAMP_DIR ${STAMP_DIR}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57521: [CMake] External compiler-rt-configure requires LLVMTestingSupport when including tests

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz created this revision.
sgraenitz added reviewers: ab, beanz.
Herald added subscribers: erik.pilkington, mgorny, dberris.

Apparently `LLVMTestingSupport` must be built before `llvm-config` can be asked 
for it. Symptom with `LLVM_INCLUDE_TESTS=ON` is:

  $ ./path/to/llvm-build/bin/llvm-config --ldflags --libs testingsupport
  -L/path/to/llvm-build/lib -Wl,-search_paths_first 
-Wl,-headerpad_max_install_names
  llvm-config: error: component libraries and shared library
  
  llvm-config: error: missing: /path/to/llvm-build/lib/libLLVMTestingSupport.a

With `LLVMTestingSupport` as dependency of `compiler-rt-configure` we get the 
expected behavior:

  $ ./path/to/llvm-build/bin/llvm-config --ldflags --libs testingsupport
  -L/path/to/llvm-build/lib -Wl,-search_paths_first 
-Wl,-headerpad_max_install_names
  -lLLVMTestingSupport -lLLVMSupport -lLLVMDemangle


Repository:
  rC Clang

https://reviews.llvm.org/D57521

Files:
  runtime/CMakeLists.txt


Index: runtime/CMakeLists.txt
===
--- runtime/CMakeLists.txt
+++ runtime/CMakeLists.txt
@@ -62,6 +62,10 @@
 set(COMPILER_RT_LIBCXX_DEPENDENCY "cxx-headers")
   endif()
 
+  if(LLVM_INCLUDE_TESTS)
+list(APPEND COMPILER_RT_LIBCXX_DEPENDENCY LLVMTestingSupport)
+  endif()
+
   ExternalProject_Add(compiler-rt
 DEPENDS llvm-config clang ${COMPILER_RT_LIBCXX_DEPENDENCY}
 PREFIX ${COMPILER_RT_PREFIX}


Index: runtime/CMakeLists.txt
===
--- runtime/CMakeLists.txt
+++ runtime/CMakeLists.txt
@@ -62,6 +62,10 @@
 set(COMPILER_RT_LIBCXX_DEPENDENCY "cxx-headers")
   endif()
 
+  if(LLVM_INCLUDE_TESTS)
+list(APPEND COMPILER_RT_LIBCXX_DEPENDENCY LLVMTestingSupport)
+  endif()
+
   ExternalProject_Add(compiler-rt
 DEPENDS llvm-config clang ${COMPILER_RT_LIBCXX_DEPENDENCY}
 PREFIX ${COMPILER_RT_PREFIX}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55128: [CMake] Store path to vendor-specific headers in clang-headers target property

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz created this revision.
sgraenitz added reviewers: aprantl, JDevlieghere, davide, friss, dexonsmith.
Herald added a subscriber: mgorny.

LLDB.framework wants a copy these headers. With this change LLDB can easily 
glob for the list of files:

  get_target_property(clang_include_dir clang-headers RUNTIME_OUTPUT_DIRECTORY)
  file(GLOB_RECURSE clang_vendor_headers RELATIVE ${clang_include_dir} 
"${clang_include_dir}/*")

By default `RUNTIME_OUTPUT_DIRECTORY` is unset for custom targets like 
`clang-headers`.
Not sure if there is a read property that matches the purpose better.

What do you think?


Repository:
  rC Clang

https://reviews.llvm.org/D55128

Files:
  lib/Headers/CMakeLists.txt


Index: lib/Headers/CMakeLists.txt
===
--- lib/Headers/CMakeLists.txt
+++ lib/Headers/CMakeLists.txt
@@ -144,7 +144,7 @@
   list(APPEND out_files ${dst})
 endforeach( f )
 
-add_custom_command(OUTPUT ${output_dir}/arm_neon.h 
+add_custom_command(OUTPUT ${output_dir}/arm_neon.h
   DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/arm_neon.h
   COMMAND ${CMAKE_COMMAND} -E copy_if_different 
${CMAKE_CURRENT_BINARY_DIR}/arm_neon.h ${output_dir}/arm_neon.h
   COMMENT "Copying clang's arm_neon.h...")
@@ -156,7 +156,9 @@
 list(APPEND out_files ${output_dir}/arm_fp16.h)
 
 add_custom_target(clang-headers ALL DEPENDS ${out_files})
-set_target_properties(clang-headers PROPERTIES FOLDER "Misc")
+set_target_properties(clang-headers PROPERTIES
+  FOLDER "Misc"
+  RUNTIME_OUTPUT_DIRECTORY "${output_dir}")
 
 install(
   FILES ${files} ${CMAKE_CURRENT_BINARY_DIR}/arm_neon.h


Index: lib/Headers/CMakeLists.txt
===
--- lib/Headers/CMakeLists.txt
+++ lib/Headers/CMakeLists.txt
@@ -144,7 +144,7 @@
   list(APPEND out_files ${dst})
 endforeach( f )
 
-add_custom_command(OUTPUT ${output_dir}/arm_neon.h 
+add_custom_command(OUTPUT ${output_dir}/arm_neon.h
   DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/arm_neon.h
   COMMAND ${CMAKE_COMMAND} -E copy_if_different ${CMAKE_CURRENT_BINARY_DIR}/arm_neon.h ${output_dir}/arm_neon.h
   COMMENT "Copying clang's arm_neon.h...")
@@ -156,7 +156,9 @@
 list(APPEND out_files ${output_dir}/arm_fp16.h)
 
 add_custom_target(clang-headers ALL DEPENDS ${out_files})
-set_target_properties(clang-headers PROPERTIES FOLDER "Misc")
+set_target_properties(clang-headers PROPERTIES
+  FOLDER "Misc"
+  RUNTIME_OUTPUT_DIRECTORY "${output_dir}")
 
 install(
   FILES ${files} ${CMAKE_CURRENT_BINARY_DIR}/arm_neon.h
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits