[PATCH] D148851: Disable llvm-symbolizer on some of the driver tests that are timing out

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

@ahatanak Thanks for working on this. I think that annotating these particular 
tests so that's clear that they are supposed to crash (and therefore 
symbolication is of no use) is a good change. If we want to make a more global 
change for tests I think that should be handled separately.

However, if `not --crash` does the job of disabling symbolication I think we 
should use that because it also documents that we expect the binary being 
executed to crash. If you make that change then I'm happy to approve.




Comment at: clang/test/Driver/crash-diagnostics-dir-3.c:4
 // RUN: rm -rf %t
 // RUN: not env CLANG_CRASH_DIAGNOSTICS_DIR=%t %clang -c %s -o - 2>&1 | 
FileCheck %s
 #pragma clang __debug parser_crash

Should we use `not --crash` instead? It's more explicit that we are expecting a 
crash and IIRC should disable symbolization as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148851

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

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Dan Liew via Phabricator via cfe-commits]

delcypher added inline comments.



Comment at: 
clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected:12
+// CHECK-NEXT:[[TMP1:%.*]] = load i32, ptr [[X]], align 4
+// CHECK-NEXT:ret i32 [[TMP1]]
+//

@hnrklssn I just noticed we don't have a `CHECK` for what `META2` actually 
refers to. Should we?

Not something that has to be fixed in this patch, more just an observation.



Comment at: llvm/utils/UpdateTestChecks/common.py:703
   def get_ir_prefix_from_ir_value_match(self, match):
-return self.ir_prefix, self.check_prefix
+return re.search(self.ir_prefix, match[0])[0], self.check_prefix
 

Is this call to `re.search(...)` guaranteed to always succeed?



Comment at: llvm/utils/UpdateTestChecks/common.py:779
 NamelessValue(r'ACC_GRP', '!' , r'!llvm.access.group ' , r'![0-9]+'
 , None ) ,
+NamelessValue(r'META'   , '!' , r'![a-z.]+ '   , r'![0-9]+'
 , None ) ,
 ]

There may be some value in having `!annotations` matched explicitly as well as 
there being a fallback. In the current patch it looks like:

* `metadata` is assigned variables with the `META` prefix on `CHECK` lines
* `!annotation` is assigned variables with the `META` prefix on `CHECK` lines
* Anything else that matches `r'![a-z.]+ ' `  is assigned variables with the 
`META` prefix on `CHECK` lines

When looking a large test having everything lumped into `META` variables could 
make the generated tests a little harder to read.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148216

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

[PATCH] D141550: [CompilerRT] Remove ubsan static runtime on Apple

[Dan Liew via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141550

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

[PATCH] D141550: [CompilerRT] Remove ubsan static runtime on Apple

[Dan Liew via Phabricator via cfe-commits]

delcypher added inline comments.



Comment at: compiler-rt/lib/ubsan/CMakeLists.txt:118
+if (NOT APPLE)
+  add_compiler_rt_runtime(clang_rt.ubsan
+STATIC

usama54321 wrote:
> delcypher wrote:
> > I think you may have accidentally added tabs here when re-indenting.
> I double checked and these are spaces :/
Oh my bad. Maybe the `>>` symbol in phrabricator means indent rather than a 
particular space/tab choice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141550

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

[PATCH] D141550: [CompilerRT] Remove ubsan static runtime on Apple

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

Overall approach LGTM. I just have some very minor nits.




Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:219
+def err_drv_unsupported_static_ubsan_darwin : Error<
+  "Static UBSan runtime is not supported on darwin">;
 def err_drv_duplicate_config : Error<

Nit: Driver messages start with lowercase.

Also please check if "UBSan" is spelt differently in existing driver messages. 
It might actually be written more explicitly like "undefined behavior 
sanitizer".



Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:1441
 Sanitize.requiresMinimalRuntime() ? "ubsan_minimal"
-  : "ubsan",
-Sanitize.needsSharedRt());
+  : "ubsan");
   if (Sanitize.needsTsanRt())

Maybe add an assert? As the code is constructed right now it should never fail 
but in the future someone could change the code and break the assumption.

```lang=c++
  if (Sanitize.needsUbsanRt()) {
assert(Sanitize.needsSharedRt() && "Static sanitizer runtimes not 
supported");
AddLinkSanitizerLibArgs(Args, CmdArgs,
Sanitize.requiresMinimalRuntime() ? "ubsan_minimal"
  : "ubsan");
}

```



Comment at: compiler-rt/lib/ubsan/CMakeLists.txt:118
+if (NOT APPLE)
+  add_compiler_rt_runtime(clang_rt.ubsan
+STATIC

I think you may have accidentally added tabs here when re-indenting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141550

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

[PATCH] D137024: [compiler-rt] Switch from llvm-config to find_package(LLVM)

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

In D137024#3931488 , @mgorny wrote:

> Well, I'm certainly not opposed to making all the paths configurable. 
> However, I'm not sure if having CMake file accessible one way or another 
> wouldn't eventually be a necessity. For one thing, I would like to move more 
> common code from standalone build codepaths of individual projects into a 
> dedicated CMake file in LLVM (and while I don't want to speak of others, it 
> seems that there are at least few other people who would like to see 
> something similar done). I suppose this wouldn't be an outright blocker if 
> `llvm/cmake/Modules` directory were present but I'm not 100% sure.

@mgorny In our use case the full llvm-project source tree is available. In fact 
`CompilerRTMockLLVMCMakeConfig` which we use currently relies on this. So I am 
100% fine with CMake code being refactored out of `compiler-rt` to elsewhere in 
the source tree (if it makes sense). What we don't have available is the "build 
tree/install tree" `LLVMConfig.cmake` file (and friends) because we don't ship 
it in our toolchain.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137024

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

[PATCH] D137024: [compiler-rt] Switch from llvm-config to find_package(LLVM)

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

In D137024#3931167 , @mgorny wrote:

> In D137024#3931126 , @thetruestblue 
> wrote:
>
>> This patch removes the logic that sets the binary tools dir using 
>> llvm-config. We don't have llvmConfig.cmake in our toolchain or our build 
>> tree and relied on llvm-config to 
>> set(LLVM_TOOLS_BINARY_DIR "${LLVM_TOOLS_BINARY_DIR}"directory.
>
> While I sympathize with you, I don't think this is valid reason to maintain 
> full compatibility with `llvm-config`. Unless i'm mistaken, its use for 
> building other LLVM projects standalone was deprecated for a few years 
> already, and compiler-rt was the last project to carry the compatibility 
> code. Furthermore, skipping cmake files from LLVM seems wrong.

@mgorny To add a bit of context to this discussion. We are not asking for 
compatibility with the `llvm-config` binary. I'm fine if that goes way, which 
it already has.

What we want to do is make sure is that our use case still works. I know that 
"skipping cmake files from LLVM seems wrong" but there is a very good reason 
for this. We have a testing configuration where we consume pre-built toolchains 
and run the compiler-rt tests against these. We use the standalone CMake 
configure of compiler-rt to generate the test suites, nothing actually gets 
built.
In this scenario the toolchain does not include LLVM CMake files because we do 
not ship them to customers. Making it a hard requirement to have those CMake 
files would prevent us from doing that kind of testing.

I suggest that @thetruestblue and I draft a patch so our intentions our clearer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137024

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

[PATCH] D137714: Do not merge traps in functions annotated optnone

[Dan Liew via Phabricator via cfe-commits]

delcypher accepted this revision.
delcypher added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137714

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

[PATCH] D137714: Do not merge traps in functions annotated optnone

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

Other than minor issue in the test this LGTM




Comment at: clang/test/CodeGen/ubsan-trap-debugloc.c:11
+void bar(volatile int a) __attribute__((optnone)) {
+  // CHECK: call void @llvm.ubsantrap(i8 0){{.*}} !dbg [[LOC2:![0-9]+]]
+  // CHECK: call void @llvm.ubsantrap(i8 0){{.*}} !dbg [[LOC3:![0-9]+]]

Could you add `CHECK-LABEL: @foo` and `CHECK-LABEL: @bar` to this test? This 
helps make it explicit which functions in the IR we are trying to match.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137714

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

[PATCH] D125919: Drop qualifiers from return types in C (DR423)

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

@rjmccall

>> Sorry if these are silly questions and if I've misunderstood something, I 
>> saw n1863 say "functions return unqualified types" and I was very surprised.
>
> Just to be clear, you understand that this is only about top-level qualifiers 
> on the return type, right?  `const void *foo();` is still meaningful, it's 
> just that `const void * const foo();` isn't.

Oh that wasn't obvious to me at all. I had assumed "unqualified types" means 
remove qualifiers from everywhere in the type, not just the top level. 
Presumably something was meant to imply that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125919

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

[PATCH] D125919: Drop qualifiers from return types in C (DR423)

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

@aaron.ballman

>> Sorry if these are silly questions and if I've misunderstood something, I 
>> saw n1863 say "functions return unqualified types" and I was very surprised.
>
> These are not at all silly questions, so thank you for asking them!

Thanks for taking the time to answer my questions. Your answers make sense to 
me apart from the `derived-declarator-type-list`. I'm not sure what that is, 
the copy of the standard I found doesn't seem seem to define what it is. I see 
other people have complained about this in the past 
(https://stackoverflow.com/questions/13779273/in-the-standard-what-is-derived-declarator-type)
 so I'll guess I'll stare at that for a while until it makes sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125919

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

[PATCH] D125919: Drop qualifiers from return types in C (DR423)

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

@aaron.ballman Hey I just saw this change and had questions about it. For 
others looking I think the resolution to DR423 is in 
https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1863.pdf, I found 
https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2148.htm#dr_423 hard to parse.

1. It looks like we're completely dropping qualifiers on the return types of 
function in C. If that's the case, what's even the point of having qualifiers 
on return types of functions?
2. Reading DR423 I see the problem but I don't understand why the desire to 
make `_Generic` work better in the presence of qualifiers means that qualifiers 
on function return types have to be dropped in all contexts. Couldn't this just 
be a thing that is done inside `_Generic` and no where else?

Sorry if these are silly questions and if I've misunderstood something, I saw 
n1863 say "functions return unqualified types" and I was very surprised.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125919

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

[PATCH] D125396: [clang] Fix KEYALL

[Dan Liew via Phabricator via cfe-commits]

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

@yaxunl Thanks for addressing my feedback so quickly. I think the commit 
message should also mention that `KEYCUDA` is now included in `KEYALL`. Other 
than that LGTM.


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

https://reviews.llvm.org/D125396

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

[PATCH] D124866: [CUDA][HIP] support __noinline__ as keyword

[Dan Liew via Phabricator via cfe-commits]

delcypher added inline comments.



Comment at: clang/lib/Basic/IdentifierTable.cpp:111
 KEYSYCL   = 0x100,
+KEYCUDA   = 0x200,
 KEYALLCXX = KEYCXX | KEYCXX11 | KEYCXX20,

yaxunl wrote:
> yaxunl wrote:
> > delcypher wrote:
> > > yaxunl wrote:
> > > > delcypher wrote:
> > > > > @yaxunl Is it intentional that you didn't update `KEYALL` here? That 
> > > > > means `KEYALL` doesn't include the bit for `KEYCUDA`.
> > > > > 
> > > > > If that was your intention then this will break if someone adds a new 
> > > > > key. E.g.
> > > > > 
> > > > > ```
> > > > > KEYCUDA = 0x200,
> > > > > KEYSOMENEWTHING = 0x400,
> > > > > // ...
> > > > > // KEYALL now includes `KEYCUDA`, whereas it didn't before.
> > > > > // KEYALL includes KEYSOMENEWTHING 
> > > > > KEYALL = (0x7ff & ~KEYNOMS18 &
> > > > >   ~KEYNOOPENCL) // KEYNOMS18 and KEYNOOPENCL are used to 
> > > > > exclude.
> > > > > ...
> > > > > ```
> > > > > 
> > > > > 
> > > > > 1. Updating the `0x1ff` constant to `0x3ff` so that `KEYALL` 
> > > > > includes `KEYCUDA`
> > > > > 2. If your intention **is** to not have `KEYCUDA`  set in `KEYALL` 
> > > > > then amend `KEYALL` to be.
> > > > > 
> > > > > ```
> > > > > KEYALL = (0x7ff & ~KEYNOMS18 &
> > > > >   ~KEYNOOPENCL & ~KEYCUDA ) // KEYNOMS18 and KEYNOOPENCL 
> > > > > are used to exclude.
> > > > > // KEYCUDA is not included in KEYALL
> > > > > ```
> > > > My intention is not to include KEYCUDA in KEYALL.
> > > > 
> > > > Should I change KEYALL to
> > > > 
> > > > 
> > > > ```
> > > > KEYALL = (0x3ff & ~KEYNOMS18 &
> > > >   ~KEYNOOPENCL & ~KEYCUDA ) // KEYNOMS18 and KEYNOOPENCL 
> > > > are used to exclude.
> > > > // KEYCUDA is not included in KEYALL
> > > > ```
> > > > 
> > > > instead of 
> > > > 
> > > > 
> > > > ```
> > > > KEYALL = (0x7ff & ~KEYNOMS18 &
> > > >   ~KEYNOOPENCL & ~KEYCUDA ) // KEYNOMS18 and KEYNOOPENCL 
> > > > are used to exclude.
> > > > // KEYCUDA is not included in KEYALL
> > > > ```
> > > > 
> > > > since the current maximum mask is 0x3ff instead of 0x7ff
> > > Oops, you're right it would be `0x3ff`. I wonder though if we should 
> > > clean this up so we don't need to manually update the bit mask every 
> > > time... what if it was written like this?
> > > 
> > > ```lang=c++
> > >  enum {
> > > KEYC99= 0x1,
> > > KEYCXX= 0x2,
> > > KEYCXX11  = 0x4,
> > > 
> > > KEYSYCL   = 0x100,
> > > KEYCUDA   = 0x200,
> > > KEYMAX = KEYCUDA, // Must be set to the largest KEY enum value
> > > KEYALLCXX = KEYCXX | KEYCXX11 | KEYCXX20,
> > > 
> > > // KEYNOMS18 and KEYNOOPENCL are used to exclude.
> > > // KEYCUDA is not included in KEYALL because 
> > > KEYALL = (((KEYMAX & (KEYMAX-1)) & ~KEYNOMS18 & ~KEYNOOPENCL & 
> > > ~KEYCUDA)
> > > };
> > > ```
> > On second thought, KEYALL does not need to exclude KEYCUDA.
> > 
> > However, it would be good to set KEYALL in a generic approach. I will open 
> > a separate review.
> opened https://reviews.llvm.org/D125396 to fix KEYALL
Oops that should say

```
KEYALL = (((KEYMAX | (KEYMAX-1)) & ~KEYNOMS18 & ~KEYNOOPENCL & ~KEYCUDA)
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124866

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

[PATCH] D124866: [CUDA][HIP] support __noinline__ as keyword

[Dan Liew via Phabricator via cfe-commits]

delcypher added inline comments.



Comment at: clang/lib/Basic/IdentifierTable.cpp:111
 KEYSYCL   = 0x100,
+KEYCUDA   = 0x200,
 KEYALLCXX = KEYCXX | KEYCXX11 | KEYCXX20,

yaxunl wrote:
> delcypher wrote:
> > @yaxunl Is it intentional that you didn't update `KEYALL` here? That means 
> > `KEYALL` doesn't include the bit for `KEYCUDA`.
> > 
> > If that was your intention then this will break if someone adds a new key. 
> > E.g.
> > 
> > ```
> > KEYCUDA = 0x200,
> > KEYSOMENEWTHING = 0x400,
> > // ...
> > // KEYALL now includes `KEYCUDA`, whereas it didn't before.
> > // KEYALL includes KEYSOMENEWTHING 
> > KEYALL = (0x7ff & ~KEYNOMS18 &
> >   ~KEYNOOPENCL) // KEYNOMS18 and KEYNOOPENCL are used to 
> > exclude.
> > ...
> > ```
> > 
> > 
> > 1. Updating the `0x1ff` constant to `0x3ff` so that `KEYALL` 
> > includes `KEYCUDA`
> > 2. If your intention **is** to not have `KEYCUDA`  set in `KEYALL` then 
> > amend `KEYALL` to be.
> > 
> > ```
> > KEYALL = (0x7ff & ~KEYNOMS18 &
> >   ~KEYNOOPENCL & ~KEYCUDA ) // KEYNOMS18 and KEYNOOPENCL are 
> > used to exclude.
> > // KEYCUDA is not included in KEYALL
> > ```
> My intention is not to include KEYCUDA in KEYALL.
> 
> Should I change KEYALL to
> 
> 
> ```
> KEYALL = (0x3ff & ~KEYNOMS18 &
>   ~KEYNOOPENCL & ~KEYCUDA ) // KEYNOMS18 and KEYNOOPENCL are used 
> to exclude.
> // KEYCUDA is not included in KEYALL
> ```
> 
> instead of 
> 
> 
> ```
> KEYALL = (0x7ff & ~KEYNOMS18 &
>   ~KEYNOOPENCL & ~KEYCUDA ) // KEYNOMS18 and KEYNOOPENCL are used 
> to exclude.
> // KEYCUDA is not included in KEYALL
> ```
> 
> since the current maximum mask is 0x3ff instead of 0x7ff
Oops, you're right it would be `0x3ff`. I wonder though if we should clean 
this up so we don't need to manually update the bit mask every time... what if 
it was written like this?

```lang=c++
 enum {
KEYC99= 0x1,
KEYCXX= 0x2,
KEYCXX11  = 0x4,

KEYSYCL   = 0x100,
KEYCUDA   = 0x200,
KEYMAX = KEYCUDA, // Must be set to the largest KEY enum value
KEYALLCXX = KEYCXX | KEYCXX11 | KEYCXX20,

// KEYNOMS18 and KEYNOOPENCL are used to exclude.
// KEYCUDA is not included in KEYALL because 
KEYALL = (((KEYMAX & (KEYMAX-1)) & ~KEYNOMS18 & ~KEYNOOPENCL & ~KEYCUDA)
};
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124866

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

[PATCH] D124866: [CUDA][HIP] support __noinline__ as keyword

[Dan Liew via Phabricator via cfe-commits]

delcypher added inline comments.



Comment at: clang/lib/Basic/IdentifierTable.cpp:111
 KEYSYCL   = 0x100,
+KEYCUDA   = 0x200,
 KEYALLCXX = KEYCXX | KEYCXX11 | KEYCXX20,

@yaxunl Is it intentional that you didn't update `KEYALL` here? That means 
`KEYALL` doesn't include the bit for `KEYCUDA`.

If that was your intention then this will break if someone adds a new key. E.g.

```
KEYCUDA = 0x200,
KEYSOMENEWTHING = 0x400,
// ...
// KEYALL now includes `KEYCUDA`, whereas it didn't before.
// KEYALL includes KEYSOMENEWTHING 
KEYALL = (0x7ff & ~KEYNOMS18 &
  ~KEYNOOPENCL) // KEYNOMS18 and KEYNOOPENCL are used to exclude.
...
```


1. Updating the `0x1ff` constant to `0x3ff` so that `KEYALL` includes 
`KEYCUDA`
2. If your intention **is** to not have `KEYCUDA`  set in `KEYALL` then amend 
`KEYALL` to be.

```
KEYALL = (0x7ff & ~KEYNOMS18 &
  ~KEYNOOPENCL & ~KEYCUDA ) // KEYNOMS18 and KEYNOOPENCL are used 
to exclude.
// KEYCUDA is not included in KEYALL
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124866

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

[PATCH] D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks.

[Dan Liew via Phabricator via cfe-commits]

delcypher added inline comments.



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2443
+  // Handle poisoning the array cookie in asan
+  if (CGM.getLangOpts().Sanitize.has(SanitizerKind::Address) && AS == 0 &&
+  (expr->getOperatorNew()->isReplaceableGlobalAllocationFunction() ||

Why is there a restriction on the address space?



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2478
+  // run-time deal with it: if the shadow is properly poisoned return the
+  // cookie, otherwise return 0 to avoid an infinite loop calling DTORs.
+  // We can't simply ignore this load using nosanitize metadata because

This comment is confusing. What's returning `0`? `__asan_load_cxx_array_cookie` 
doesn't do that and AFAICT neither does this code.



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2479
+  // cookie, otherwise return 0 to avoid an infinite loop calling DTORs.
+  // We can't simply ignore this load using nosanitize metadata because
+  // the metadata may be lost.

I also don't understand what you mean by the comment. Could you elaborate?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125195

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

[PATCH] D124489: Deprecate LLVM_BUILD_EXTERNAL_COMPILER_RT

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

I spoke to a few other engineers in Apple. They are

1. Happy for us to proceed with marking `LLVM_BUILD_EXTERNAL_COMPILER_RT` as 
deprecated.
2. Okay with using `LLVM_BUILD_EXTERNAL_COMPILER_RT` being an error by default 
that can be downgraded to a warning using a CMake cache option.

@beanz If you can adapt the patch to make the warning into an error with a way 
to downgrade then I'll happily approve.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124489

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

[PATCH] D124489: Deprecate LLVM_BUILD_EXTERNAL_COMPILER_RT

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

> Making it an ERROR and providing an option to downgrade it to a WARNING seems 
> reasonable to me. Thoughts?

One nice property this would have is it would (eventually) allow us to collect 
a list of all the places that depend on this old code. It is potentially quite 
disruptive though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124489

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

[PATCH] D124474: Honor COMPILER_RT_INCLUDE_TESTS when using LLVM_BUILD_EXTERNAL_COMPILER_RT=ON

[Dan Liew via Phabricator via cfe-commits]

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

Change LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124474

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

[PATCH] D124474: Honor COMPILER_RT_INCLUDE_TESTS when using LLVM_BUILD_EXTERNAL_COMPILER_RT=ON

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

> I'd definitely prefer moving towards LLVM_ENABLE_RUNTIMES. We already require 
> LLVM_ENABLE_RUNTIMES for libc++, libc++abi and libunwind and I'm going to 
> propose doing the same for compiler-rt as well.

+1 on this. Compiler-RT's CMake code is extremely and unnecessarily complicated 
because we support far too many exotic build configurations.

> I don't want to roadblock or say "no we absolutely can't let this in", 
> _but_... Apple Clang and Swift have long been the only reason we've kept this 
> code alive. I didn't have the time to fix Apple Clang years ago, and nobody 
> has made the effort since.

I'm going to approve because this change seems reasonable. However, I think we 
need to have a serious conversation about removing 
`LLVM_BUILD_EXTERNAL_COMPILER_RT` with an actual deadline so that we can make 
forward progress here. This review is probably the wrong place to have that 
discussion. We probably should have an RFC on the forums with the deprecation 
plan.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124474

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

[PATCH] D124054: [NFC] Avoid unnecessary duplication of code generating diagnostic.

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

@aaron.ballman Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124054

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

[PATCH] D124054: [NFC] Avoid unnecessary duplication of code generating diagnostic.

[Dan Liew via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG3d612a930dce: [NFC] Avoid unnecessary duplication of code 
generating diagnostic. (authored by delcypher).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124054

Files:
  clang/lib/Sema/SemaExpr.cpp


Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -16932,10 +16932,12 @@
   }
 
   PartialDiagnostic FDiag = PDiag(DiagKind);
+  AssignmentAction ActionForDiag = Action;
   if (Action == AA_Passing_CFAudited)
-FDiag << FirstType << SecondType << AA_Passing << 
SrcExpr->getSourceRange();
-  else
-FDiag << FirstType << SecondType << Action << SrcExpr->getSourceRange();
+ActionForDiag = AA_Passing;
+
+  FDiag << FirstType << SecondType << ActionForDiag
+<< SrcExpr->getSourceRange();
 
   if (DiagKind == diag::ext_typecheck_convert_incompatible_pointer_sign ||
   DiagKind == diag::err_typecheck_convert_incompatible_pointer_sign) {


Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -16932,10 +16932,12 @@
   }
 
   PartialDiagnostic FDiag = PDiag(DiagKind);
+  AssignmentAction ActionForDiag = Action;
   if (Action == AA_Passing_CFAudited)
-FDiag << FirstType << SecondType << AA_Passing << SrcExpr->getSourceRange();
-  else
-FDiag << FirstType << SecondType << Action << SrcExpr->getSourceRange();
+ActionForDiag = AA_Passing;
+
+  FDiag << FirstType << SecondType << ActionForDiag
+<< SrcExpr->getSourceRange();
 
   if (DiagKind == diag::ext_typecheck_convert_incompatible_pointer_sign ||
   DiagKind == diag::err_typecheck_convert_incompatible_pointer_sign) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124054: [NFC] Avoid unnecessary duplication of code generating diagnostic.

[Dan Liew via Phabricator via cfe-commits]

delcypher created this revision.
delcypher added reviewers: rapidsna, fcloutier, aaron.ballman.
Herald added a project: All.
delcypher requested review of this revision.
Herald added a project: clang.

The previous code unneccessarily duplicated the creation of a diagnostic
where the only difference was the `AssignmentAction` being passed.

rdar://88664722


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124054

Files:
  clang/lib/Sema/SemaExpr.cpp


Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -16943,10 +16943,12 @@
   }
 
   PartialDiagnostic FDiag = PDiag(DiagKind);
+  AssignmentAction ActionForDiag = Action;
   if (Action == AA_Passing_CFAudited)
-FDiag << FirstType << SecondType << AA_Passing << 
SrcExpr->getSourceRange();
-  else
-FDiag << FirstType << SecondType << Action << SrcExpr->getSourceRange();
+ActionForDiag = AA_Passing;
+
+  FDiag << FirstType << SecondType << ActionForDiag
+<< SrcExpr->getSourceRange();
 
   if (DiagKind == diag::ext_typecheck_convert_incompatible_pointer_sign ||
   DiagKind == diag::err_typecheck_convert_incompatible_pointer_sign) {


Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -16943,10 +16943,12 @@
   }
 
   PartialDiagnostic FDiag = PDiag(DiagKind);
+  AssignmentAction ActionForDiag = Action;
   if (Action == AA_Passing_CFAudited)
-FDiag << FirstType << SecondType << AA_Passing << SrcExpr->getSourceRange();
-  else
-FDiag << FirstType << SecondType << Action << SrcExpr->getSourceRange();
+ActionForDiag = AA_Passing;
+
+  FDiag << FirstType << SecondType << ActionForDiag
+<< SrcExpr->getSourceRange();
 
   if (DiagKind == diag::ext_typecheck_convert_incompatible_pointer_sign ||
   DiagKind == diag::err_typecheck_convert_incompatible_pointer_sign) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.

[Dan Liew via Phabricator via cfe-commits]

delcypher abandoned this revision.
delcypher added inline comments.
Herald added a project: All.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:5529
+def warn_call_function_without_prototype : Warning<
+  "calling function %0 with arguments when function has no prototype">, 
InGroup<
+  DiagGroup<"strict-calls-without-prototype">>, DefaultIgnore;

aaron.ballman wrote:
> delcypher wrote:
> > aaron.ballman wrote:
> > > delcypher wrote:
> > > > aaron.ballman wrote:
> > > > > delcypher wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > This diagnostic doesn't tell me what's wrong with the code (and 
> > > > > > > in fact, there's very possibly nothing wrong with the code 
> > > > > > > whatsoever). Further, why does calling a function *with no 
> > > > > > > arguments* matter when it has no prototype? I would imagine this 
> > > > > > > should flag any call to a function without a prototype given that 
> > > > > > > the function without a prototype may still expect arguments. e.g.,
> > > > > > > ```
> > > > > > > // Header.h
> > > > > > > int f();
> > > > > > > 
> > > > > > > // Source.c
> > > > > > > int f(a) int a; { ... }
> > > > > > > 
> > > > > > > // Caller.c
> > > > > > > #include "Header.h"
> > > > > > > 
> > > > > > > int main() {
> > > > > > >   return f();
> > > > > > > }
> > > > > > > ```
> > > > > > > I think the diagnostic text should be updated to something more 
> > > > > > > like `cannot verify %0 is being called with the correct number or 
> > > > > > > %plural{1:type|:types}1 of arguments because it was declared 
> > > > > > > without a prototype` (or something along those lines that 
> > > > > > > explains what's wrong with the code).
> > > > > > @aaron.ballman  Thanks for the helpful feedback.
> > > > > > 
> > > > > > > This diagnostic doesn't tell me what's wrong with the code (and 
> > > > > > > in fact, there's very possibly nothing wrong with the code 
> > > > > > > whatsoever).
> > > > > > 
> > > > > > That's a fair criticism.  I think the diagnostic message you 
> > > > > > suggest is a lot more helpful so I'll go for something like that.
> > > > > > 
> > > > > > > Further, why does calling a function *with no arguments* matter 
> > > > > > > when it has no prototype?
> > > > > > 
> > > > > > The reason was to avoid the warning being noisy. E.g. I didn't the 
> > > > > > warning to fire in this situation.
> > > > > > 
> > > > > > ```
> > > > > > // Header.h
> > > > > > int f(); // The user forgot to put `void` between parentheses
> > > > > > 
> > > > > > // Source.c
> > > > > > int f(void) { ... }
> > > > > > 
> > > > > > // Caller.c
> > > > > > #include "Header.h"
> > > > > > 
> > > > > > int main() {
> > > > > >   return f();
> > > > > > }
> > > > > > ```
> > > > > > 
> > > > > > Forgetting to put `void` in the declaration of `f()` is a pretty 
> > > > > > common thing to do because a lot of people read `int f()` as 
> > > > > > declaring a function that takes no arguments (it does in C++ but 
> > > > > > not in C).
> > > > > > 
> > > > > > I don't want the warning to be noisy because I was planning on 
> > > > > > switching it on by default in open source and in a downstream 
> > > > > > use-case make it an error.
> > > > > > 
> > > > > > How about this as a compromise? Split the warning into two separate 
> > > > > > warnings
> > > > > > 
> > > > > > * `strict-call-without-prototype` -  Warns on calls to functions 
> > > > > > without a prototype when no arguments are passed. Not enabled by 
> > > > > > default
> > > > > > * `call-without-prototype` -Warns on calls to functions without a 
> > > > > > prototype when arguments are passed.  Enable this by default.
> > > > > > 
> > > > > > Alternatively we could enable both by default. That would still 
> > > > > > allow me to make `call-without-prototype` an error and 
> > > > > > `strict-call-without-prototype` not be an error for my downstream 
> > > > > > use-case.
> > > > > > 
> > > > > > Thoughts?
> > > > > > Forgetting to put void in the declaration of f() is a pretty common 
> > > > > > thing to do because a lot of people read int f() as declaring a 
> > > > > > function that takes no arguments (it does in C++ but not in C).
> > > > > 
> > > > > Yup, and this is exactly why I think we *should* be warning. That is 
> > > > > a function without a prototype, so the code is very brittle and 
> > > > > dangerous at the call site. The fact that the call site *currently* 
> > > > > is correct doesn't mean it's *intentionally* correct. e.g.,
> > > > > ```
> > > > > // Header.h
> > > > > int f(); // No prototype
> > > > > 
> > > > > // Source.c
> > > > > int f(int a, int b) { return 0; } // Has a prototype, no diagnostic
> > > > > 
> > > > > // OtherSource.c
> > > > > #include "Header.h"
> > > > > 
> > > > > int main() {
> > > > >   return f(); // No diagnostic with this patch, but still have the UB.
> > > > > }
> > > > > ```
> > > > > 
> > > > > > I don't want the warning to be 

[PATCH] D121327: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO

[Dan Liew via Phabricator via cfe-commits]

delcypher added inline comments.



Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:1181
+unsigned Priority, const MCSymbol *KeySym) const {
+  // TODO(yln): Remove -lower-global-dtors-via-cxa-atexit fallback flag
+  // (LowerGlobalDtorsViaCxaAtExit) and issue a fatal error here.

If you have access to `Options.LowerGlobalDtorsViaCxaAtExit` then you could do 
something like

```
if (Options.LowerGlobalDtorsViaCxaAtExit)
  report_fatal_error("@llvm.global_dtors should have been lowered already");
```

so that we error when appropriate.

I'm not sure if you have easy access to the option from this class, so if it's 
difficult then we can ignore this problem.



Comment at: llvm/lib/CodeGen/TargetPassConfig.cpp:900
+  // __cxa_atexit calls to avoid emitting the deprecated __mod_term_func.
+  if (TM->getTargetTriple().isOSBinFormatMachO())
+addPass(createLowerGlobalDtorsLegacyPass());

yln wrote:
> yln wrote:
> > delcypher wrote:
> > > Random thought. Do we want to support the legacy way of calling 
> > > destructors, rather than removing it entirely? If we were to do such a 
> > > thing I'd suspect we'd guard using the legacy way on the OS deployment 
> > > target.
> > > 
> > > Just to be clear. I'm happy with the patch the way it is. I'm just 
> > > wondering if we should consider allowing the legacy way as well. I can't 
> > > see an obvious use case for it because the new way should work on older 
> > > OSs too but maybe there's a use case I haven't thought about?
> > Having a way to explicitly request the old behavior sounds like a good 
> > idea.  I will look into it.
> I added an escape hatch to fallback to the old behavior:
> * via Clang driver flag `-fregister-global-dtors-with-atexit`
> * llc / code generation flag -lower-global-dtors-via-cxa-atexit.
> This escape hatch will be removed in the future.
@yln I don't see any modifications to the driver. How is 
`-fregister-global-dtors-with-atexit` added as a driver flag?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121327

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

[PATCH] D118855: [modules] Add a flag for TagDecl if it was a definition demoted to a declaration.

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

Change seems reasonable but I don't have expertise on this code. I've left a 
few minor nits.




Comment at: clang/include/clang/AST/Decl.h:3494
+  /// symbol and not interested in the final AST with deduplicated definitions.
+  bool isThisDeclarationADemotedDefinition() const {
+return TagDeclBits.IsThisDeclarationADemotedDefinition;

This name feels a little clunky. How about `isDemotedDefinition()`?



Comment at: clang/include/clang/AST/Decl.h:3500
+  /// a definition.
+  void demoteThisDefinitionToDeclaration() {
+assert(isCompleteDefinition() && "Not a definition!");

Given the name of this function (suggesting it always `demotes`) it probably 
should 

```
assert(!isThisDeclarationADemotedDefinition() && "decl already demoted");
```

alternatively comments saying the operation is idempotent is probably fine too.



Comment at: clang/include/clang/AST/Decl.h:3500
+  /// a definition.
+  void demoteThisDefinitionToDeclaration() {
+assert(isCompleteDefinition() && "Not a definition!");

delcypher wrote:
> Given the name of this function (suggesting it always `demotes`) it probably 
> should 
> 
> ```
> assert(!isThisDeclarationADemotedDefinition() && "decl already demoted");
> ```
> 
> alternatively comments saying the operation is idempotent is probably fine 
> too.
The `demoteThisDefinitionToDeclaration()` name feels a little bit clunky. How 
about `demoteDefinitionToDecl()`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118855

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

[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.

[Dan Liew via Phabricator via cfe-commits]

delcypher added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:5529
+def warn_call_function_without_prototype : Warning<
+  "calling function %0 with arguments when function has no prototype">, 
InGroup<
+  DiagGroup<"strict-calls-without-prototype">>, DefaultIgnore;

aaron.ballman wrote:
> delcypher wrote:
> > aaron.ballman wrote:
> > > delcypher wrote:
> > > > aaron.ballman wrote:
> > > > > This diagnostic doesn't tell me what's wrong with the code (and in 
> > > > > fact, there's very possibly nothing wrong with the code whatsoever). 
> > > > > Further, why does calling a function *with no arguments* matter when 
> > > > > it has no prototype? I would imagine this should flag any call to a 
> > > > > function without a prototype given that the function without a 
> > > > > prototype may still expect arguments. e.g.,
> > > > > ```
> > > > > // Header.h
> > > > > int f();
> > > > > 
> > > > > // Source.c
> > > > > int f(a) int a; { ... }
> > > > > 
> > > > > // Caller.c
> > > > > #include "Header.h"
> > > > > 
> > > > > int main() {
> > > > >   return f();
> > > > > }
> > > > > ```
> > > > > I think the diagnostic text should be updated to something more like 
> > > > > `cannot verify %0 is being called with the correct number or 
> > > > > %plural{1:type|:types}1 of arguments because it was declared without 
> > > > > a prototype` (or something along those lines that explains what's 
> > > > > wrong with the code).
> > > > @aaron.ballman  Thanks for the helpful feedback.
> > > > 
> > > > > This diagnostic doesn't tell me what's wrong with the code (and in 
> > > > > fact, there's very possibly nothing wrong with the code whatsoever).
> > > > 
> > > > That's a fair criticism.  I think the diagnostic message you suggest is 
> > > > a lot more helpful so I'll go for something like that.
> > > > 
> > > > > Further, why does calling a function *with no arguments* matter when 
> > > > > it has no prototype?
> > > > 
> > > > The reason was to avoid the warning being noisy. E.g. I didn't the 
> > > > warning to fire in this situation.
> > > > 
> > > > ```
> > > > // Header.h
> > > > int f(); // The user forgot to put `void` between parentheses
> > > > 
> > > > // Source.c
> > > > int f(void) { ... }
> > > > 
> > > > // Caller.c
> > > > #include "Header.h"
> > > > 
> > > > int main() {
> > > >   return f();
> > > > }
> > > > ```
> > > > 
> > > > Forgetting to put `void` in the declaration of `f()` is a pretty common 
> > > > thing to do because a lot of people read `int f()` as declaring a 
> > > > function that takes no arguments (it does in C++ but not in C).
> > > > 
> > > > I don't want the warning to be noisy because I was planning on 
> > > > switching it on by default in open source and in a downstream use-case 
> > > > make it an error.
> > > > 
> > > > How about this as a compromise? Split the warning into two separate 
> > > > warnings
> > > > 
> > > > * `strict-call-without-prototype` -  Warns on calls to functions 
> > > > without a prototype when no arguments are passed. Not enabled by default
> > > > * `call-without-prototype` -Warns on calls to functions without a 
> > > > prototype when arguments are passed.  Enable this by default.
> > > > 
> > > > Alternatively we could enable both by default. That would still allow 
> > > > me to make `call-without-prototype` an error and 
> > > > `strict-call-without-prototype` not be an error for my downstream 
> > > > use-case.
> > > > 
> > > > Thoughts?
> > > > Forgetting to put void in the declaration of f() is a pretty common 
> > > > thing to do because a lot of people read int f() as declaring a 
> > > > function that takes no arguments (it does in C++ but not in C).
> > > 
> > > Yup, and this is exactly why I think we *should* be warning. That is a 
> > > function without a prototype, so the code is very brittle and dangerous 
> > > at the call site. The fact that the call site *currently* is correct 
> > > doesn't mean it's *intentionally* correct. e.g.,
> > > ```
> > > // Header.h
> > > int f(); // No prototype
> > > 
> > > // Source.c
> > > int f(int a, int b) { return 0; } // Has a prototype, no diagnostic
> > > 
> > > // OtherSource.c
> > > #include "Header.h"
> > > 
> > > int main() {
> > >   return f(); // No diagnostic with this patch, but still have the UB.
> > > }
> > > ```
> > > 
> > > > I don't want the warning to be noisy because I was planning on 
> > > > switching it on by default in open source and in a downstream use-case 
> > > > make it an error.
> > > 
> > > Hmmm. Thinking out loud here.
> > > 
> > > Functions without prototypes were standardized in C89 as a deprecated 
> > > feature (C89 3.9.4, 3.9.5). I'd like to get to the point where any code 
> > > that doesn't pass `-ansi` is given a diagnostic (at least in pedantic 
> > > mode outside of system headers) about this deprecation, though I could 
> > > probably be persuaded to keep not diagnose in c89 mode if 

[PATCH] D117924: [compiler_rt] Add a seperate runtime for Mac Catalyst

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

@bc-lee Thanks for the patch. While I get what you're trying to do I have 
doubts about being able to accept the patch in its current form. Apple's ASan 
catalyst doesn't work by building a separate dylib, instead it builds the 
macosx dylib in a different way to make it work with catalyst code.

I've made @aralisza a blocking reviewer because she'll need to approve this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117924

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

[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.

[Dan Liew via Phabricator via cfe-commits]

delcypher added inline comments.



Comment at: clang/test/Sema/warn-calls-without-prototype.c:39
+  return a + b +c;
+}
+

aaron.ballman wrote:
> delcypher wrote:
> > NoQ wrote:
> > > delcypher wrote:
> > > > delcypher wrote:
> > > > > delcypher wrote:
> > > > > > @NoQ Any ideas about this?  It seems kind of weird that when 
> > > > > > merging `not_a_prototype3` prototype with the K style definition 
> > > > > > of `not_a_prototype3` that the resulting FunctionDecl we see at the 
> > > > > > call site in `call_to_function_without_prototype3` is marked as not 
> > > > > > having a prototype.
> > > > > > 
> > > > > > If I flip the order (see `not_a_prototype6`) then the merged 
> > > > > > declaration is marked as having a prototype.
> > > > > > 
> > > > > > I'm not sure if this is a bug in `Sema::MergeFunctionDecl` or if 
> > > > > > this just a peculiarity of K style function definitions.
> > > > > I suspect the problem might be here in `Sema::MergeFunctionDecl`.
> > > > > 
> > > > > ```lang=c++
> > > > >// C: Function types need to be compatible, not identical. This 
> > > > > handles
> > > > >   // duplicate function decls like "void f(int); void f(enum X);" 
> > > > > properly.
> > > > >   if (!getLangOpts().CPlusPlus &&
> > > > >   Context.typesAreCompatible(OldQType, NewQType)) {
> > > > > const FunctionType *OldFuncType = OldQType->getAs();
> > > > > const FunctionType *NewFuncType = NewQType->getAs();
> > > > > const FunctionProtoType *OldProto = nullptr;
> > > > > if (MergeTypeWithOld && isa(NewFuncType) &&
> > > > > (OldProto = dyn_cast(OldFuncType))) {
> > > > >   // The old declaration provided a function prototype, but the
> > > > >   // new declaration does not. Merge in the prototype.
> > > > > ```
> > > > > 
> > > > > ` isa(NewFuncType)` is false in this particular 
> > > > > case, however `New` doesn't have a prototype (i.e. 
> > > > > `New->hasPrototype()` is false). One fix might be to replace 
> > > > > `isa(NewFuncType)` with 
> > > > > 
> > > > > ```
> > > > > (isa(NewFuncType) || !New->hasPrototype())
> > > > > ```
> > > > > 
> > > > > However, I don't really know this code well enough to know if that's 
> > > > > the right fix.
> > > > Okay. I think the above would actually be the wrong location for a fix 
> > > > because in this case we don't need to go down the path that synthesizes 
> > > > the parameters because we already know them for both `old` and `new` in 
> > > > this situation.
> > > > 
> > > > Instead I think the change would have to be in 
> > > > `Sema::MergeCompatibleFunctionDecls` to do something like.
> > > > 
> > > > ```lang=c++
> > > >   // If New is a K function definition it will be marked
> > > >   // as not having a prototype. If `Old` has a prototype
> > > >   // then to "merge" we should mark the K function as having a 
> > > > prototype.
> > > >   if (!getLangOpts().CPlusPlus && Old->hasPrototype() && 
> > > > !New->hasPrototype())
> > > > New->setHasInheritedPrototype(); 
> > > > ```
> > > > 
> > > > What I'm not sure about is if this is semantically the right thing to 
> > > > do. Thoughts?
> > > Ok dunno but I definitely find this whole thing surprising. I'd expect 
> > > this example to be the entirely normal situation for this code, where it 
> > > sees that the new declaration has no prototype so it "inherits" it from 
> > > the old declaration. But you're saying that
> > > 
> > > > `isa(NewFuncType)` is false in this particular case
> > > 
> > > Where does that proto-type come from then? I only see this code affecting 
> > > the type
> > > ```lang=c++
> > >3523   if (RequiresAdjustment) {
> > >3524 const FunctionType *AdjustedType = 
> > > New->getType()->getAs();
> > >3525 AdjustedType = Context.adjustFunctionType(AdjustedType, 
> > > NewTypeInfo);
> > >3526 New->setType(QualType(AdjustedType, 0));
> > >3527 NewQType = Context.getCanonicalType(New->getType());
> > >3528   }
> > > ```
> > > which doesn't seem to touch no-proto-types:
> > > ```lang=c++
> > >3094 const FunctionType *ASTContext::adjustFunctionType(const 
> > > FunctionType *T,
> > >3095
> > > FunctionType::ExtInfo Info) {
> > >3096   if (T->getExtInfo() == Info)
> > >3097 return T;
> > >3098
> > >3099   QualType Result;
> > >3100   if (const auto *FNPT = dyn_cast(T)) {
> > >3101 Result = getFunctionNoProtoType(FNPT->getReturnType(), Info);
> > >...
> > >3107   }
> > >3108
> > >3109   return cast(Result.getTypePtr());
> > >3110 }
> > > ```
> > > So it sounds like `New->getType()` was already with prototype from the 
> > > start? Maybe whatever code has set that type should also have set the 
> > > `HasInheritedPrototype` flag?
> > > Ok dunno but I definitely find this whole thing surprising. I'd expect 
> > > this example to be the entirely normal situation for 

[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.

[Dan Liew via Phabricator via cfe-commits]

delcypher added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:5529
+def warn_call_function_without_prototype : Warning<
+  "calling function %0 with arguments when function has no prototype">, 
InGroup<
+  DiagGroup<"strict-calls-without-prototype">>, DefaultIgnore;

aaron.ballman wrote:
> delcypher wrote:
> > aaron.ballman wrote:
> > > This diagnostic doesn't tell me what's wrong with the code (and in fact, 
> > > there's very possibly nothing wrong with the code whatsoever). Further, 
> > > why does calling a function *with no arguments* matter when it has no 
> > > prototype? I would imagine this should flag any call to a function 
> > > without a prototype given that the function without a prototype may still 
> > > expect arguments. e.g.,
> > > ```
> > > // Header.h
> > > int f();
> > > 
> > > // Source.c
> > > int f(a) int a; { ... }
> > > 
> > > // Caller.c
> > > #include "Header.h"
> > > 
> > > int main() {
> > >   return f();
> > > }
> > > ```
> > > I think the diagnostic text should be updated to something more like 
> > > `cannot verify %0 is being called with the correct number or 
> > > %plural{1:type|:types}1 of arguments because it was declared without a 
> > > prototype` (or something along those lines that explains what's wrong 
> > > with the code).
> > @aaron.ballman  Thanks for the helpful feedback.
> > 
> > > This diagnostic doesn't tell me what's wrong with the code (and in fact, 
> > > there's very possibly nothing wrong with the code whatsoever).
> > 
> > That's a fair criticism.  I think the diagnostic message you suggest is a 
> > lot more helpful so I'll go for something like that.
> > 
> > > Further, why does calling a function *with no arguments* matter when it 
> > > has no prototype?
> > 
> > The reason was to avoid the warning being noisy. E.g. I didn't the warning 
> > to fire in this situation.
> > 
> > ```
> > // Header.h
> > int f(); // The user forgot to put `void` between parentheses
> > 
> > // Source.c
> > int f(void) { ... }
> > 
> > // Caller.c
> > #include "Header.h"
> > 
> > int main() {
> >   return f();
> > }
> > ```
> > 
> > Forgetting to put `void` in the declaration of `f()` is a pretty common 
> > thing to do because a lot of people read `int f()` as declaring a function 
> > that takes no arguments (it does in C++ but not in C).
> > 
> > I don't want the warning to be noisy because I was planning on switching it 
> > on by default in open source and in a downstream use-case make it an error.
> > 
> > How about this as a compromise? Split the warning into two separate warnings
> > 
> > * `strict-call-without-prototype` -  Warns on calls to functions without a 
> > prototype when no arguments are passed. Not enabled by default
> > * `call-without-prototype` -Warns on calls to functions without a prototype 
> > when arguments are passed.  Enable this by default.
> > 
> > Alternatively we could enable both by default. That would still allow me to 
> > make `call-without-prototype` an error and `strict-call-without-prototype` 
> > not be an error for my downstream use-case.
> > 
> > Thoughts?
> > Forgetting to put void in the declaration of f() is a pretty common thing 
> > to do because a lot of people read int f() as declaring a function that 
> > takes no arguments (it does in C++ but not in C).
> 
> Yup, and this is exactly why I think we *should* be warning. That is a 
> function without a prototype, so the code is very brittle and dangerous at 
> the call site. The fact that the call site *currently* is correct doesn't 
> mean it's *intentionally* correct. e.g.,
> ```
> // Header.h
> int f(); // No prototype
> 
> // Source.c
> int f(int a, int b) { return 0; } // Has a prototype, no diagnostic
> 
> // OtherSource.c
> #include "Header.h"
> 
> int main() {
>   return f(); // No diagnostic with this patch, but still have the UB.
> }
> ```
> 
> > I don't want the warning to be noisy because I was planning on switching it 
> > on by default in open source and in a downstream use-case make it an error.
> 
> Hmmm. Thinking out loud here.
> 
> Functions without prototypes were standardized in C89 as a deprecated feature 
> (C89 3.9.4, 3.9.5). I'd like to get to the point where any code that doesn't 
> pass `-ansi` is given a diagnostic (at least in pedantic mode outside of 
> system headers) about this deprecation, though I could probably be persuaded 
> to keep not diagnose in c89 mode if that's a massive pain point. But if in 
> C99 or later, I definitely see no reason not to diagnose the declarations as 
> deprecated by default.
> 
> However, calling a function without a prototype declaration is not itself 
> indicative of a programming mistake and is also not deprecated (it just stops 
> being a problem once all functions are required to have a prototype), so I'm 
> not certain it's well-motivated to enable the new diagnostic by default. This 
> is a bit more like use of VLAs, in that it's a 

[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.

[Dan Liew via Phabricator via cfe-commits]

delcypher added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:5529
+def warn_call_function_without_prototype : Warning<
+  "calling function %0 with arguments when function has no prototype">, 
InGroup<
+  DiagGroup<"strict-calls-without-prototype">>, DefaultIgnore;

aaron.ballman wrote:
> This diagnostic doesn't tell me what's wrong with the code (and in fact, 
> there's very possibly nothing wrong with the code whatsoever). Further, why 
> does calling a function *with no arguments* matter when it has no prototype? 
> I would imagine this should flag any call to a function without a prototype 
> given that the function without a prototype may still expect arguments. e.g.,
> ```
> // Header.h
> int f();
> 
> // Source.c
> int f(a) int a; { ... }
> 
> // Caller.c
> #include "Header.h"
> 
> int main() {
>   return f();
> }
> ```
> I think the diagnostic text should be updated to something more like `cannot 
> verify %0 is being called with the correct number or %plural{1:type|:types}1 
> of arguments because it was declared without a prototype` (or something along 
> those lines that explains what's wrong with the code).
@aaron.ballman  Thanks for the helpful feedback.

> This diagnostic doesn't tell me what's wrong with the code (and in fact, 
> there's very possibly nothing wrong with the code whatsoever).

That's a fair criticism.  I think the diagnostic message you suggest is a lot 
more helpful so I'll go for something like that.

> Further, why does calling a function *with no arguments* matter when it has 
> no prototype?

The reason was to avoid the warning being noisy. E.g. I didn't the warning to 
fire in this situation.

```
// Header.h
int f(); // The user forgot to put `void` between parentheses

// Source.c
int f(void) { ... }

// Caller.c
#include "Header.h"

int main() {
  return f();
}
```

Forgetting to put `void` in the declaration of `f()` is a pretty common thing 
to do because a lot of people read `int f()` as declaring a function that takes 
no arguments (it does in C++ but not in C).

I don't want the warning to be noisy because I was planning on switching it on 
by default in open source and in a downstream use-case make it an error.

How about this as a compromise? Split the warning into two separate warnings

* `strict-call-without-prototype` -  Warns on calls to functions without a 
prototype when no arguments are passed. Not enabled by default
* `call-without-prototype` -Warns on calls to functions without a prototype 
when arguments are passed.  Enable this by default.

Alternatively we could enable both by default. That would still allow me to 
make `call-without-prototype` an error and `strict-call-without-prototype` not 
be an error for my downstream use-case.

Thoughts?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116635

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

[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.

[Dan Liew via Phabricator via cfe-commits]

delcypher added inline comments.



Comment at: clang/test/Sema/warn-calls-without-prototype.c:39
+  return a + b +c;
+}
+

NoQ wrote:
> delcypher wrote:
> > delcypher wrote:
> > > delcypher wrote:
> > > > @NoQ Any ideas about this?  It seems kind of weird that when merging 
> > > > `not_a_prototype3` prototype with the K style definition of 
> > > > `not_a_prototype3` that the resulting FunctionDecl we see at the call 
> > > > site in `call_to_function_without_prototype3` is marked as not having a 
> > > > prototype.
> > > > 
> > > > If I flip the order (see `not_a_prototype6`) then the merged 
> > > > declaration is marked as having a prototype.
> > > > 
> > > > I'm not sure if this is a bug in `Sema::MergeFunctionDecl` or if this 
> > > > just a peculiarity of K style function definitions.
> > > I suspect the problem might be here in `Sema::MergeFunctionDecl`.
> > > 
> > > ```lang=c++
> > >// C: Function types need to be compatible, not identical. This handles
> > >   // duplicate function decls like "void f(int); void f(enum X);" 
> > > properly.
> > >   if (!getLangOpts().CPlusPlus &&
> > >   Context.typesAreCompatible(OldQType, NewQType)) {
> > > const FunctionType *OldFuncType = OldQType->getAs();
> > > const FunctionType *NewFuncType = NewQType->getAs();
> > > const FunctionProtoType *OldProto = nullptr;
> > > if (MergeTypeWithOld && isa(NewFuncType) &&
> > > (OldProto = dyn_cast(OldFuncType))) {
> > >   // The old declaration provided a function prototype, but the
> > >   // new declaration does not. Merge in the prototype.
> > > ```
> > > 
> > > ` isa(NewFuncType)` is false in this particular 
> > > case, however `New` doesn't have a prototype (i.e. `New->hasPrototype()` 
> > > is false). One fix might be to replace 
> > > `isa(NewFuncType)` with 
> > > 
> > > ```
> > > (isa(NewFuncType) || !New->hasPrototype())
> > > ```
> > > 
> > > However, I don't really know this code well enough to know if that's the 
> > > right fix.
> > Okay. I think the above would actually be the wrong location for a fix 
> > because in this case we don't need to go down the path that synthesizes the 
> > parameters because we already know them for both `old` and `new` in this 
> > situation.
> > 
> > Instead I think the change would have to be in 
> > `Sema::MergeCompatibleFunctionDecls` to do something like.
> > 
> > ```lang=c++
> >   // If New is a K function definition it will be marked
> >   // as not having a prototype. If `Old` has a prototype
> >   // then to "merge" we should mark the K function as having a prototype.
> >   if (!getLangOpts().CPlusPlus && Old->hasPrototype() && 
> > !New->hasPrototype())
> > New->setHasInheritedPrototype(); 
> > ```
> > 
> > What I'm not sure about is if this is semantically the right thing to do. 
> > Thoughts?
> Ok dunno but I definitely find this whole thing surprising. I'd expect this 
> example to be the entirely normal situation for this code, where it sees that 
> the new declaration has no prototype so it "inherits" it from the old 
> declaration. But you're saying that
> 
> > `isa(NewFuncType)` is false in this particular case
> 
> Where does that proto-type come from then? I only see this code affecting the 
> type
> ```lang=c++
>3523   if (RequiresAdjustment) {
>3524 const FunctionType *AdjustedType = 
> New->getType()->getAs();
>3525 AdjustedType = Context.adjustFunctionType(AdjustedType, 
> NewTypeInfo);
>3526 New->setType(QualType(AdjustedType, 0));
>3527 NewQType = Context.getCanonicalType(New->getType());
>3528   }
> ```
> which doesn't seem to touch no-proto-types:
> ```lang=c++
>3094 const FunctionType *ASTContext::adjustFunctionType(const FunctionType 
> *T,
>3095
> FunctionType::ExtInfo Info) {
>3096   if (T->getExtInfo() == Info)
>3097 return T;
>3098
>3099   QualType Result;
>3100   if (const auto *FNPT = dyn_cast(T)) {
>3101 Result = getFunctionNoProtoType(FNPT->getReturnType(), Info);
>...
>3107   }
>3108
>3109   return cast(Result.getTypePtr());
>3110 }
> ```
> So it sounds like `New->getType()` was already with prototype from the start? 
> Maybe whatever code has set that type should also have set the 
> `HasInheritedPrototype` flag?
> Ok dunno but I definitely find this whole thing surprising. I'd expect this 
> example to be the entirely normal situation for this code, where it sees that 
> the new declaration has no prototype so it "inherits" it from the old 
> declaration. But you're saying that
> 
> `isa(NewFuncType) `is false in this particular case

Yep.

```
(lldb) p NewFuncType->dump()
FunctionProtoType 0x128829430 'int (int, int)' cdecl
|-BuiltinType 0x12782bf10 'int'
|-BuiltinType 0x12782bf10 'int'
`-BuiltinType 0x12782bf10 'int'
(lldb) p OldFuncType->dump()
FunctionProtoType 0x128829430 'int (int, int)' cdecl

[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.

[Dan Liew via Phabricator via cfe-commits]

delcypher added inline comments.



Comment at: clang/test/Sema/warn-calls-without-prototype.c:39
+  return a + b +c;
+}
+

delcypher wrote:
> delcypher wrote:
> > @NoQ Any ideas about this?  It seems kind of weird that when merging 
> > `not_a_prototype3` prototype with the K style definition of 
> > `not_a_prototype3` that the resulting FunctionDecl we see at the call site 
> > in `call_to_function_without_prototype3` is marked as not having a 
> > prototype.
> > 
> > If I flip the order (see `not_a_prototype6`) then the merged declaration is 
> > marked as having a prototype.
> > 
> > I'm not sure if this is a bug in `Sema::MergeFunctionDecl` or if this just 
> > a peculiarity of K style function definitions.
> I suspect the problem might be here in `Sema::MergeFunctionDecl`.
> 
> ```lang=c++
>// C: Function types need to be compatible, not identical. This handles
>   // duplicate function decls like "void f(int); void f(enum X);" properly.
>   if (!getLangOpts().CPlusPlus &&
>   Context.typesAreCompatible(OldQType, NewQType)) {
> const FunctionType *OldFuncType = OldQType->getAs();
> const FunctionType *NewFuncType = NewQType->getAs();
> const FunctionProtoType *OldProto = nullptr;
> if (MergeTypeWithOld && isa(NewFuncType) &&
> (OldProto = dyn_cast(OldFuncType))) {
>   // The old declaration provided a function prototype, but the
>   // new declaration does not. Merge in the prototype.
> ```
> 
> ` isa(NewFuncType)` is false in this particular case, 
> however `New` doesn't have a prototype (i.e. `New->hasPrototype()` is false). 
> One fix might be to replace `isa(NewFuncType)` with 
> 
> ```
> (isa(NewFuncType) || !New->hasPrototype())
> ```
> 
> However, I don't really know this code well enough to know if that's the 
> right fix.
Okay. I think the above would actually be the wrong location for a fix because 
in this case we don't need to go down the path that synthesizes the parameters 
because we already know them for both `old` and `new` in this situation.

Instead I think the change would have to be in 
`Sema::MergeCompatibleFunctionDecls` to do something like.

```lang=c++
  // If New is a K function definition it will be marked
  // as not having a prototype. If `Old` has a prototype
  // then to "merge" we should mark the K function as having a prototype.
  if (!getLangOpts().CPlusPlus && Old->hasPrototype() && !New->hasPrototype())
New->setHasInheritedPrototype(); 
```

What I'm not sure about is if this is semantically the right thing to do. 
Thoughts?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116635

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

[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.

[Dan Liew via Phabricator via cfe-commits]

delcypher added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:5530
+  "calling function %0 with arguments when function has no prototype">, 
InGroup<
+  DiagGroup<"strict-calls-without-prototype">>, DefaultIgnore;
 def warn_missing_variable_declarations : Warning<

fcloutier wrote:
> sp: I think this should be called "strict-call-without-prototype" (singular 
> call) as most warning flag names seem to prefer singulars.
@fcloutier 
I'm open to renaming the warning. I originally made the warning plural because 
there were warnings near by that used it. E.g.

* `missing-variable-declarations`
* `strict-prototypes`
* `missing-prototypes`

If no one else chimes in I'll make it singular.

Also, do you think we should drop the "strict" prefix? I.e. 
`call-without-prototype` instead of `strict-call-without-prototype`. The reason 
I'm thinking that using `strict` might be a bad idea is:

1. The warning isn't actually strict. A strict version of the warning would 
warn on **all calls to functions without prototypes**. We've decided to only 
warn on calls with arguments to avoid noisy warnings that result from people 
frequently forgetting to explicitly mark functions as taking no arguments.

2. Calling this warning  `strict-call-without-prototype` precludes from adding 
a stricter version of warning in the future (i.e. we'd be force to name it 
something like `strict-strict-call-without-prototype` which is super confusing).

3. The intention in a future patch is to enable the warning by default. It 
seems odd to have a "strict" warning on by default given that there is 
precedence for doing the opposite (e.g. `-Wstrict-prototypes` is off by 
default).



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116635

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

[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.

[Dan Liew via Phabricator via cfe-commits]

delcypher added inline comments.



Comment at: clang/test/Sema/warn-calls-without-prototype.c:39
+  return a + b +c;
+}
+

delcypher wrote:
> @NoQ Any ideas about this?  It seems kind of weird that when merging 
> `not_a_prototype3` prototype with the K style definition of 
> `not_a_prototype3` that the resulting FunctionDecl we see at the call site in 
> `call_to_function_without_prototype3` is marked as not having a prototype.
> 
> If I flip the order (see `not_a_prototype6`) then the merged declaration is 
> marked as having a prototype.
> 
> I'm not sure if this is a bug in `Sema::MergeFunctionDecl` or if this just a 
> peculiarity of K style function definitions.
I suspect the problem might be here in `Sema::MergeFunctionDecl`.

```lang=c++
   // C: Function types need to be compatible, not identical. This handles
  // duplicate function decls like "void f(int); void f(enum X);" properly.
  if (!getLangOpts().CPlusPlus &&
  Context.typesAreCompatible(OldQType, NewQType)) {
const FunctionType *OldFuncType = OldQType->getAs();
const FunctionType *NewFuncType = NewQType->getAs();
const FunctionProtoType *OldProto = nullptr;
if (MergeTypeWithOld && isa(NewFuncType) &&
(OldProto = dyn_cast(OldFuncType))) {
  // The old declaration provided a function prototype, but the
  // new declaration does not. Merge in the prototype.
```

` isa(NewFuncType)` is false in this particular case, 
however `New` doesn't have a prototype (i.e. `New->hasPrototype()` is false). 
One fix might be to replace `isa(NewFuncType)` with 

```
(isa(NewFuncType) || !New->hasPrototype())
```

However, I don't really know this code well enough to know if that's the right 
fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116635

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

[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

Patch https://reviews.llvm.org/D116636 demonstrates the changes needed if the 
warning was enabled by default:


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116635

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

[PATCH] D116636: [WIP] Enable `-Wstrict-calls-without-prototype` by default

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

This change depends on https://reviews.llvm.org/D116635.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116636

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

[PATCH] D116636: [WIP] Enable `-Wstrict-calls-without-prototype` by default

[Dan Liew via Phabricator via cfe-commits]

delcypher created this revision.
delcypher added reviewers: arphaman, rapidsna, fcloutier, NoQ.
delcypher requested review of this revision.
Herald added a project: clang.

This patch enables the `-Wstrict-calls-without-prototype` warning by
default and fixes the existing clang test suite to pass.

This patch is considered work-in-progress because the patch it is
based on is not finalized. It is also unclear at this point whether this 
warning should actually
be enabled by default. So this patch serves to illustrate the changes
that would need to be made to have the warning on by default.

rdar://87118271


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116636

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/test/Analysis/casts.c
  clang/test/Analysis/inline.c
  clang/test/Analysis/misc-ps-region-store.m
  clang/test/Analysis/misc-ps.m
  clang/test/Analysis/null-deref-ps.c
  clang/test/Analysis/solver-sym-simplification-adjustment.c
  clang/test/Analysis/solver-sym-simplification-concreteint.c
  clang/test/CodeGen/functions.c
  clang/test/Sema/arg-duplicate.c
  clang/test/Sema/block-misc.c
  clang/test/Sema/extern-redecl.c
  clang/test/Sema/function-redecl.c
  clang/test/Sema/function.c
  clang/test/Sema/knr-def-call.c
  clang/test/Sema/knr-variadic-def.c
  clang/test/Sema/merge-decls.c
  clang/test/Sema/sizeless-1.c
  clang/test/Sema/unused-expr.c
  clang/test/SemaObjC/nonnull.m
  clang/test/SemaObjC/protocol-archane.m

Index: clang/test/SemaObjC/protocol-archane.m
===
--- clang/test/SemaObjC/protocol-archane.m
+++ clang/test/SemaObjC/protocol-archane.m
@@ -5,6 +5,8 @@
 - (void) bar;
 @end
 
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wstrict-calls-without-prototype"
 void bar();
 void foo(id x) {
   bar((short)x); // expected-error {{expected ')'}} expected-note {{to match this '('}}
@@ -12,6 +14,7 @@
 
   [()x bar];  // expected-warning {{protocol has no object type specified; defaults to qualified 'id'}}
 }
+#pragma clang diagnostic pop
 
 @protocol MyProtocol
 - (void)doSomething;
Index: clang/test/SemaObjC/nonnull.m
===
--- clang/test/SemaObjC/nonnull.m
+++ clang/test/SemaObjC/nonnull.m
@@ -23,8 +23,8 @@
 extern void func4 (void (^block1)(), void (^block2)()) __attribute__((nonnull(1)))
 __attribute__((nonnull(2)));
 
-void func6();
-void func7();
+void func6(NSObject *);
+void func7(NSObject *);
 
 void
 foo (int i1, int i2, int i3, void (^cp1)(), void (^cp2)(), void (^cp3)())
@@ -63,7 +63,7 @@
 __attribute__((nonnull))
 void _dispatch_queue_push_list(dispatch_object_t _head); // no warning
 
-void func6(dispatch_object_t _head) {
+void func8(dispatch_object_t _head) {
   _dispatch_queue_push_list(0); // expected-warning {{null passed to a callee that requires a non-null argument}}
   _dispatch_queue_push_list(_head._do);  // no warning
 }
Index: clang/test/Sema/unused-expr.c
===
--- clang/test/Sema/unused-expr.c
+++ clang/test/Sema/unused-expr.c
@@ -79,10 +79,9 @@
   t5f();   // expected-warning {{ignoring return value of function declared with 'warn_unused_result' attribute}}
 }
 
-
 int fn1() __attribute__ ((warn_unused_result));
-int fn2() __attribute__ ((pure));
-int fn3() __attribute__ ((__const));
+int fn2(int, int) __attribute__((pure));
+int fn3(int) __attribute__((__const));
 // rdar://6587766
 int t6() {
   if (fn1() < 0 || fn2(2,1) < 0 || fn3(2) < 0)  // no warnings
@@ -129,18 +128,18 @@
 #define NOP(a) (a)
 #define M1(a, b) (long)foo((a), (b))
 #define M2 (long)0;
-#define M3(a) (t3(a), fn2())
+#define M3(a) (t3(a), fn2(0, 1))
 #define M4(a, b) (foo((a), (b)) ? 0 : t3(a), 1)
 #define M5(a, b) (foo((a), (b)), 1)
 #define M6() fn1()
-#define M7() fn2()
+#define M7() fn2(0, 1)
 void t11(int i, int j) {
   M1(i, j);  // no warning
   NOP((long)foo(i, j)); // expected-warning {{expression result unused}}
   M2;  // no warning
   NOP((long)0); // expected-warning {{expression result unused}}
   M3(i); // no warning
-  NOP((t3(i), fn2())); // expected-warning {{ignoring return value}}
+  NOP((t3(i), fn2(0, 0))); // expected-warning {{ignoring return value}}
   M4(i, j); // no warning
   NOP((foo(i, j) ? 0 : t3(i), 1)); // expected-warning {{expression result unused}}
   M5(i, j); // no warning
Index: clang/test/Sema/sizeless-1.c
===
--- clang/test/Sema/sizeless-1.c
+++ clang/test/Sema/sizeless-1.c
@@ -42,7 +42,7 @@
 void __attribute__((overloadable)) overf16(svint16_t); // expected-note + {{not viable}}
 void __attribute__((overloadable)) overf16(int);   // expected-note + {{not viable}}
 
-void noproto();
+void noproto(); // expected-note{{'noproto' declared here}}
 void varargs(int, ...);
 
 void unused() {
@@ -174,7 +174,7 @@
   overf16(local_int8); // expected-error {{no 

[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.

[Dan Liew via Phabricator via cfe-commits]

delcypher added inline comments.



Comment at: clang/test/Sema/warn-calls-without-prototype.c:39
+  return a + b +c;
+}
+

@NoQ Any ideas about this?  It seems kind of weird that when merging 
`not_a_prototype3` prototype with the K style definition of 
`not_a_prototype3` that the resulting FunctionDecl we see at the call site in 
`call_to_function_without_prototype3` is marked as not having a prototype.

If I flip the order (see `not_a_prototype6`) then the merged declaration is 
marked as having a prototype.

I'm not sure if this is a bug in `Sema::MergeFunctionDecl` or if this just a 
peculiarity of K style function definitions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116635

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

[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.

[Dan Liew via Phabricator via cfe-commits]

delcypher updated this revision to Diff 397431.
delcypher added a comment.

Add another test case for function definitions without prototypes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116635

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Sema/warn-calls-without-prototype.c

Index: clang/test/Sema/warn-calls-without-prototype.c
===
--- /dev/null
+++ clang/test/Sema/warn-calls-without-prototype.c
@@ -0,0 +1,151 @@
+// RUN: %clang_cc1 -triple i386-pc-unknown -fsyntax-only -Wstrict-calls-without-prototype -std=c99 -verify %s
+
+//--
+// Expected -Wstrict-calls-without-prototype warnings
+//--
+
+unsigned not_a_prototype();
+
+unsigned call_to_function_without_prototype(void) {
+  // expected-warning@+2{{calling function 'not_a_prototype' with arguments when function has no prototype}}
+  // expected-note@-4{{'not_a_prototype' declared here}}
+  return not_a_prototype(1);
+}
+
+// K style function declaration has no proto-type
+unsigned not_a_prototype2(a, b)
+int a;
+int b;
+{
+  return a + b;
+}
+
+unsigned call_to_function_without_prototype2(void) {
+  // expected-warning@+2{{calling function 'not_a_prototype2' with arguments when function has no prototype}}
+  // expected-note@-9{{'not_a_prototype2' declared here}}
+  return not_a_prototype2(1, 2);
+}
+
+// K style function seems to "hide" the prototype because the merged
+// FunctionDecl seems to not have a prototype.
+// FIXME(dliew): Is this a bug in `Sema::MergeFunctionDecl`?
+unsigned not_a_prototype3(int a, int b, int c); // Start with a prototype
+unsigned not_a_prototype3(a, b, c )
+  int a;
+  int b;
+  int c;
+{
+  return a + b +c;
+}
+
+unsigned call_to_function_without_prototype3(void) {
+  // FIXME(dliew): Should we actually warn about this? There is a prototype it
+  // just gets hidden when the FunctionDecls get merged.
+  // expected-warning@+2{{calling function 'not_a_prototype3' with arguments when function has no prototype}}
+  // expected-note@-12{{'not_a_prototype3' declared here}}
+  return not_a_prototype3(1, 2, 3);
+}
+
+// Merging two FunctionDecls without prototypes should result in a FunctionDecl without a prototype.
+unsigned not_a_prototype4(); // Start with something that has no prototype.
+unsigned not_a_prototype4(a, b, c)
+  int a;
+  int b;
+  int c;
+{
+  return a + b +c;
+}
+
+unsigned call_to_function_without_prototype4(void) {
+  // expected-warning@+2{{calling function 'not_a_prototype4' with arguments when function has no prototype}}
+  // expected-note@-10{{'not_a_prototype4' declared here}}
+  return not_a_prototype4(1, 2, 3);
+}
+
+// Definition does not have a prototype
+unsigned not_a_prototype5() {
+  return 0;
+}
+
+unsigned call_to_function_without_prototype5(void) {
+  // expected-warning@+3{{too many arguments in call to 'not_a_prototype5'}}
+  // expected-warning@+2{{calling function 'not_a_prototype5' with arguments when function has no prototype}}
+  // expected-note@-7{{'not_a_prototype5' declared here}}
+  return not_a_prototype5(1, 2, 3);
+}
+
+//--
+// No expected warnings from -Wstrict-calls-without-prototype
+//--
+
+unsigned call_to_function_without_declartion(void) {
+  // `-Wstrict-calls-without-prototype` doesn't emit a warning here because
+  // `-Wimplicit-function-declaration` cover this.
+  // expected-warning@+1{{implicit declaration of function 'function_without_declaration' is invalid in C99}}
+  return function_without_declaration(5);
+}
+
+unsigned call_builtins(void) {
+  // Builtin with a signature
+  int *alloc = (int *)__builtin_alloca(sizeof(int)); // no warning
+  *alloc = 5;
+
+  // Builtin without a meaningful signature
+  int gt = __builtin_isgreater(0.0, 0.1); // no warning
+
+  return *alloc + gt;
+}
+
+unsigned provides_prototype(int *a);
+unsigned provides_prototype_from_definition(int *a) {
+  return a[1];
+}
+unsigned provides_prototype2(void) {
+  return 0;
+}
+unsigned call_prototypes(void) {
+  int a[] = {0, 1, 2};
+  int result = provides_prototype_from_definition(a); // no warning
+  int result2 = provides_prototype(a); // no warning
+  // Deliberately pass arguments. Even in this case
+  //`-Wstrict-calls-without-prototype` should not fire.
+  // expected-error@+2{{too many arguments to function call, expected 0, have 1}}
+  // expected-note@-10{{'provides_prototype2' declared here}}
+  int result3 = provides_prototype2(5);
+  return result + result2 + result3;
+}
+
+unsigned will_be_refined();
+unsigned will_be_refined(void *); // refine the decl to be a 

[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.

[Dan Liew via Phabricator via cfe-commits]

delcypher created this revision.
delcypher added reviewers: arphaman, rapidsna, fcloutier, NoQ.
delcypher requested review of this revision.
Herald added a project: clang.

The warning is currently disabled by default but can be enabled with
`-Wstrict-calls-without-prototype`. Enabling it by default will be
handled by future patches.

The warning is intended to catch C code like this:

  int f(); // No prototype, accepts any number of arguments of any type.
  
  int call(void) {
return f(1, 2, 3);
  }

While code like this is legal it can very easily invoke undefined behavior
by passing the wrong number of arguments or wrong types.

The warning only fires when arguments are passed to make it less noisy, i.e.
so that the warning does not fire on code like this:

  int g();
  int call(void) {
  return g();
  }

While the above code could invoke undefined behavior, if the definition of 
`g()` takes no
arguments then the lack of a prototype is benign.

This warning while similar to `-Wstrict-prototypes` is distinct because
it warns at call sites rather at declarations.

This patch currently warns on calls to K style function declarations where a
previous declaration has a prototype. It is currently not clear if this is the 
correct behavior
as noted in the included test case.

rdar://87118271


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116635

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Sema/warn-calls-without-prototype.c

Index: clang/test/Sema/warn-calls-without-prototype.c
===
--- /dev/null
+++ clang/test/Sema/warn-calls-without-prototype.c
@@ -0,0 +1,131 @@
+// RUN: %clang_cc1 -triple i386-pc-unknown -fsyntax-only -Wstrict-calls-without-prototype -std=c99 -verify %s
+
+//--
+// Expected -Wstrict-calls-without-prototype warnings
+//--
+
+unsigned not_a_prototype();
+
+unsigned call_to_function_without_prototype(void) {
+  // expected-warning@+2{{calling function 'not_a_prototype' with arguments when function has no prototype}}
+  // expected-note@-4{{'not_a_prototype' declared here}}
+  return not_a_prototype(1);
+}
+
+// K style function declaration has no proto-type
+unsigned not_a_prototype2(a, b)
+int a;
+int b;
+{
+  return a + b;
+}
+
+unsigned call_to_function_without_prototype2(void) {
+  // expected-warning@+2{{calling function 'not_a_prototype2' with arguments when function has no prototype}}
+  // expected-note@-9{{'not_a_prototype2' declared here}}
+  return not_a_prototype2(1, 2);
+}
+
+// K style function seems to "hide" the prototype because the merged
+// FunctionDecl seems to not have a prototype.
+// FIXME(dliew): Is this a bug in `Sema::MergeFunctionDecl`?
+unsigned not_a_prototype3(int a, int b, int c); // Start with a prototype
+unsigned not_a_prototype3(a, b, c )
+  int a;
+  int b;
+  int c;
+{
+  return a + b +c;
+}
+
+unsigned call_to_function_without_prototype3(void) {
+  // FIXME(dliew): Should we actually warn about this? There is a prototype it
+  // just gets hidden when the FunctionDecls get merged.
+  // expected-warning@+2{{calling function 'not_a_prototype3' with arguments when function has no prototype}}
+  // expected-note@-12{{'not_a_prototype3' declared here}}
+  return not_a_prototype3(1, 2, 3);
+}
+
+// Merging two FunctionDecls without prototypes should result in a FunctionDecl without a prototype.
+unsigned not_a_prototype4(); // Start with something that has no prototype.
+unsigned not_a_prototype4(a, b, c)
+  int a;
+  int b;
+  int c;
+{
+  return a + b +c;
+}
+
+unsigned call_to_function_without_prototype4(void) {
+  // expected-warning@+2{{calling function 'not_a_prototype4' with arguments when function has no prototype}}
+  // expected-note@-10{{'not_a_prototype4' declared here}}
+  return not_a_prototype4(1, 2, 3);
+}
+
+//--
+// No expected warnings from -Wstrict-calls-without-prototype
+//--
+
+unsigned call_to_function_without_declartion(void) {
+  // `-Wstrict-calls-without-prototype` doesn't emit a warning here because
+  // `-Wimplicit-function-declaration` cover this.
+  // expected-warning@+1{{implicit declaration of function 'function_without_declaration' is invalid in C99}}
+  return function_without_declaration(5);
+}
+
+unsigned call_builtins(void) {
+  // Builtin with a signature
+  int *alloc = (int *)__builtin_alloca(sizeof(int)); // no warning
+  *alloc = 5;
+
+  // Builtin without a meaningful signature
+  int gt = __builtin_isgreater(0.0, 0.1); // no warning
+
+  return *alloc + gt;
+}
+
+unsigned provides_prototype(int *a);
+unsigned provides_prototype_from_definition(int *a) {
+  return 

[PATCH] D105489: [compiler-rt] [test] Fix asan symbolize tests on py3.10

[Dan Liew via Phabricator via cfe-commits]

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

LGTM


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

https://reviews.llvm.org/D105489

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

[PATCH] D101682: [Driver] Fix `ToolChain::getCompilerRTPath()` to return the correct path on Apple platforms.

[Dan Liew 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 rG1971823ecb9e: [Driver] Fix `ToolChain::getCompilerRTPath()` 
to return the correct path on… (authored by delcypher).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101682

Files:
  clang/lib/Driver/ToolChain.cpp
  clang/test/Driver/darwin-print-file-name.c
  clang/test/Driver/darwin-print-runtime-dir.c


Index: clang/test/Driver/darwin-print-runtime-dir.c
===
--- /dev/null
+++ clang/test/Driver/darwin-print-runtime-dir.c
@@ -0,0 +1,24 @@
+// Regression test. Previously the output returned the full OS name
+// (e.g. `darwin20.3.0`) instead of just `darwin`.
+
+// RUN: %clang -print-runtime-dir --target=x86_64-apple-darwin20.3.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+
+// RUN: %clang -print-runtime-dir --target=x86_64-apple-macosx11.0.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+
+// RUN: %clang -print-runtime-dir --target=arm64-apple-ios14.0.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+
+// RUN: %clang -print-runtime-dir --target=arm64-apple-tvos14.0.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+
+// RUN: %clang -print-runtime-dir --target=arm64-apple-watchos5.0.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+
+// PRINT-RUNTIME-DIR: lib{{/|\\}}darwin{{$}}
Index: clang/test/Driver/darwin-print-file-name.c
===
--- /dev/null
+++ clang/test/Driver/darwin-print-file-name.c
@@ -0,0 +1,27 @@
+// Regression test. Previously Clang just returned the library name instead
+// of the full path.
+
+// RUN: %clang -print-file-name=libclang_rt.osx.a 
--target=x86_64-apple-darwin20.3.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+
+// RUN: %clang -print-file-name=libclang_rt.osx.a 
--target=x86_64-apple-macosx11.0.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+
+// PRINT-RUNTIME-DIR: lib{{/|\\}}darwin{{/|\\}}libclang_rt.osx.a
+
+// RUN: %clang -print-file-name=libclang_rt.ios.a 
--target=arm64-apple-ios14.0.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR-IOS %s
+// PRINT-RUNTIME-DIR-IOS: lib{{/|\\}}darwin{{/|\\}}libclang_rt.ios.a
+
+// RUN: %clang -print-file-name=libclang_rt.tvos.a 
--target=arm64-apple-tvos14.0.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR-TVOS %s
+// PRINT-RUNTIME-DIR-TVOS: lib{{/|\\}}darwin{{/|\\}}libclang_rt.tvos.a
+
+// RUN: %clang -print-file-name=libclang_rt.watchos.a 
--target=arm64-apple-watchos5.0.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR-WATCHOS %s
+// PRINT-RUNTIME-DIR-WATCHOS: lib{{/|\\}}darwin{{/|\\}}libclang_rt.watchos.a
Index: clang/lib/Driver/ToolChain.cpp
===
--- clang/lib/Driver/ToolChain.cpp
+++ clang/lib/Driver/ToolChain.cpp
@@ -387,6 +387,9 @@
 }
 
 StringRef ToolChain::getOSLibName() const {
+  if (Triple.isOSDarwin())
+return "darwin";
+
   switch (Triple.getOS()) {
   case llvm::Triple::FreeBSD:
 return "freebsd";


Index: clang/test/Driver/darwin-print-runtime-dir.c
===
--- /dev/null
+++ clang/test/Driver/darwin-print-runtime-dir.c
@@ -0,0 +1,24 @@
+// Regression test. Previously the output returned the full OS name
+// (e.g. `darwin20.3.0`) instead of just `darwin`.
+
+// RUN: %clang -print-runtime-dir --target=x86_64-apple-darwin20.3.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+
+// RUN: %clang -print-runtime-dir --target=x86_64-apple-macosx11.0.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+
+// RUN: %clang -print-runtime-dir --target=arm64-apple-ios14.0.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+
+// RUN: %clang -print-runtime-dir --target=arm64-apple-tvos14.0.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+
+// RUN: %clang -print-runtime-dir 

[PATCH] D101682: [Driver] Fix `ToolChain::getCompilerRTPath()` to return the correct path on Apple platforms.

[Dan Liew via Phabricator via cfe-commits]

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



Comment at: clang/lib/Driver/ToolChain.cpp:408
   default:
 return getOS();
   }

arphaman wrote:
> It might be cleaner to do a check for `isOSDarwin` here, as that will help 
> with any other Darwin platforms that we need to support.
@arphaman That seems like a good idea. I didn't know this API existed. I'll try 
to switch to that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101682

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

[PATCH] D101682: [Driver] Fix `ToolChain::getCompilerRTPath()` to return the correct path on Apple platforms.

[Dan Liew via Phabricator via cfe-commits]

delcypher updated this revision to Diff 342809.
delcypher edited the summary of this revision.
delcypher added a comment.

Use `isOSDarwin()` instead of explicitly checking Darwin OS enum values.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101682

Files:
  clang/lib/Driver/ToolChain.cpp
  clang/test/Driver/darwin-print-file-name.c
  clang/test/Driver/darwin-print-runtime-dir.c


Index: clang/test/Driver/darwin-print-runtime-dir.c
===
--- /dev/null
+++ clang/test/Driver/darwin-print-runtime-dir.c
@@ -0,0 +1,24 @@
+// Regression test. Previously the output returned the full OS name
+// (e.g. `darwin20.3.0`) instead of just `darwin`.
+
+// RUN: %clang -print-runtime-dir --target=x86_64-apple-darwin20.3.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+
+// RUN: %clang -print-runtime-dir --target=x86_64-apple-macosx11.0.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+
+// RUN: %clang -print-runtime-dir --target=arm64-apple-ios14.0.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+
+// RUN: %clang -print-runtime-dir --target=arm64-apple-tvos14.0.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+
+// RUN: %clang -print-runtime-dir --target=arm64-apple-watchos5.0.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+
+// PRINT-RUNTIME-DIR: lib{{/|\\}}darwin{{$}}
Index: clang/test/Driver/darwin-print-file-name.c
===
--- /dev/null
+++ clang/test/Driver/darwin-print-file-name.c
@@ -0,0 +1,27 @@
+// Regression test. Previously Clang just returned the library name instead
+// of the full path.
+
+// RUN: %clang -print-file-name=libclang_rt.osx.a 
--target=x86_64-apple-darwin20.3.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+
+// RUN: %clang -print-file-name=libclang_rt.osx.a 
--target=x86_64-apple-macosx11.0.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+
+// PRINT-RUNTIME-DIR: lib{{/|\\}}darwin{{/|\\}}libclang_rt.osx.a
+
+// RUN: %clang -print-file-name=libclang_rt.ios.a 
--target=arm64-apple-ios14.0.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR-IOS %s
+// PRINT-RUNTIME-DIR-IOS: lib{{/|\\}}darwin{{/|\\}}libclang_rt.ios.a
+
+// RUN: %clang -print-file-name=libclang_rt.tvos.a 
--target=arm64-apple-tvos14.0.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR-TVOS %s
+// PRINT-RUNTIME-DIR-TVOS: lib{{/|\\}}darwin{{/|\\}}libclang_rt.tvos.a
+
+// RUN: %clang -print-file-name=libclang_rt.watchos.a 
--target=arm64-apple-watchos5.0.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR-WATCHOS %s
+// PRINT-RUNTIME-DIR-WATCHOS: lib{{/|\\}}darwin{{/|\\}}libclang_rt.watchos.a
Index: clang/lib/Driver/ToolChain.cpp
===
--- clang/lib/Driver/ToolChain.cpp
+++ clang/lib/Driver/ToolChain.cpp
@@ -387,6 +387,9 @@
 }
 
 StringRef ToolChain::getOSLibName() const {
+  if (Triple.isOSDarwin())
+return "darwin";
+
   switch (Triple.getOS()) {
   case llvm::Triple::FreeBSD:
 return "freebsd";


Index: clang/test/Driver/darwin-print-runtime-dir.c
===
--- /dev/null
+++ clang/test/Driver/darwin-print-runtime-dir.c
@@ -0,0 +1,24 @@
+// Regression test. Previously the output returned the full OS name
+// (e.g. `darwin20.3.0`) instead of just `darwin`.
+
+// RUN: %clang -print-runtime-dir --target=x86_64-apple-darwin20.3.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+
+// RUN: %clang -print-runtime-dir --target=x86_64-apple-macosx11.0.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+
+// RUN: %clang -print-runtime-dir --target=arm64-apple-ios14.0.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+
+// RUN: %clang -print-runtime-dir --target=arm64-apple-tvos14.0.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+
+// RUN: %clang -print-runtime-dir --target=arm64-apple-watchos5.0.0 \
+// RUN:

[PATCH] D101682: [Driver] Fix `ToolChain::getCompilerRTPath()` to return the correct path on Darwin.

[Dan Liew via Phabricator via cfe-commits]

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



Comment at: clang/lib/Driver/ToolChain.cpp:401
 return "aix";
+  case llvm::Triple::Darwin:
+return "darwin";

arphaman wrote:
> Should this also apply to `llvm::Triple::MacOSX` , `iOS`, and other Darwin 
> OSes?
Good catch. I've fixed this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101682

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

[PATCH] D101682: [Driver] Fix `ToolChain::getCompilerRTPath()` to return the correct path on Darwin.

[Dan Liew via Phabricator via cfe-commits]

delcypher updated this revision to Diff 342152.
delcypher added a comment.

- Support other Apple target triples.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101682

Files:
  clang/lib/Driver/ToolChain.cpp
  clang/test/Driver/darwin-print-file-name.c
  clang/test/Driver/darwin-print-runtime-dir.c


Index: clang/test/Driver/darwin-print-runtime-dir.c
===
--- /dev/null
+++ clang/test/Driver/darwin-print-runtime-dir.c
@@ -0,0 +1,24 @@
+// Regression test. Previously the output returned the full OS name
+// (e.g. `darwin20.3.0`) instead of just `darwin`.
+
+// RUN: %clang -print-runtime-dir --target=x86_64-apple-darwin20.3.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+
+// RUN: %clang -print-runtime-dir --target=x86_64-apple-macosx11.0.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+
+// RUN: %clang -print-runtime-dir --target=arm64-apple-ios14.0.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+
+// RUN: %clang -print-runtime-dir --target=arm64-apple-tvos14.0.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+
+// RUN: %clang -print-runtime-dir --target=arm64-apple-watchos5.0.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+
+// PRINT-RUNTIME-DIR: lib{{/|\\}}darwin{{$}}
Index: clang/test/Driver/darwin-print-file-name.c
===
--- /dev/null
+++ clang/test/Driver/darwin-print-file-name.c
@@ -0,0 +1,27 @@
+// Regression test. Previously Clang just returned the library name instead
+// of the full path.
+
+// RUN: %clang -print-file-name=libclang_rt.osx.a 
--target=x86_64-apple-darwin20.3.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+
+// RUN: %clang -print-file-name=libclang_rt.osx.a 
--target=x86_64-apple-macosx11.0.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+
+// PRINT-RUNTIME-DIR: lib{{/|\\}}darwin{{/|\\}}libclang_rt.osx.a
+
+// RUN: %clang -print-file-name=libclang_rt.ios.a 
--target=arm64-apple-ios14.0.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR-IOS %s
+// PRINT-RUNTIME-DIR-IOS: lib{{/|\\}}darwin{{/|\\}}libclang_rt.ios.a
+
+// RUN: %clang -print-file-name=libclang_rt.tvos.a 
--target=arm64-apple-tvos14.0.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR-TVOS %s
+// PRINT-RUNTIME-DIR-TVOS: lib{{/|\\}}darwin{{/|\\}}libclang_rt.tvos.a
+
+// RUN: %clang -print-file-name=libclang_rt.watchos.a 
--target=arm64-apple-watchos5.0.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR-WATCHOS %s
+// PRINT-RUNTIME-DIR-WATCHOS: lib{{/|\\}}darwin{{/|\\}}libclang_rt.watchos.a
Index: clang/lib/Driver/ToolChain.cpp
===
--- clang/lib/Driver/ToolChain.cpp
+++ clang/lib/Driver/ToolChain.cpp
@@ -398,6 +398,12 @@
 return "sunos";
   case llvm::Triple::AIX:
 return "aix";
+  case llvm::Triple::Darwin:
+  case llvm::Triple::MacOSX:
+  case llvm::Triple::IOS:
+  case llvm::Triple::TvOS:
+  case llvm::Triple::WatchOS:
+return "darwin";
   default:
 return getOS();
   }


Index: clang/test/Driver/darwin-print-runtime-dir.c
===
--- /dev/null
+++ clang/test/Driver/darwin-print-runtime-dir.c
@@ -0,0 +1,24 @@
+// Regression test. Previously the output returned the full OS name
+// (e.g. `darwin20.3.0`) instead of just `darwin`.
+
+// RUN: %clang -print-runtime-dir --target=x86_64-apple-darwin20.3.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+
+// RUN: %clang -print-runtime-dir --target=x86_64-apple-macosx11.0.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+
+// RUN: %clang -print-runtime-dir --target=arm64-apple-ios14.0.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+
+// RUN: %clang -print-runtime-dir --target=arm64-apple-tvos14.0.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+
+// RUN: %clang -print-runtime-dir --target=arm64-apple-watchos5.0.0 \
+// RUN:

[PATCH] D101682: [Driver] Fix `ToolChain::getCompilerRTPath()` to return the correct path on Darwin.

[Dan Liew via Phabricator via cfe-commits]

delcypher created this revision.
delcypher added reviewers: arphaman, dexonsmith, kubamracek, aralisza, yln.
delcypher requested review of this revision.
Herald added a project: clang.

When the Darwin target triple included the OS version number this would cause
`ToolChain::getCompilerRTPath()` to return an incorrect path because the
suffix was something like `darwin20.3.0` instead of `darwin`.

This in turn caused

- `-print-runtime-dir` to return a non-existant path.
- `-print-file-name=` to not return a useful path.

Two regression tests are included.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D101682

Files:
  clang/lib/Driver/ToolChain.cpp
  clang/test/Driver/darwin-print-file-name.c
  clang/test/Driver/darwin-print-runtime-dir.c


Index: clang/test/Driver/darwin-print-runtime-dir.c
===
--- /dev/null
+++ clang/test/Driver/darwin-print-runtime-dir.c
@@ -0,0 +1,6 @@
+// Regression test. Previously the output included the OS version number
+// which was not correct.
+// RUN: %clang -print-runtime-dir --target=x86_64-apple-darwin20.3.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+// PRINT-RUNTIME-DIR: lib{{/|\\}}darwin{{$}}
\ No newline at end of file
Index: clang/test/Driver/darwin-print-file-name.c
===
--- /dev/null
+++ clang/test/Driver/darwin-print-file-name.c
@@ -0,0 +1,6 @@
+// Regression test. Previously Clang just returned the library name instead
+// of the full path.
+// RUN: %clang -print-file-name=libclang_rt.osx.a 
--target=x86_64-apple-darwin20.3.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+// PRINT-RUNTIME-DIR: lib{{/|\\}}darwin{{/|\\}}libclang_rt.osx.a
\ No newline at end of file
Index: clang/lib/Driver/ToolChain.cpp
===
--- clang/lib/Driver/ToolChain.cpp
+++ clang/lib/Driver/ToolChain.cpp
@@ -398,6 +398,8 @@
 return "sunos";
   case llvm::Triple::AIX:
 return "aix";
+  case llvm::Triple::Darwin:
+return "darwin";
   default:
 return getOS();
   }


Index: clang/test/Driver/darwin-print-runtime-dir.c
===
--- /dev/null
+++ clang/test/Driver/darwin-print-runtime-dir.c
@@ -0,0 +1,6 @@
+// Regression test. Previously the output included the OS version number
+// which was not correct.
+// RUN: %clang -print-runtime-dir --target=x86_64-apple-darwin20.3.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+// PRINT-RUNTIME-DIR: lib{{/|\\}}darwin{{$}}
\ No newline at end of file
Index: clang/test/Driver/darwin-print-file-name.c
===
--- /dev/null
+++ clang/test/Driver/darwin-print-file-name.c
@@ -0,0 +1,6 @@
+// Regression test. Previously Clang just returned the library name instead
+// of the full path.
+// RUN: %clang -print-file-name=libclang_rt.osx.a --target=x86_64-apple-darwin20.3.0 \
+// RUN:-resource-dir=%S/Inputs/resource_dir \
+// RUN:  | FileCheck --check-prefix=PRINT-RUNTIME-DIR %s
+// PRINT-RUNTIME-DIR: lib{{/|\\}}darwin{{/|\\}}libclang_rt.osx.a
\ No newline at end of file
Index: clang/lib/Driver/ToolChain.cpp
===
--- clang/lib/Driver/ToolChain.cpp
+++ clang/lib/Driver/ToolChain.cpp
@@ -398,6 +398,8 @@
 return "sunos";
   case llvm::Triple::AIX:
 return "aix";
+  case llvm::Triple::Darwin:
+return "darwin";
   default:
 return getOS();
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101491: [ASan] Rename `-fsanitize-address-destructor-kind=` to drop the `-kind` suffix.

[Dan Liew 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 rG2d42b2ee7baf: [ASan] Rename 
`-fsanitize-address-destructor-kind=` to drop the `-kind` suffix. (authored by 
delcypher).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101491

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/SanitizerArgs.cpp
  clang/test/CodeGen/asan-destructor-kind.cpp
  clang/test/Driver/darwin-asan-mkernel-kext.c
  clang/test/Driver/fsanitize-address-destructor-kind.c
  clang/test/Driver/fsanitize-address-destructor.c

Index: clang/test/Driver/fsanitize-address-destructor.c
===
--- clang/test/Driver/fsanitize-address-destructor.c
+++ clang/test/Driver/fsanitize-address-destructor.c
@@ -2,19 +2,19 @@
 // RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address %s \
 // RUN:   -### 2>&1 | \
 // RUN:   FileCheck %s
-// CHECK-NOT: -fsanitize-address-destructor-kind
+// CHECK-NOT: -fsanitize-address-destructor
 
 // RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address \
-// RUN:   -fsanitize-address-destructor-kind=none %s -### 2>&1 | \
+// RUN:   -fsanitize-address-destructor=none %s -### 2>&1 | \
 // RUN:   FileCheck -check-prefix=CHECK-NONE-ARG %s
-// CHECK-NONE-ARG: "-fsanitize-address-destructor-kind=none"
+// CHECK-NONE-ARG: "-fsanitize-address-destructor=none"
 
 // RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address \
-// RUN:   -fsanitize-address-destructor-kind=global %s -### 2>&1 | \
+// RUN:   -fsanitize-address-destructor=global %s -### 2>&1 | \
 // RUN:   FileCheck -check-prefix=CHECK-GLOBAL-ARG %s
-// CHECK-GLOBAL-ARG: "-fsanitize-address-destructor-kind=global"
+// CHECK-GLOBAL-ARG: "-fsanitize-address-destructor=global"
 
 // RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address \
-// RUN:   -fsanitize-address-destructor-kind=bad_arg %s -### 2>&1 | \
+// RUN:   -fsanitize-address-destructor=bad_arg %s -### 2>&1 | \
 // RUN:   FileCheck -check-prefix=CHECK-INVALID-ARG %s
-// CHECK-INVALID-ARG: error: unsupported argument 'bad_arg' to option 'fsanitize-address-destructor-kind='
+// CHECK-INVALID-ARG: error: unsupported argument 'bad_arg' to option 'fsanitize-address-destructor='
Index: clang/test/Driver/darwin-asan-mkernel-kext.c
===
--- clang/test/Driver/darwin-asan-mkernel-kext.c
+++ clang/test/Driver/darwin-asan-mkernel-kext.c
@@ -5,11 +5,11 @@
 // RUN: %clang -target x86_64-apple-darwin10 -fsanitize=address -fapple-kext \
 // RUN:   -mkernel -### %s 2>&1 | FileCheck %s
 
-// CHECK: "-fsanitize-address-destructor-kind=none"
+// CHECK: "-fsanitize-address-destructor=none"
 
 // Check it's possible to override the driver's decision.
 // RUN: %clang -target x86_64-apple-darwin10 -fsanitize=address -fapple-kext \
-// RUN:   -mkernel -### -fsanitize-address-destructor-kind=global %s 2>&1 | \
+// RUN:   -mkernel -### -fsanitize-address-destructor=global %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=CHECK-OVERRIDE %s
 
-// CHECK-OVERRIDE: "-fsanitize-address-destructor-kind=global"
+// CHECK-OVERRIDE: "-fsanitize-address-destructor=global"
Index: clang/test/CodeGen/asan-destructor-kind.cpp
===
--- clang/test/CodeGen/asan-destructor-kind.cpp
+++ clang/test/CodeGen/asan-destructor-kind.cpp
@@ -1,9 +1,9 @@
 // Frontend rejects invalid option
 // RUN: not %clang_cc1 -fsanitize=address \
-// RUN:   -fsanitize-address-destructor-kind=bad_arg -emit-llvm -o - \
+// RUN:   -fsanitize-address-destructor=bad_arg -emit-llvm -o - \
 // RUN:   -triple x86_64-apple-macosx10.15 %s 2>&1 | \
 // RUN:   FileCheck %s --check-prefixes=CHECK-BAD-ARG
-// CHECK-BAD-ARG: invalid value 'bad_arg' in '-fsanitize-address-destructor-kind=bad_arg'
+// CHECK-BAD-ARG: invalid value 'bad_arg' in '-fsanitize-address-destructor=bad_arg'
 
 // Default is global dtor
 // RUN: %clang_cc1 -fsanitize=address -emit-llvm -o - -triple x86_64-apple-macosx10.15 \
@@ -16,12 +16,12 @@
 
 // Explictly ask for global dtor
 // RUN: %clang_cc1 -fsanitize=address \
-// RUN:   -fsanitize-address-destructor-kind=global -emit-llvm -o - \
+// RUN:   -fsanitize-address-destructor=global -emit-llvm -o - \
 // RUN:   -triple x86_64-apple-macosx10.15 -fno-legacy-pass-manager %s | \
 // RUN:   FileCheck %s --check-prefixes=CHECK-GLOBAL-DTOR
 //
 // RUN: %clang_cc1 -fsanitize=address \
-// RUN:   -fsanitize-address-destructor-kind=global -emit-llvm -o - \
+// RUN:   -fsanitize-address-destructor=global -emit-llvm -o - \
 // RUN:   -triple x86_64-apple-macosx10.15 -flegacy-pass-manager %s | \
 // RUN:   FileCheck %s --check-prefixes=CHECK-GLOBAL-DTOR
 
@@ -30,12 +30,12 @@
 
 // Explictly ask for no dtors
 // 

[PATCH] D101491: [ASan] Rename `-fsanitize-address-destructor-kind=` to drop the `-kind` suffix.

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

@jansvoboda11 I'm going to land this patch as is (with your nit fixed). If you 
would like me to add an alias please follow up with me and I can put up a patch 
to do that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101491

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

[PATCH] D101491: [ASan] Rename `-fsanitize-address-destructor-kind=` to drop the `-kind` suffix.

[Dan Liew via Phabricator via cfe-commits]

delcypher updated this revision to Diff 341596.
delcypher added a comment.

Rename def too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101491

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/SanitizerArgs.cpp
  clang/test/CodeGen/asan-destructor-kind.cpp
  clang/test/Driver/darwin-asan-mkernel-kext.c
  clang/test/Driver/fsanitize-address-destructor-kind.c
  clang/test/Driver/fsanitize-address-destructor.c

Index: clang/test/Driver/fsanitize-address-destructor.c
===
--- clang/test/Driver/fsanitize-address-destructor.c
+++ clang/test/Driver/fsanitize-address-destructor.c
@@ -2,19 +2,19 @@
 // RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address %s \
 // RUN:   -### 2>&1 | \
 // RUN:   FileCheck %s
-// CHECK-NOT: -fsanitize-address-destructor-kind
+// CHECK-NOT: -fsanitize-address-destructor
 
 // RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address \
-// RUN:   -fsanitize-address-destructor-kind=none %s -### 2>&1 | \
+// RUN:   -fsanitize-address-destructor=none %s -### 2>&1 | \
 // RUN:   FileCheck -check-prefix=CHECK-NONE-ARG %s
-// CHECK-NONE-ARG: "-fsanitize-address-destructor-kind=none"
+// CHECK-NONE-ARG: "-fsanitize-address-destructor=none"
 
 // RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address \
-// RUN:   -fsanitize-address-destructor-kind=global %s -### 2>&1 | \
+// RUN:   -fsanitize-address-destructor=global %s -### 2>&1 | \
 // RUN:   FileCheck -check-prefix=CHECK-GLOBAL-ARG %s
-// CHECK-GLOBAL-ARG: "-fsanitize-address-destructor-kind=global"
+// CHECK-GLOBAL-ARG: "-fsanitize-address-destructor=global"
 
 // RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address \
-// RUN:   -fsanitize-address-destructor-kind=bad_arg %s -### 2>&1 | \
+// RUN:   -fsanitize-address-destructor=bad_arg %s -### 2>&1 | \
 // RUN:   FileCheck -check-prefix=CHECK-INVALID-ARG %s
-// CHECK-INVALID-ARG: error: unsupported argument 'bad_arg' to option 'fsanitize-address-destructor-kind='
+// CHECK-INVALID-ARG: error: unsupported argument 'bad_arg' to option 'fsanitize-address-destructor='
Index: clang/test/Driver/darwin-asan-mkernel-kext.c
===
--- clang/test/Driver/darwin-asan-mkernel-kext.c
+++ clang/test/Driver/darwin-asan-mkernel-kext.c
@@ -5,11 +5,11 @@
 // RUN: %clang -target x86_64-apple-darwin10 -fsanitize=address -fapple-kext \
 // RUN:   -mkernel -### %s 2>&1 | FileCheck %s
 
-// CHECK: "-fsanitize-address-destructor-kind=none"
+// CHECK: "-fsanitize-address-destructor=none"
 
 // Check it's possible to override the driver's decision.
 // RUN: %clang -target x86_64-apple-darwin10 -fsanitize=address -fapple-kext \
-// RUN:   -mkernel -### -fsanitize-address-destructor-kind=global %s 2>&1 | \
+// RUN:   -mkernel -### -fsanitize-address-destructor=global %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=CHECK-OVERRIDE %s
 
-// CHECK-OVERRIDE: "-fsanitize-address-destructor-kind=global"
+// CHECK-OVERRIDE: "-fsanitize-address-destructor=global"
Index: clang/test/CodeGen/asan-destructor-kind.cpp
===
--- clang/test/CodeGen/asan-destructor-kind.cpp
+++ clang/test/CodeGen/asan-destructor-kind.cpp
@@ -1,9 +1,9 @@
 // Frontend rejects invalid option
 // RUN: not %clang_cc1 -fsanitize=address \
-// RUN:   -fsanitize-address-destructor-kind=bad_arg -emit-llvm -o - \
+// RUN:   -fsanitize-address-destructor=bad_arg -emit-llvm -o - \
 // RUN:   -triple x86_64-apple-macosx10.15 %s 2>&1 | \
 // RUN:   FileCheck %s --check-prefixes=CHECK-BAD-ARG
-// CHECK-BAD-ARG: invalid value 'bad_arg' in '-fsanitize-address-destructor-kind=bad_arg'
+// CHECK-BAD-ARG: invalid value 'bad_arg' in '-fsanitize-address-destructor=bad_arg'
 
 // Default is global dtor
 // RUN: %clang_cc1 -fsanitize=address -emit-llvm -o - -triple x86_64-apple-macosx10.15 \
@@ -16,12 +16,12 @@
 
 // Explictly ask for global dtor
 // RUN: %clang_cc1 -fsanitize=address \
-// RUN:   -fsanitize-address-destructor-kind=global -emit-llvm -o - \
+// RUN:   -fsanitize-address-destructor=global -emit-llvm -o - \
 // RUN:   -triple x86_64-apple-macosx10.15 -fno-legacy-pass-manager %s | \
 // RUN:   FileCheck %s --check-prefixes=CHECK-GLOBAL-DTOR
 //
 // RUN: %clang_cc1 -fsanitize=address \
-// RUN:   -fsanitize-address-destructor-kind=global -emit-llvm -o - \
+// RUN:   -fsanitize-address-destructor=global -emit-llvm -o - \
 // RUN:   -triple x86_64-apple-macosx10.15 -flegacy-pass-manager %s | \
 // RUN:   FileCheck %s --check-prefixes=CHECK-GLOBAL-DTOR
 
@@ -30,12 +30,12 @@
 
 // Explictly ask for no dtors
 // RUN: %clang_cc1 -fsanitize=address \
-// RUN:   -fsanitize-address-destructor-kind=none -emit-llvm -o - \
+// RUN:   -fsanitize-address-destructor=none -emit-llvm -o - \
 // 

[PATCH] D101491: [ASan] Rename `-fsanitize-address-destructor-kind=` to drop the `-kind` suffix.

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

In D101491#2725348 , @jansvoboda11 
wrote:

> In D101491#2724025 , @delcypher 
> wrote:
>
>> @jansvoboda11 Should I use the the alias feature so that the old 
>> `-fsanitize-address-destructor-kind` argument is still parsed? I'm not sure 
>> if it's worth it but if doing so is very simple and has a low maintenance 
>> burden we should probably do it.
>
> I'm fine with omitting the alias if the original flag didn't make it into a 
> release and it's unlikely that downstream TOT users are using it.

The change that landed this flag originally 
(5d64dd8e3c22e12e4f7b3d08ffe88fc41e727117 
) doesn't 
seem to be in the `release/12.x` branch. It also wasn't cherry picked to the 
downstream Apple branches on GitHub. I can't be sure about other downstream 
users of LLVM but I do think it's very unlikely that anyone adopted this flag.

I don't mind adding an alias but I'd need a little guidance on how to do it 
correctly and also where to add code to emit a warning about the old flag being 
deprecated.




Comment at: clang/docs/ClangCommandLineReference.rst:873
 
-.. option:: -fsanitize-address-destructor-kind=
+.. option:: -fsanitize-address-destructor=
 

vitalybuka wrote:
>  here is inconsistent with MetaVarName<""
I'll fix that now.



Comment at: clang/include/clang/Driver/Options.td:1542
   Group;
 def sanitize_address_destructor_kind_EQ
+: Joined<["-"], "fsanitize-address-destructor=">,

jansvoboda11 wrote:
> Nit: remove `_kind` from the def name.
Good catch. I'll fix that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101491

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

[PATCH] D101491: [ASan] Rename `-fsanitize-address-destructor-kind=` to drop the `-kind` suffix.

[Dan Liew via Phabricator via cfe-commits]

delcypher updated this revision to Diff 341384.
delcypher added a comment.

Remove metavar


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101491

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/SanitizerArgs.cpp
  clang/test/CodeGen/asan-destructor-kind.cpp
  clang/test/Driver/darwin-asan-mkernel-kext.c
  clang/test/Driver/fsanitize-address-destructor-kind.c
  clang/test/Driver/fsanitize-address-destructor.c

Index: clang/test/Driver/fsanitize-address-destructor.c
===
--- clang/test/Driver/fsanitize-address-destructor.c
+++ clang/test/Driver/fsanitize-address-destructor.c
@@ -2,19 +2,19 @@
 // RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address %s \
 // RUN:   -### 2>&1 | \
 // RUN:   FileCheck %s
-// CHECK-NOT: -fsanitize-address-destructor-kind
+// CHECK-NOT: -fsanitize-address-destructor
 
 // RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address \
-// RUN:   -fsanitize-address-destructor-kind=none %s -### 2>&1 | \
+// RUN:   -fsanitize-address-destructor=none %s -### 2>&1 | \
 // RUN:   FileCheck -check-prefix=CHECK-NONE-ARG %s
-// CHECK-NONE-ARG: "-fsanitize-address-destructor-kind=none"
+// CHECK-NONE-ARG: "-fsanitize-address-destructor=none"
 
 // RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address \
-// RUN:   -fsanitize-address-destructor-kind=global %s -### 2>&1 | \
+// RUN:   -fsanitize-address-destructor=global %s -### 2>&1 | \
 // RUN:   FileCheck -check-prefix=CHECK-GLOBAL-ARG %s
-// CHECK-GLOBAL-ARG: "-fsanitize-address-destructor-kind=global"
+// CHECK-GLOBAL-ARG: "-fsanitize-address-destructor=global"
 
 // RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address \
-// RUN:   -fsanitize-address-destructor-kind=bad_arg %s -### 2>&1 | \
+// RUN:   -fsanitize-address-destructor=bad_arg %s -### 2>&1 | \
 // RUN:   FileCheck -check-prefix=CHECK-INVALID-ARG %s
-// CHECK-INVALID-ARG: error: unsupported argument 'bad_arg' to option 'fsanitize-address-destructor-kind='
+// CHECK-INVALID-ARG: error: unsupported argument 'bad_arg' to option 'fsanitize-address-destructor='
Index: clang/test/Driver/darwin-asan-mkernel-kext.c
===
--- clang/test/Driver/darwin-asan-mkernel-kext.c
+++ clang/test/Driver/darwin-asan-mkernel-kext.c
@@ -5,11 +5,11 @@
 // RUN: %clang -target x86_64-apple-darwin10 -fsanitize=address -fapple-kext \
 // RUN:   -mkernel -### %s 2>&1 | FileCheck %s
 
-// CHECK: "-fsanitize-address-destructor-kind=none"
+// CHECK: "-fsanitize-address-destructor=none"
 
 // Check it's possible to override the driver's decision.
 // RUN: %clang -target x86_64-apple-darwin10 -fsanitize=address -fapple-kext \
-// RUN:   -mkernel -### -fsanitize-address-destructor-kind=global %s 2>&1 | \
+// RUN:   -mkernel -### -fsanitize-address-destructor=global %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=CHECK-OVERRIDE %s
 
-// CHECK-OVERRIDE: "-fsanitize-address-destructor-kind=global"
+// CHECK-OVERRIDE: "-fsanitize-address-destructor=global"
Index: clang/test/CodeGen/asan-destructor-kind.cpp
===
--- clang/test/CodeGen/asan-destructor-kind.cpp
+++ clang/test/CodeGen/asan-destructor-kind.cpp
@@ -1,9 +1,9 @@
 // Frontend rejects invalid option
 // RUN: not %clang_cc1 -fsanitize=address \
-// RUN:   -fsanitize-address-destructor-kind=bad_arg -emit-llvm -o - \
+// RUN:   -fsanitize-address-destructor=bad_arg -emit-llvm -o - \
 // RUN:   -triple x86_64-apple-macosx10.15 %s 2>&1 | \
 // RUN:   FileCheck %s --check-prefixes=CHECK-BAD-ARG
-// CHECK-BAD-ARG: invalid value 'bad_arg' in '-fsanitize-address-destructor-kind=bad_arg'
+// CHECK-BAD-ARG: invalid value 'bad_arg' in '-fsanitize-address-destructor=bad_arg'
 
 // Default is global dtor
 // RUN: %clang_cc1 -fsanitize=address -emit-llvm -o - -triple x86_64-apple-macosx10.15 \
@@ -16,12 +16,12 @@
 
 // Explictly ask for global dtor
 // RUN: %clang_cc1 -fsanitize=address \
-// RUN:   -fsanitize-address-destructor-kind=global -emit-llvm -o - \
+// RUN:   -fsanitize-address-destructor=global -emit-llvm -o - \
 // RUN:   -triple x86_64-apple-macosx10.15 -fno-legacy-pass-manager %s | \
 // RUN:   FileCheck %s --check-prefixes=CHECK-GLOBAL-DTOR
 //
 // RUN: %clang_cc1 -fsanitize=address \
-// RUN:   -fsanitize-address-destructor-kind=global -emit-llvm -o - \
+// RUN:   -fsanitize-address-destructor=global -emit-llvm -o - \
 // RUN:   -triple x86_64-apple-macosx10.15 -flegacy-pass-manager %s | \
 // RUN:   FileCheck %s --check-prefixes=CHECK-GLOBAL-DTOR
 
@@ -30,12 +30,12 @@
 
 // Explictly ask for no dtors
 // RUN: %clang_cc1 -fsanitize=address \
-// RUN:   -fsanitize-address-destructor-kind=none -emit-llvm -o - \
+// RUN:   -fsanitize-address-destructor=none -emit-llvm -o - \
 // 

[PATCH] D101490: [NFC] Rename SanitizeAddressDtorKind codegen opt to not have `Kind` suffix.

[Dan Liew 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 rG1bbbcff99de8: [NFC] Rename SanitizeAddressDtorKind codegen 
opt to not have `Kind` suffix. (authored by delcypher).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101490

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp


Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -287,7 +287,7 @@
   bool UseAfterScope = CGOpts.SanitizeAddressUseAfterScope;
   bool UseOdrIndicator = CGOpts.SanitizeAddressUseOdrIndicator;
   bool UseGlobalsGC = asanUseGlobalsGC(T, CGOpts);
-  llvm::AsanDtorKind DestructorKind = CGOpts.getSanitizeAddressDtorKind();
+  llvm::AsanDtorKind DestructorKind = CGOpts.getSanitizeAddressDtor();
   PM.add(createAddressSanitizerFunctionPass(/*CompileKernel*/ false, Recover,
 UseAfterScope));
   PM.add(createModuleAddressSanitizerLegacyPassPass(
@@ -1150,7 +1150,7 @@
 bool ModuleUseAfterScope = asanUseGlobalsGC(TargetTriple, CodeGenOpts);
 bool UseOdrIndicator = CodeGenOpts.SanitizeAddressUseOdrIndicator;
 llvm::AsanDtorKind DestructorKind =
-CodeGenOpts.getSanitizeAddressDtorKind();
+CodeGenOpts.getSanitizeAddressDtor();
 MPM.addPass(RequireAnalysisPass());
 MPM.addPass(ModuleAddressSanitizerPass(
 CompileKernel, Recover, ModuleUseAfterScope, UseOdrIndicator,
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1548,7 +1548,7 @@
   Values<"none,global">,
   NormalizedValuesScope<"llvm::AsanDtorKind">,
   NormalizedValues<["None", "Global"]>,
-  MarshallingInfoEnum, "Global">;
+  MarshallingInfoEnum, "Global">;
 // Note: This flag was introduced when it was necessary to distinguish between
 //   ABI for correct codegen.  This is no longer needed, but the flag is
 //   not removed since targeting either ABI will behave the same.
Index: clang/include/clang/Basic/CodeGenOptions.def
===
--- clang/include/clang/Basic/CodeGenOptions.def
+++ clang/include/clang/Basic/CodeGenOptions.def
@@ -219,7 +219,7 @@
 CODEGENOPT(SanitizeAddressUseOdrIndicator, 1, 0) ///< Enable ODR indicator 
globals
 CODEGENOPT(SanitizeMemoryTrackOrigins, 2, 0) ///< Enable tracking origins in
  ///< MemorySanitizer
-ENUM_CODEGENOPT(SanitizeAddressDtorKind, llvm::AsanDtorKind, 2,
+ENUM_CODEGENOPT(SanitizeAddressDtor, llvm::AsanDtorKind, 2,
 llvm::AsanDtorKind::Global)  ///< Set how ASan global
  ///< destructors are emitted.
 CODEGENOPT(SanitizeMemoryUseAfterDtor, 1, 0) ///< Enable use-after-delete 
detection


Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -287,7 +287,7 @@
   bool UseAfterScope = CGOpts.SanitizeAddressUseAfterScope;
   bool UseOdrIndicator = CGOpts.SanitizeAddressUseOdrIndicator;
   bool UseGlobalsGC = asanUseGlobalsGC(T, CGOpts);
-  llvm::AsanDtorKind DestructorKind = CGOpts.getSanitizeAddressDtorKind();
+  llvm::AsanDtorKind DestructorKind = CGOpts.getSanitizeAddressDtor();
   PM.add(createAddressSanitizerFunctionPass(/*CompileKernel*/ false, Recover,
 UseAfterScope));
   PM.add(createModuleAddressSanitizerLegacyPassPass(
@@ -1150,7 +1150,7 @@
 bool ModuleUseAfterScope = asanUseGlobalsGC(TargetTriple, CodeGenOpts);
 bool UseOdrIndicator = CodeGenOpts.SanitizeAddressUseOdrIndicator;
 llvm::AsanDtorKind DestructorKind =
-CodeGenOpts.getSanitizeAddressDtorKind();
+CodeGenOpts.getSanitizeAddressDtor();
 MPM.addPass(RequireAnalysisPass());
 MPM.addPass(ModuleAddressSanitizerPass(
 CompileKernel, Recover, ModuleUseAfterScope, UseOdrIndicator,
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1548,7 +1548,7 @@
   Values<"none,global">,
   NormalizedValuesScope<"llvm::AsanDtorKind">,
   NormalizedValues<["None", "Global"]>,
-  MarshallingInfoEnum, "Global">;
+  MarshallingInfoEnum, "Global">;
 // Note: This flag was introduced when it was necessary to distinguish between
 //   ABI for 

[PATCH] D101491: [ASan] Rename `-fsanitize-address-destructor-kind=` to drop the `-kind` suffix.

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

@jansvoboda11 Should I use the the alias feature so that the old 
`-fsanitize-address-destructor-kind` argument is still parsed? I'm not sure if 
it's worth it but if doing so is very simple and has a low maintenance burden 
we should probably do it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101491

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

[PATCH] D101122: introduce flag -fsanitize-address-detect-stack-use-after-return-mode. No functional change.

[Dan Liew via Phabricator via cfe-commits]

delcypher added inline comments.



Comment at: clang/include/clang/Basic/CodeGenOptions.def:225-228
+ENUM_CODEGENOPT(SanitizeAddressDetectStackUseAfterReturnMode,
+llvm::AsanDetectStackUseAfterReturnMode, 2,
+llvm::AsanDetectStackUseAfterReturnMode::Runtime
+) ///< Set detection mode for stack-use-after-return.

vitalybuka wrote:
> delcypher wrote:
> > vitalybuka wrote:
> > > vitalybuka wrote:
> > > > Mode->Kind to be consistent with the file
> > > I believe for consistency we need:
> > > 1. Var name (SanitizeAddressDetectStackUseAfterReturn) no Mode/Kind suffix
> > > 2. Enum type AsanDetectStackUseAfterReturnKind uses "kind"
> > > 3. Actual flag includes no "kind" 
> > > -fsanitize-address-detect-stack-use-after-return=
> > > 
> > > BTW. @delcypher  -fsanitize-address-destructor-kind was not consistent 
> > > with the rest of file, but I am not sure it's worth of fixing now.
> > > 
> > > 
> > @vitalybuka It's not too late to fix this because the 
> > `-fsanitize-address-destructor-kind=` flag hasn't been adopted yet AFAIK.
> > 
> > Am I right in understanding that what I landed doesn't match points 1 and 3 
> > above? If you can confirm that's what you'd like to be changed I can put up 
> > some patches to fix this.
> Correct
> I'd expect 
> 
> ```
> ENUM_CODEGENOPT(SanitizeAddressDtor, llvm::AsanDtorKind, 2,
> llvm::AsanDtorKind::Global)
> ```
> 
> and 
> 
> ```
> def sanitize_address_destructor_EQ
> : Joined<["-"], "fsanitize-address-destructor=">,
>   MetaVarName<"">,
>   Flags<[CC1Option]>,
>   HelpText<"Set destructor type used in ASan instrumentation">,
>   Group,
>   Values<"none,global">,
>   NormalizedValuesScope<"llvm::AsanDtorKind">,
>   NormalizedValues<["None", "Global"]>,
>   MarshallingInfoEnum, "Global">;
> ```
> 
> And one more thing, probably not very important, and matter of personal 
> preferences.
> Maybe we don't need MetaVarName<""> for both of these flags.
> Without MetaVarName documentation is going to be 
> -fsanitize-address-destructor= 
> MetaVarName just lets use to replace  with more common and specific 
> , .. etc.
> To me  is almost generic as , so why not to stick to default.
@vitalybuka I put up the following patches for review

https://reviews.llvm.org/D101490
https://reviews.llvm.org/D101491


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101122

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

[PATCH] D101491: [ASan] Rename `-fsanitize-address-destructor-kind=` to drop the `-kind` suffix.

[Dan Liew via Phabricator via cfe-commits]

delcypher created this revision.
delcypher added reviewers: vitalybuka, jansvoboda11.
Herald added a subscriber: dang.
delcypher requested review of this revision.
Herald added a project: clang.

Renaming the option is based on discussions in https://reviews.llvm.org/D101122.

It is normally not a good idea to rename driver flags but this flag is
new enough and obscure enough that it is very unlikely to have adopters.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D101491

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/SanitizerArgs.cpp
  clang/test/CodeGen/asan-destructor-kind.cpp
  clang/test/Driver/darwin-asan-mkernel-kext.c
  clang/test/Driver/fsanitize-address-destructor-kind.c
  clang/test/Driver/fsanitize-address-destructor.c

Index: clang/test/Driver/fsanitize-address-destructor.c
===
--- clang/test/Driver/fsanitize-address-destructor.c
+++ clang/test/Driver/fsanitize-address-destructor.c
@@ -2,19 +2,19 @@
 // RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address %s \
 // RUN:   -### 2>&1 | \
 // RUN:   FileCheck %s
-// CHECK-NOT: -fsanitize-address-destructor-kind
+// CHECK-NOT: -fsanitize-address-destructor
 
 // RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address \
-// RUN:   -fsanitize-address-destructor-kind=none %s -### 2>&1 | \
+// RUN:   -fsanitize-address-destructor=none %s -### 2>&1 | \
 // RUN:   FileCheck -check-prefix=CHECK-NONE-ARG %s
-// CHECK-NONE-ARG: "-fsanitize-address-destructor-kind=none"
+// CHECK-NONE-ARG: "-fsanitize-address-destructor=none"
 
 // RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address \
-// RUN:   -fsanitize-address-destructor-kind=global %s -### 2>&1 | \
+// RUN:   -fsanitize-address-destructor=global %s -### 2>&1 | \
 // RUN:   FileCheck -check-prefix=CHECK-GLOBAL-ARG %s
-// CHECK-GLOBAL-ARG: "-fsanitize-address-destructor-kind=global"
+// CHECK-GLOBAL-ARG: "-fsanitize-address-destructor=global"
 
 // RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address \
-// RUN:   -fsanitize-address-destructor-kind=bad_arg %s -### 2>&1 | \
+// RUN:   -fsanitize-address-destructor=bad_arg %s -### 2>&1 | \
 // RUN:   FileCheck -check-prefix=CHECK-INVALID-ARG %s
-// CHECK-INVALID-ARG: error: unsupported argument 'bad_arg' to option 'fsanitize-address-destructor-kind='
+// CHECK-INVALID-ARG: error: unsupported argument 'bad_arg' to option 'fsanitize-address-destructor='
Index: clang/test/Driver/darwin-asan-mkernel-kext.c
===
--- clang/test/Driver/darwin-asan-mkernel-kext.c
+++ clang/test/Driver/darwin-asan-mkernel-kext.c
@@ -5,11 +5,11 @@
 // RUN: %clang -target x86_64-apple-darwin10 -fsanitize=address -fapple-kext \
 // RUN:   -mkernel -### %s 2>&1 | FileCheck %s
 
-// CHECK: "-fsanitize-address-destructor-kind=none"
+// CHECK: "-fsanitize-address-destructor=none"
 
 // Check it's possible to override the driver's decision.
 // RUN: %clang -target x86_64-apple-darwin10 -fsanitize=address -fapple-kext \
-// RUN:   -mkernel -### -fsanitize-address-destructor-kind=global %s 2>&1 | \
+// RUN:   -mkernel -### -fsanitize-address-destructor=global %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=CHECK-OVERRIDE %s
 
-// CHECK-OVERRIDE: "-fsanitize-address-destructor-kind=global"
+// CHECK-OVERRIDE: "-fsanitize-address-destructor=global"
Index: clang/test/CodeGen/asan-destructor-kind.cpp
===
--- clang/test/CodeGen/asan-destructor-kind.cpp
+++ clang/test/CodeGen/asan-destructor-kind.cpp
@@ -1,9 +1,9 @@
 // Frontend rejects invalid option
 // RUN: not %clang_cc1 -fsanitize=address \
-// RUN:   -fsanitize-address-destructor-kind=bad_arg -emit-llvm -o - \
+// RUN:   -fsanitize-address-destructor=bad_arg -emit-llvm -o - \
 // RUN:   -triple x86_64-apple-macosx10.15 %s 2>&1 | \
 // RUN:   FileCheck %s --check-prefixes=CHECK-BAD-ARG
-// CHECK-BAD-ARG: invalid value 'bad_arg' in '-fsanitize-address-destructor-kind=bad_arg'
+// CHECK-BAD-ARG: invalid value 'bad_arg' in '-fsanitize-address-destructor=bad_arg'
 
 // Default is global dtor
 // RUN: %clang_cc1 -fsanitize=address -emit-llvm -o - -triple x86_64-apple-macosx10.15 \
@@ -16,12 +16,12 @@
 
 // Explictly ask for global dtor
 // RUN: %clang_cc1 -fsanitize=address \
-// RUN:   -fsanitize-address-destructor-kind=global -emit-llvm -o - \
+// RUN:   -fsanitize-address-destructor=global -emit-llvm -o - \
 // RUN:   -triple x86_64-apple-macosx10.15 -fno-legacy-pass-manager %s | \
 // RUN:   FileCheck %s --check-prefixes=CHECK-GLOBAL-DTOR
 //
 // RUN: %clang_cc1 -fsanitize=address \
-// RUN:   -fsanitize-address-destructor-kind=global -emit-llvm -o - \
+// RUN:   -fsanitize-address-destructor=global -emit-llvm -o - \
 // RUN:   -triple x86_64-apple-macosx10.15 -flegacy-pass-manager %s | \
 // RUN:   FileCheck %s 

[PATCH] D101490: [NFC] Rename SanitizeAddressDtorKind codegen opt to not have `Kind` suffix.

[Dan Liew via Phabricator via cfe-commits]

delcypher created this revision.
delcypher added a reviewer: vitalybuka.
Herald added subscribers: dexonsmith, dang.
delcypher requested review of this revision.
Herald added a project: clang.

This is post commit follow up based on discussions in
https://reviews.llvm.org/D101122.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D101490

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp


Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -287,7 +287,7 @@
   bool UseAfterScope = CGOpts.SanitizeAddressUseAfterScope;
   bool UseOdrIndicator = CGOpts.SanitizeAddressUseOdrIndicator;
   bool UseGlobalsGC = asanUseGlobalsGC(T, CGOpts);
-  llvm::AsanDtorKind DestructorKind = CGOpts.getSanitizeAddressDtorKind();
+  llvm::AsanDtorKind DestructorKind = CGOpts.getSanitizeAddressDtor();
   PM.add(createAddressSanitizerFunctionPass(/*CompileKernel*/ false, Recover,
 UseAfterScope));
   PM.add(createModuleAddressSanitizerLegacyPassPass(
@@ -1150,7 +1150,7 @@
 bool ModuleUseAfterScope = asanUseGlobalsGC(TargetTriple, CodeGenOpts);
 bool UseOdrIndicator = CodeGenOpts.SanitizeAddressUseOdrIndicator;
 llvm::AsanDtorKind DestructorKind =
-CodeGenOpts.getSanitizeAddressDtorKind();
+CodeGenOpts.getSanitizeAddressDtor();
 MPM.addPass(RequireAnalysisPass());
 MPM.addPass(ModuleAddressSanitizerPass(
 CompileKernel, Recover, ModuleUseAfterScope, UseOdrIndicator,
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1548,7 +1548,7 @@
   Values<"none,global">,
   NormalizedValuesScope<"llvm::AsanDtorKind">,
   NormalizedValues<["None", "Global"]>,
-  MarshallingInfoEnum, "Global">;
+  MarshallingInfoEnum, "Global">;
 // Note: This flag was introduced when it was necessary to distinguish between
 //   ABI for correct codegen.  This is no longer needed, but the flag is
 //   not removed since targeting either ABI will behave the same.
Index: clang/include/clang/Basic/CodeGenOptions.def
===
--- clang/include/clang/Basic/CodeGenOptions.def
+++ clang/include/clang/Basic/CodeGenOptions.def
@@ -219,7 +219,7 @@
 CODEGENOPT(SanitizeAddressUseOdrIndicator, 1, 0) ///< Enable ODR indicator 
globals
 CODEGENOPT(SanitizeMemoryTrackOrigins, 2, 0) ///< Enable tracking origins in
  ///< MemorySanitizer
-ENUM_CODEGENOPT(SanitizeAddressDtorKind, llvm::AsanDtorKind, 2,
+ENUM_CODEGENOPT(SanitizeAddressDtor, llvm::AsanDtorKind, 2,
 llvm::AsanDtorKind::Global)  ///< Set how ASan global
  ///< destructors are emitted.
 CODEGENOPT(SanitizeMemoryUseAfterDtor, 1, 0) ///< Enable use-after-delete 
detection


Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -287,7 +287,7 @@
   bool UseAfterScope = CGOpts.SanitizeAddressUseAfterScope;
   bool UseOdrIndicator = CGOpts.SanitizeAddressUseOdrIndicator;
   bool UseGlobalsGC = asanUseGlobalsGC(T, CGOpts);
-  llvm::AsanDtorKind DestructorKind = CGOpts.getSanitizeAddressDtorKind();
+  llvm::AsanDtorKind DestructorKind = CGOpts.getSanitizeAddressDtor();
   PM.add(createAddressSanitizerFunctionPass(/*CompileKernel*/ false, Recover,
 UseAfterScope));
   PM.add(createModuleAddressSanitizerLegacyPassPass(
@@ -1150,7 +1150,7 @@
 bool ModuleUseAfterScope = asanUseGlobalsGC(TargetTriple, CodeGenOpts);
 bool UseOdrIndicator = CodeGenOpts.SanitizeAddressUseOdrIndicator;
 llvm::AsanDtorKind DestructorKind =
-CodeGenOpts.getSanitizeAddressDtorKind();
+CodeGenOpts.getSanitizeAddressDtor();
 MPM.addPass(RequireAnalysisPass());
 MPM.addPass(ModuleAddressSanitizerPass(
 CompileKernel, Recover, ModuleUseAfterScope, UseOdrIndicator,
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1548,7 +1548,7 @@
   Values<"none,global">,
   NormalizedValuesScope<"llvm::AsanDtorKind">,
   NormalizedValues<["None", "Global"]>,
-  MarshallingInfoEnum, "Global">;
+  MarshallingInfoEnum, "Global">;
 // Note: This flag was introduced when it was necessary to distinguish between
 //   ABI for correct codegen.  This is no longer needed, but the 

[PATCH] D101122: introduce flag -fsanitize-address-detect-stack-use-after-return-mode. No functional change.

[Dan Liew via Phabricator via cfe-commits]

delcypher added inline comments.



Comment at: clang/include/clang/Basic/CodeGenOptions.def:225-228
+ENUM_CODEGENOPT(SanitizeAddressDetectStackUseAfterReturnMode,
+llvm::AsanDetectStackUseAfterReturnMode, 2,
+llvm::AsanDetectStackUseAfterReturnMode::Runtime
+) ///< Set detection mode for stack-use-after-return.

vitalybuka wrote:
> vitalybuka wrote:
> > Mode->Kind to be consistent with the file
> I believe for consistency we need:
> 1. Var name (SanitizeAddressDetectStackUseAfterReturn) no Mode/Kind suffix
> 2. Enum type AsanDetectStackUseAfterReturnKind uses "kind"
> 3. Actual flag includes no "kind" 
> -fsanitize-address-detect-stack-use-after-return=
> 
> BTW. @delcypher  -fsanitize-address-destructor-kind was not consistent with 
> the rest of file, but I am not sure it's worth of fixing now.
> 
> 
@vitalybuka It's not too late to fix this because the 
`-fsanitize-address-destructor-kind=` flag hasn't been adopted yet AFAIK.

Am I right in understanding that what I landed doesn't match points 1 and 3 
above? If you can confirm that's what you'd like to be changed I can put up 
some patches to fix this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101122

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

[PATCH] D100405: Ship `llvm-cxxfilt` in the toolchain.

[Dan Liew 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 rG4c0bc69490a5: Ship `llvm-cxxfilt` in the toolchain. 
(authored by delcypher).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100405

Files:
  clang/cmake/caches/Apple-stage2.cmake


Index: clang/cmake/caches/Apple-stage2.cmake
===
--- clang/cmake/caches/Apple-stage2.cmake
+++ clang/cmake/caches/Apple-stage2.cmake
@@ -56,6 +56,7 @@
   llvm-objdump
   llvm-nm
   llvm-size
+  llvm-cxxfilt
   llvm-config
   CACHE STRING "")
 


Index: clang/cmake/caches/Apple-stage2.cmake
===
--- clang/cmake/caches/Apple-stage2.cmake
+++ clang/cmake/caches/Apple-stage2.cmake
@@ -56,6 +56,7 @@
   llvm-objdump
   llvm-nm
   llvm-size
+  llvm-cxxfilt
   llvm-config
   CACHE STRING "")
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100405: Ship `llvm-cxxfilt` in the toolchain.

[Dan Liew via Phabricator via cfe-commits]

delcypher created this revision.
delcypher added reviewers: steven_wu, arphaman, dexonsmith.
Herald added subscribers: yaxunl, mgorny.
delcypher requested review of this revision.
Herald added a project: clang.

Originally done for rdar://problem/57155465.

rdar://76602859


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D100405

Files:
  clang/cmake/caches/Apple-stage2.cmake


Index: clang/cmake/caches/Apple-stage2.cmake
===
--- clang/cmake/caches/Apple-stage2.cmake
+++ clang/cmake/caches/Apple-stage2.cmake
@@ -56,6 +56,7 @@
   llvm-objdump
   llvm-nm
   llvm-size
+  llvm-cxxfilt
   llvm-config
   CACHE STRING "")
 


Index: clang/cmake/caches/Apple-stage2.cmake
===
--- clang/cmake/caches/Apple-stage2.cmake
+++ clang/cmake/caches/Apple-stage2.cmake
@@ -56,6 +56,7 @@
   llvm-objdump
   llvm-nm
   llvm-size
+  llvm-cxxfilt
   llvm-config
   CACHE STRING "")
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100087: Include `count` in AppleClang toolchains.

[Dan Liew via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf66e05a720f7: Include `count` in AppleClang toolchains. 
(authored by delcypher).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100087

Files:
  clang/cmake/caches/Apple-stage2.cmake


Index: clang/cmake/caches/Apple-stage2.cmake
===
--- clang/cmake/caches/Apple-stage2.cmake
+++ clang/cmake/caches/Apple-stage2.cmake
@@ -65,6 +65,7 @@
   FileCheck
   yaml2obj
   not
+  count
   CACHE STRING "")
 
 set(LLVM_DISTRIBUTION_COMPONENTS


Index: clang/cmake/caches/Apple-stage2.cmake
===
--- clang/cmake/caches/Apple-stage2.cmake
+++ clang/cmake/caches/Apple-stage2.cmake
@@ -65,6 +65,7 @@
   FileCheck
   yaml2obj
   not
+  count
   CACHE STRING "")
 
 set(LLVM_DISTRIBUTION_COMPONENTS
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100087: Include `count` in AppleClang toolchains.

[Dan Liew via Phabricator via cfe-commits]

delcypher created this revision.
delcypher added reviewers: arphaman, JDevlieghere.
Herald added a subscriber: mgorny.
delcypher requested review of this revision.
Herald added a project: clang.

The motivation here is so we can run the compiler-rt tests
from a standalone build against AppleClang.

In particular the `Posix/halt_on_error-torture.cpp` and
`Posix/halt_on_error_suppress_equal_pcs.cpp` ASan test cases currently
require this binary for the tests to pass.

rdar://76366784


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D100087

Files:
  clang/cmake/caches/Apple-stage2.cmake


Index: clang/cmake/caches/Apple-stage2.cmake
===
--- clang/cmake/caches/Apple-stage2.cmake
+++ clang/cmake/caches/Apple-stage2.cmake
@@ -65,6 +65,7 @@
   FileCheck
   yaml2obj
   not
+  count
   CACHE STRING "")
 
 set(LLVM_DISTRIBUTION_COMPONENTS


Index: clang/cmake/caches/Apple-stage2.cmake
===
--- clang/cmake/caches/Apple-stage2.cmake
+++ clang/cmake/caches/Apple-stage2.cmake
@@ -65,6 +65,7 @@
   FileCheck
   yaml2obj
   not
+  count
   CACHE STRING "")
 
 set(LLVM_DISTRIBUTION_COMPONENTS
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100086: Include `llvm-config` and `not` in AppleClang toolchains.

[Dan Liew 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 rG73cbc7f60ed9: Include `llvm-config` and `not` in AppleClang 
toolchains. (authored by delcypher).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100086

Files:
  clang/cmake/caches/Apple-stage2.cmake


Index: clang/cmake/caches/Apple-stage2.cmake
===
--- clang/cmake/caches/Apple-stage2.cmake
+++ clang/cmake/caches/Apple-stage2.cmake
@@ -56,6 +56,7 @@
   llvm-objdump
   llvm-nm
   llvm-size
+  llvm-config
   CACHE STRING "")
 
 set(LLVM_BUILD_UTILS ON CACHE BOOL "")
@@ -63,6 +64,7 @@
 set(LLVM_TOOLCHAIN_UTILITIES
   FileCheck
   yaml2obj
+  not
   CACHE STRING "")
 
 set(LLVM_DISTRIBUTION_COMPONENTS


Index: clang/cmake/caches/Apple-stage2.cmake
===
--- clang/cmake/caches/Apple-stage2.cmake
+++ clang/cmake/caches/Apple-stage2.cmake
@@ -56,6 +56,7 @@
   llvm-objdump
   llvm-nm
   llvm-size
+  llvm-config
   CACHE STRING "")
 
 set(LLVM_BUILD_UTILS ON CACHE BOOL "")
@@ -63,6 +64,7 @@
 set(LLVM_TOOLCHAIN_UTILITIES
   FileCheck
   yaml2obj
+  not
   CACHE STRING "")
 
 set(LLVM_DISTRIBUTION_COMPONENTS
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100086: Include `llvm-config` and `not` in AppleClang toolchains.

[Dan Liew via Phabricator via cfe-commits]

delcypher created this revision.
delcypher added reviewers: JDevlieghere, arphaman.
Herald added a subscriber: mgorny.
delcypher requested review of this revision.
Herald added a project: clang.

The motivation here is so that we can configure and run compiler-rt
tests from a standalone build against AppleClang.

rdar://75975846


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D100086

Files:
  clang/cmake/caches/Apple-stage2.cmake


Index: clang/cmake/caches/Apple-stage2.cmake
===
--- clang/cmake/caches/Apple-stage2.cmake
+++ clang/cmake/caches/Apple-stage2.cmake
@@ -56,6 +56,7 @@
   llvm-objdump
   llvm-nm
   llvm-size
+  llvm-config
   CACHE STRING "")
 
 set(LLVM_BUILD_UTILS ON CACHE BOOL "")
@@ -63,6 +64,7 @@
 set(LLVM_TOOLCHAIN_UTILITIES
   FileCheck
   yaml2obj
+  not
   CACHE STRING "")
 
 set(LLVM_DISTRIBUTION_COMPONENTS


Index: clang/cmake/caches/Apple-stage2.cmake
===
--- clang/cmake/caches/Apple-stage2.cmake
+++ clang/cmake/caches/Apple-stage2.cmake
@@ -56,6 +56,7 @@
   llvm-objdump
   llvm-nm
   llvm-size
+  llvm-config
   CACHE STRING "")
 
 set(LLVM_BUILD_UTILS ON CACHE BOOL "")
@@ -63,6 +64,7 @@
 set(LLVM_TOOLCHAIN_UTILITIES
   FileCheck
   yaml2obj
+  not
   CACHE STRING "")
 
 set(LLVM_DISTRIBUTION_COMPONENTS
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98291: [compiler-rt] Fix stale incremental builds when using `LLVM_BUILD_EXTERNAL_COMPILER_RT=ON`.

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

In D98291#2616865 , @kubamracek wrote:

> Looks good!
>
> By the way, Apple Clang releases also build compiler-rt this way, so it would 
> be worth checking with @arphaman that he's okay with the change.

@kubamracek @arphaman Ah sorry I landed the change before I saw this message. 
It looks like the `LLVM_BUILD_EXTERNAL_COMPILER_RT` option is used as part of 
the 2-stage boot strap for AppleClang as it's set in 
`clang/cmake/caches/Apple-stage2.cmake`. I'll follow up offline.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98291

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

[PATCH] D98291: [compiler-rt] Fix stale incremental builds when using `LLVM_BUILD_EXTERNAL_COMPILER_RT=ON`.

[Dan Liew 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 rGa159f91c8d06: [compiler-rt] Fix stale incremental builds 
when using… (authored by delcypher).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98291

Files:
  clang/runtime/CMakeLists.txt


Index: clang/runtime/CMakeLists.txt
===
--- clang/runtime/CMakeLists.txt
+++ clang/runtime/CMakeLists.txt
@@ -95,6 +95,8 @@
 USES_TERMINAL_CONFIGURE 1
 USES_TERMINAL_BUILD 1
 USES_TERMINAL_INSTALL 1
+# Always run the build command so that incremental builds are correct.
+BUILD_ALWAYS 1
 )
 
   get_ext_project_build_command(run_clean_compiler_rt clean)


Index: clang/runtime/CMakeLists.txt
===
--- clang/runtime/CMakeLists.txt
+++ clang/runtime/CMakeLists.txt
@@ -95,6 +95,8 @@
 USES_TERMINAL_CONFIGURE 1
 USES_TERMINAL_BUILD 1
 USES_TERMINAL_INSTALL 1
+# Always run the build command so that incremental builds are correct.
+BUILD_ALWAYS 1
 )
 
   get_ext_project_build_command(run_clean_compiler_rt clean)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98291: [compiler-rt] Fix stale incremental builds when using `LLVM_BUILD_EXTERNAL_COMPILER_RT=ON`.

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

In D98291#2615593 , @beanz wrote:

> So... this patch is fine. (marked as approved). That said, we should really 
> be working to remove the `LLVM_BUILD_EXTERNAL_COMPILER_RT` option entirely in 
> favor of just using `LLVM_ENABLE_RUNTIMES=compiler-rt`. That build process 
> does support invoking the sub-project build on build invocation to make sure 
> builds get re-run, and it supports better understanding of the dependencies 
> between runtimes.
>
> Have you considered using that instead of `LLVM_BUILD_EXTERNAL_COMPILER_RT`?

@beanz It's something I've thought about but haven't had the time to do. The 
fact that we have 3 different ways to build compiler-rt in LLVM makes me sad. 
There should only be 1. I don't have time to do it now but I've filed a radar 
to do this work at some point (rdar://75248447).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98291

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

[PATCH] D98291: [compiler-rt] Fix stale incremental builds when using `LLVM_BUILD_EXTERNAL_COMPILER_RT=ON`.

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

In D98291#2615590 , @kubamracek wrote:

> When using Ninja, does this mean running a null build is no longer a null 
> build, but it at least always invokes this sub-build? Not saying that's bad, 
> just want to understand the change. What does the output of just "ninja" say 
> in that case?

Yes the sub-build will always be invoked. That being said in the sub-build if 
nothing has changed it will be extremely fast because ninja is fast.

The output looks something like this

  $ ninja
  [0/3] Performing build step for 'compiler-rt'
  ninja: no work to do.
  [1/3] No install step for 'compiler-rt'
  [3/3] Completed 'compiler-rt'

This being said I'd expect very few people to actually run into this because 
this build configuration (`LLVM_BUILD_EXTERNAL_COMPILER_RT=ON`) is not common 
because it is not the default. The default if you enable compiler-rt is for it 
to be built as part of the main build using the host compiler, which is all 
kinds of wrong but is very convenient.

Swift does use this build configuration but I doubt anyone would complain about 
the extra console noise given how noisy build-script is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98291

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

[PATCH] D98291: [compiler-rt] Fix stale incremental builds when using `LLVM_BUILD_EXTERNAL_COMPILER_RT=ON`.

[Dan Liew via Phabricator via cfe-commits]

delcypher updated this revision to Diff 329469.
delcypher added a comment.

- update description


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98291

Files:
  clang/runtime/CMakeLists.txt


Index: clang/runtime/CMakeLists.txt
===
--- clang/runtime/CMakeLists.txt
+++ clang/runtime/CMakeLists.txt
@@ -95,6 +95,8 @@
 USES_TERMINAL_CONFIGURE 1
 USES_TERMINAL_BUILD 1
 USES_TERMINAL_INSTALL 1
+# Always run the build command so that incremental builds are correct.
+BUILD_ALWAYS 1
 )
 
   get_ext_project_build_command(run_clean_compiler_rt clean)


Index: clang/runtime/CMakeLists.txt
===
--- clang/runtime/CMakeLists.txt
+++ clang/runtime/CMakeLists.txt
@@ -95,6 +95,8 @@
 USES_TERMINAL_CONFIGURE 1
 USES_TERMINAL_BUILD 1
 USES_TERMINAL_INSTALL 1
+# Always run the build command so that incremental builds are correct.
+BUILD_ALWAYS 1
 )
 
   get_ext_project_build_command(run_clean_compiler_rt clean)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98291: [compiler-rt] Fix stale incremental builds when using `LLVM_BUILD_EXTERNAL_COMPILER_RT=ON`.

[Dan Liew via Phabricator via cfe-commits]

delcypher created this revision.
delcypher added reviewers: kubamracek, yln, aralisza, beanz, samsonov.
Herald added subscribers: mgorny, dberris.
delcypher requested review of this revision.
Herald added a project: clang.

When building with `LLVM_BUILD_EXTERNAL_COMPILER_RT=ON` (e.g. Swift does
this) we do an "external" build of compiler-rt where we build
compiler-rt with the just built clang.

Unfortunately building in this mode had a bug where compiler-rt would
not get rebuilt if compiler-rt sources changed. This is problematic
for incremental builds because it meant that the compiler-rt binaries
were stale.

The fix is to use the `BUILD_ALWAYS` ExternalProject_Add option which
means the build command for compiler-rt is always run.

If all of the following are true:

- compiler-rt has already been built.
- there are no compiler-rt source changes.
- ninja is being used as the generator for the compiler-rt build.

then the overhead for always running the build command for incremental
builds is negligible.

rdar://75150660


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D98291

Files:
  clang/runtime/CMakeLists.txt


Index: clang/runtime/CMakeLists.txt
===
--- clang/runtime/CMakeLists.txt
+++ clang/runtime/CMakeLists.txt
@@ -95,6 +95,8 @@
 USES_TERMINAL_CONFIGURE 1
 USES_TERMINAL_BUILD 1
 USES_TERMINAL_INSTALL 1
+# Always run the build command so that incremental builds are correct.
+BUILD_ALWAYS 1
 )
 
   get_ext_project_build_command(run_clean_compiler_rt clean)


Index: clang/runtime/CMakeLists.txt
===
--- clang/runtime/CMakeLists.txt
+++ clang/runtime/CMakeLists.txt
@@ -95,6 +95,8 @@
 USES_TERMINAL_CONFIGURE 1
 USES_TERMINAL_BUILD 1
 USES_TERMINAL_INSTALL 1
+# Always run the build command so that incremental builds are correct.
+BUILD_ALWAYS 1
 )
 
   get_ext_project_build_command(run_clean_compiler_rt clean)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97496: [Clang][ASan] Correct AsanDtorKindToString to return non-void in default case

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

@cryptoad Thanks.

LGTM

Originally this code had a `default` case in the switch but clang complained 
about it being unnecessary because it was exhaustive, so I removed it... It 
seems this is causing problems with your compiler :(


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97496

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

[PATCH] D97327: [NFC] Switch to auto marshalling infrastructure for `-fsanitize-address-destructor-kind=` flag.

[Dan Liew 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 rG7b1d2a2891d8: [NFC] Switch to auto marshalling 
infrastructure for `-fsanitize-address… (authored by delcypher).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97327

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/asan-destructor-kind.cpp


Index: clang/test/CodeGen/asan-destructor-kind.cpp
===
--- clang/test/CodeGen/asan-destructor-kind.cpp
+++ clang/test/CodeGen/asan-destructor-kind.cpp
@@ -3,7 +3,7 @@
 // RUN:   -fsanitize-address-destructor-kind=bad_arg -emit-llvm -o - \
 // RUN:   -triple x86_64-apple-macosx10.15 %s 2>&1 | \
 // RUN:   FileCheck %s --check-prefixes=CHECK-BAD-ARG
-// CHECK-BAD-ARG: unsupported argument 'bad_arg' to option 
'fsanitize-address-destructor-kind='
+// CHECK-BAD-ARG: invalid value 'bad_arg' in 
'-fsanitize-address-destructor-kind=bad_arg'
 
 // Default is global dtor
 // RUN: %clang_cc1 -fsanitize=address -emit-llvm -o - -triple 
x86_64-apple-macosx10.15 \
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1937,19 +1937,6 @@
 
   Opts.EmitVersionIdentMetadata = Args.hasFlag(OPT_Qy, OPT_Qn, true);
 
-  if (LangOptsRef.Sanitize.has(SanitizerKind::Address)) {
-if (Arg *A =
-Args.getLastArg(options::OPT_sanitize_address_destructor_kind_EQ)) 
{
-  auto destructorKind = AsanDtorKindFromString(A->getValue());
-  if (destructorKind == llvm::AsanDtorKind::Invalid) {
-Diags.Report(clang::diag::err_drv_unsupported_option_argument)
-<< A->getOption().getName() << A->getValue();
-  } else {
-Opts.setSanitizeAddressDtorKind(destructorKind);
-  }
-}
-  }
-
   if (Args.hasArg(options::OPT_ffinite_loops))
 Opts.FiniteLoops = CodeGenOptions::FiniteLoopsKind::Always;
   else if (Args.hasArg(options::OPT_fno_finite_loops))
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1500,11 +1500,16 @@
 " reports in partially sanitized programs at the cost of an 
increase in binary size">,
   NegFlag>,
   Group;
-def sanitize_address_destructor_kind_EQ : Joined<["-"], 
"fsanitize-address-destructor-kind=">,
-  MetaVarName<"">,
-  Flags<[CC1Option]>,
-  HelpText<"Set destructor type used in ASan instrumentation">,
-  Group;
+def sanitize_address_destructor_kind_EQ
+: Joined<["-"], "fsanitize-address-destructor-kind=">,
+  MetaVarName<"">,
+  Flags<[CC1Option]>,
+  HelpText<"Set destructor type used in ASan instrumentation">,
+  Group,
+  Values<"none,global">,
+  NormalizedValuesScope<"llvm::AsanDtorKind">,
+  NormalizedValues<["None", "Global"]>,
+  MarshallingInfoEnum, "Global">;
 // Note: This flag was introduced when it was necessary to distinguish between
 //   ABI for correct codegen.  This is no longer needed, but the flag is
 //   not removed since targeting either ABI will behave the same.


Index: clang/test/CodeGen/asan-destructor-kind.cpp
===
--- clang/test/CodeGen/asan-destructor-kind.cpp
+++ clang/test/CodeGen/asan-destructor-kind.cpp
@@ -3,7 +3,7 @@
 // RUN:   -fsanitize-address-destructor-kind=bad_arg -emit-llvm -o - \
 // RUN:   -triple x86_64-apple-macosx10.15 %s 2>&1 | \
 // RUN:   FileCheck %s --check-prefixes=CHECK-BAD-ARG
-// CHECK-BAD-ARG: unsupported argument 'bad_arg' to option 'fsanitize-address-destructor-kind='
+// CHECK-BAD-ARG: invalid value 'bad_arg' in '-fsanitize-address-destructor-kind=bad_arg'
 
 // Default is global dtor
 // RUN: %clang_cc1 -fsanitize=address -emit-llvm -o - -triple x86_64-apple-macosx10.15 \
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1937,19 +1937,6 @@
 
   Opts.EmitVersionIdentMetadata = Args.hasFlag(OPT_Qy, OPT_Qn, true);
 
-  if (LangOptsRef.Sanitize.has(SanitizerKind::Address)) {
-if (Arg *A =
-Args.getLastArg(options::OPT_sanitize_address_destructor_kind_EQ)) {
-  auto destructorKind = AsanDtorKindFromString(A->getValue());
-  if (destructorKind == llvm::AsanDtorKind::Invalid) {
-Diags.Report(clang::diag::err_drv_unsupported_option_argument)
-<< A->getOption().getName() << A->getValue();
-  } else {
-Opts.setSanitizeAddressDtorKind(destructorKind);
-  }
-}
-  }
-
   if 

[PATCH] D97327: [NFC] Switch to auto marshalling infrastructure for `-fsanitize-address-destructor-kind=` flag.

[Dan Liew via Phabricator via cfe-commits]

delcypher updated this revision to Diff 326474.
delcypher added a comment.

- Fix build.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97327

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/asan-destructor-kind.cpp


Index: clang/test/CodeGen/asan-destructor-kind.cpp
===
--- clang/test/CodeGen/asan-destructor-kind.cpp
+++ clang/test/CodeGen/asan-destructor-kind.cpp
@@ -3,7 +3,7 @@
 // RUN:   -fsanitize-address-destructor-kind=bad_arg -emit-llvm -o - \
 // RUN:   -triple x86_64-apple-macosx10.15 %s 2>&1 | \
 // RUN:   FileCheck %s --check-prefixes=CHECK-BAD-ARG
-// CHECK-BAD-ARG: unsupported argument 'bad_arg' to option 
'fsanitize-address-destructor-kind='
+// CHECK-BAD-ARG: invalid value 'bad_arg' in 
'-fsanitize-address-destructor-kind=bad_arg'
 
 // Default is global dtor
 // RUN: %clang_cc1 -fsanitize=address -emit-llvm -o - -triple 
x86_64-apple-macosx10.15 \
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1937,19 +1937,6 @@
 
   Opts.EmitVersionIdentMetadata = Args.hasFlag(OPT_Qy, OPT_Qn, true);
 
-  if (LangOptsRef.Sanitize.has(SanitizerKind::Address)) {
-if (Arg *A =
-Args.getLastArg(options::OPT_sanitize_address_destructor_kind_EQ)) 
{
-  auto destructorKind = AsanDtorKindFromString(A->getValue());
-  if (destructorKind == llvm::AsanDtorKind::Invalid) {
-Diags.Report(clang::diag::err_drv_unsupported_option_argument)
-<< A->getOption().getName() << A->getValue();
-  } else {
-Opts.setSanitizeAddressDtorKind(destructorKind);
-  }
-}
-  }
-
   if (Args.hasArg(options::OPT_ffinite_loops))
 Opts.FiniteLoops = CodeGenOptions::FiniteLoopsKind::Always;
   else if (Args.hasArg(options::OPT_fno_finite_loops))
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1500,11 +1500,16 @@
 " reports in partially sanitized programs at the cost of an 
increase in binary size">,
   NegFlag>,
   Group;
-def sanitize_address_destructor_kind_EQ : Joined<["-"], 
"fsanitize-address-destructor-kind=">,
-  MetaVarName<"">,
-  Flags<[CC1Option]>,
-  HelpText<"Set destructor type used in ASan instrumentation">,
-  Group;
+def sanitize_address_destructor_kind_EQ
+: Joined<["-"], "fsanitize-address-destructor-kind=">,
+  MetaVarName<"">,
+  Flags<[CC1Option]>,
+  HelpText<"Set destructor type used in ASan instrumentation">,
+  Group,
+  Values<"none,global">,
+  NormalizedValuesScope<"llvm::AsanDtorKind">,
+  NormalizedValues<["None", "Global"]>,
+  MarshallingInfoEnum, "Global">;
 // Note: This flag was introduced when it was necessary to distinguish between
 //   ABI for correct codegen.  This is no longer needed, but the flag is
 //   not removed since targeting either ABI will behave the same.


Index: clang/test/CodeGen/asan-destructor-kind.cpp
===
--- clang/test/CodeGen/asan-destructor-kind.cpp
+++ clang/test/CodeGen/asan-destructor-kind.cpp
@@ -3,7 +3,7 @@
 // RUN:   -fsanitize-address-destructor-kind=bad_arg -emit-llvm -o - \
 // RUN:   -triple x86_64-apple-macosx10.15 %s 2>&1 | \
 // RUN:   FileCheck %s --check-prefixes=CHECK-BAD-ARG
-// CHECK-BAD-ARG: unsupported argument 'bad_arg' to option 'fsanitize-address-destructor-kind='
+// CHECK-BAD-ARG: invalid value 'bad_arg' in '-fsanitize-address-destructor-kind=bad_arg'
 
 // Default is global dtor
 // RUN: %clang_cc1 -fsanitize=address -emit-llvm -o - -triple x86_64-apple-macosx10.15 \
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1937,19 +1937,6 @@
 
   Opts.EmitVersionIdentMetadata = Args.hasFlag(OPT_Qy, OPT_Qn, true);
 
-  if (LangOptsRef.Sanitize.has(SanitizerKind::Address)) {
-if (Arg *A =
-Args.getLastArg(options::OPT_sanitize_address_destructor_kind_EQ)) {
-  auto destructorKind = AsanDtorKindFromString(A->getValue());
-  if (destructorKind == llvm::AsanDtorKind::Invalid) {
-Diags.Report(clang::diag::err_drv_unsupported_option_argument)
-<< A->getOption().getName() << A->getValue();
-  } else {
-Opts.setSanitizeAddressDtorKind(destructorKind);
-  }
-}
-  }
-
   if (Args.hasArg(options::OPT_ffinite_loops))
 Opts.FiniteLoops = CodeGenOptions::FiniteLoopsKind::Always;
   else if (Args.hasArg(options::OPT_fno_finite_loops))
Index: 

[PATCH] D96573: [Clang][ASan] Teach Clang to not emit ASan module destructors when compiling with `-mkernel` or `-fapple-kext`.

[Dan Liew 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 rGfdce098b49cb: [Clang][ASan] Teach Clang to not emit ASan 
module destructors when compiling… (authored by delcypher).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96573

Files:
  clang/lib/Driver/SanitizerArgs.cpp
  clang/test/Driver/darwin-asan-mkernel-kext.c


Index: clang/test/Driver/darwin-asan-mkernel-kext.c
===
--- /dev/null
+++ clang/test/Driver/darwin-asan-mkernel-kext.c
@@ -0,0 +1,15 @@
+// RUN: %clang -target x86_64-apple-darwin10 -fsanitize=address -mkernel -### \
+// RUN:   %s 2>&1 | FileCheck %s
+// RUN: %clang -target x86_64-apple-darwin10 -fsanitize=address -fapple-kext \
+// RUN:   -### %s 2>&1 | FileCheck %s
+// RUN: %clang -target x86_64-apple-darwin10 -fsanitize=address -fapple-kext \
+// RUN:   -mkernel -### %s 2>&1 | FileCheck %s
+
+// CHECK: "-fsanitize-address-destructor-kind=none"
+
+// Check it's possible to override the driver's decision.
+// RUN: %clang -target x86_64-apple-darwin10 -fsanitize=address -fapple-kext \
+// RUN:   -mkernel -### -fsanitize-address-destructor-kind=global %s 2>&1 | \
+// RUN:   FileCheck -check-prefix=CHECK-OVERRIDE %s
+
+// CHECK-OVERRIDE: "-fsanitize-address-destructor-kind=global"
Index: clang/lib/Driver/SanitizerArgs.cpp
===
--- clang/lib/Driver/SanitizerArgs.cpp
+++ clang/lib/Driver/SanitizerArgs.cpp
@@ -825,6 +825,12 @@
   AsanInvalidPointerSub = true;
 }
 
+if (TC.getTriple().isOSDarwin() &&
+(Args.hasArg(options::OPT_mkernel) ||
+ Args.hasArg(options::OPT_fapple_kext))) {
+  AsanDtorKind = llvm::AsanDtorKind::None;
+}
+
 if (const auto *Arg =
 Args.getLastArg(options::OPT_sanitize_address_destructor_kind_EQ)) 
{
   auto parsedAsanDtorKind = AsanDtorKindFromString(Arg->getValue());


Index: clang/test/Driver/darwin-asan-mkernel-kext.c
===
--- /dev/null
+++ clang/test/Driver/darwin-asan-mkernel-kext.c
@@ -0,0 +1,15 @@
+// RUN: %clang -target x86_64-apple-darwin10 -fsanitize=address -mkernel -### \
+// RUN:   %s 2>&1 | FileCheck %s
+// RUN: %clang -target x86_64-apple-darwin10 -fsanitize=address -fapple-kext \
+// RUN:   -### %s 2>&1 | FileCheck %s
+// RUN: %clang -target x86_64-apple-darwin10 -fsanitize=address -fapple-kext \
+// RUN:   -mkernel -### %s 2>&1 | FileCheck %s
+
+// CHECK: "-fsanitize-address-destructor-kind=none"
+
+// Check it's possible to override the driver's decision.
+// RUN: %clang -target x86_64-apple-darwin10 -fsanitize=address -fapple-kext \
+// RUN:   -mkernel -### -fsanitize-address-destructor-kind=global %s 2>&1 | \
+// RUN:   FileCheck -check-prefix=CHECK-OVERRIDE %s
+
+// CHECK-OVERRIDE: "-fsanitize-address-destructor-kind=global"
Index: clang/lib/Driver/SanitizerArgs.cpp
===
--- clang/lib/Driver/SanitizerArgs.cpp
+++ clang/lib/Driver/SanitizerArgs.cpp
@@ -825,6 +825,12 @@
   AsanInvalidPointerSub = true;
 }
 
+if (TC.getTriple().isOSDarwin() &&
+(Args.hasArg(options::OPT_mkernel) ||
+ Args.hasArg(options::OPT_fapple_kext))) {
+  AsanDtorKind = llvm::AsanDtorKind::None;
+}
+
 if (const auto *Arg =
 Args.getLastArg(options::OPT_sanitize_address_destructor_kind_EQ)) {
   auto parsedAsanDtorKind = AsanDtorKindFromString(Arg->getValue());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96572: [Clang][ASan] Introduce `-fsanitize-address-destructor-kind=` driver & frontend option.

[Dan Liew 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 rG5d64dd8e3c22: [Clang][ASan] Introduce 
`-fsanitize-address-destructor-kind=` driver  frontend… (authored by 
delcypher).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96572

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Basic/Sanitizers.h
  clang/include/clang/Driver/Options.td
  clang/include/clang/Driver/SanitizerArgs.h
  clang/lib/Basic/Sanitizers.cpp
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/SanitizerArgs.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/asan-destructor-kind.cpp
  clang/test/Driver/fsanitize-address-destructor-kind.c

Index: clang/test/Driver/fsanitize-address-destructor-kind.c
===
--- /dev/null
+++ clang/test/Driver/fsanitize-address-destructor-kind.c
@@ -0,0 +1,20 @@
+// Option should not be passed to the frontend by default.
+// RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address %s \
+// RUN:   -### 2>&1 | \
+// RUN:   FileCheck %s
+// CHECK-NOT: -fsanitize-address-destructor-kind
+
+// RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=none %s -### 2>&1 | \
+// RUN:   FileCheck -check-prefix=CHECK-NONE-ARG %s
+// CHECK-NONE-ARG: "-fsanitize-address-destructor-kind=none"
+
+// RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=global %s -### 2>&1 | \
+// RUN:   FileCheck -check-prefix=CHECK-GLOBAL-ARG %s
+// CHECK-GLOBAL-ARG: "-fsanitize-address-destructor-kind=global"
+
+// RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=bad_arg %s -### 2>&1 | \
+// RUN:   FileCheck -check-prefix=CHECK-INVALID-ARG %s
+// CHECK-INVALID-ARG: error: unsupported argument 'bad_arg' to option 'fsanitize-address-destructor-kind='
Index: clang/test/CodeGen/asan-destructor-kind.cpp
===
--- /dev/null
+++ clang/test/CodeGen/asan-destructor-kind.cpp
@@ -0,0 +1,49 @@
+// Frontend rejects invalid option
+// RUN: not %clang_cc1 -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=bad_arg -emit-llvm -o - \
+// RUN:   -triple x86_64-apple-macosx10.15 %s 2>&1 | \
+// RUN:   FileCheck %s --check-prefixes=CHECK-BAD-ARG
+// CHECK-BAD-ARG: unsupported argument 'bad_arg' to option 'fsanitize-address-destructor-kind='
+
+// Default is global dtor
+// RUN: %clang_cc1 -fsanitize=address -emit-llvm -o - -triple x86_64-apple-macosx10.15 \
+// RUN:   -fno-legacy-pass-manager %s \
+// RUN:   | FileCheck %s --check-prefixes=CHECK-GLOBAL-DTOR
+//
+// RUN: %clang_cc1 -fsanitize=address -emit-llvm -o - -triple x86_64-apple-macosx10.15 \
+// RUN:   -flegacy-pass-manager %s \
+// RUN:   | FileCheck %s --check-prefixes=CHECK-GLOBAL-DTOR
+
+// Explictly ask for global dtor
+// RUN: %clang_cc1 -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=global -emit-llvm -o - \
+// RUN:   -triple x86_64-apple-macosx10.15 -fno-legacy-pass-manager %s | \
+// RUN:   FileCheck %s --check-prefixes=CHECK-GLOBAL-DTOR
+//
+// RUN: %clang_cc1 -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=global -emit-llvm -o - \
+// RUN:   -triple x86_64-apple-macosx10.15 -flegacy-pass-manager %s | \
+// RUN:   FileCheck %s --check-prefixes=CHECK-GLOBAL-DTOR
+
+// CHECK-GLOBAL-DTOR: llvm.global_dtor{{.+}}asan.module_dtor
+// CHECK-GLOBAL-DTOR: define internal void @asan.module_dtor
+
+// Explictly ask for no dtors
+// RUN: %clang_cc1 -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=none -emit-llvm -o - \
+// RUN:   -triple x86_64-apple-macosx10.15 -fno-legacy-pass-manager %s | \
+// RUN:   FileCheck %s --check-prefixes=CHECK-NONE-DTOR
+//
+// RUN: %clang_cc1 -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=none -emit-llvm -o - \
+// RUN:   -triple x86_64-apple-macosx10.15 -flegacy-pass-manager %s | \
+// RUN:   FileCheck %s --check-prefixes=CHECK-NONE-DTOR
+
+int global;
+
+int main() {
+  return global;
+}
+
+// CHECK-NONE-DTOR-NOT: llvm.global_dtor{{.+}}asan.module_dtor
+// CHECK-NONE-DTOR-NOT: define internal void @asan.module_dtor
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1937,6 +1937,19 @@
 
   Opts.EmitVersionIdentMetadata = Args.hasFlag(OPT_Qy, OPT_Qn, true);
 
+  if (LangOptsRef.Sanitize.has(SanitizerKind::Address)) {
+if (Arg *A =
+

[PATCH] D97327: [NFC] Switch to auto marshalling infrastructure for `-fsanitize-address-destructor-kind=` flag.

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

In D97327#2582981 , @yln wrote:

> LGTM, cleanup only, right?

Yes. This is just a small clean up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97327

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

[PATCH] D97327: [NFC] Switch to auto marshalling infrastructure for `-fsanitize-address-destructor-kind=` flag.

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

In D97327#2585410 , @jansvoboda11 
wrote:

> LGTM.
>
> If you don't get around committing this today, please rebase this on top of 
> D97375  that I'm going to commit tomorrow.

@jansvoboda11  This doesn't build anymore because 
https://reviews.llvm.org/D97375 removed `AutoNormalizeEnum`. Based on skim 
reading the patch it looks like I should remove the `AutoNormalizeEnum` mixin 
and replace `MarshallingInfoString` with `MarshallingInfoEnum`. Does that sound 
right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97327

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

[PATCH] D96572: [Clang][ASan] Introduce `-fsanitize-address-destructor-kind=` driver & frontend option.

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

@arphaman @jansvoboda11 ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96572

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

[PATCH] D96572: [Clang][ASan] Introduce `-fsanitize-address-destructor-kind=` driver & frontend option.

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

In D96572#2563707 , @jansvoboda11 
wrote:

> In D96572#2561216 , @delcypher wrote:
>
>> @jansvoboda11 I noticed that the `Options.td` file is making use of some 
>> fancy mixins that mean the option gets automatically marshalled by the 
>> frontend into the codegen options :). I tried this out using the patch to 
>> this patch and all my tests seem to pass. Should I be using these mixins (I 
>> don't know how stable they are because this looks like a new feature). Am I 
>> using them properly? It seems that the driver still has to parse the option 
>> manually (which is actually what I want) but the frontend automagically 
>> parses the option and modifies the right codegen option for me.
>
> Hi, yes, this is a new feature. The option generation is not being enforced 
> in any way yet, but the automatic marshalling should work. You can test it by 
> running your `clang -cc1` commands with the `-round-trip-args` flag (or 
> `clang` with `-Xclang -round-trip-args`). With this option, the original 
> `cc1` command line is first parsed into a dummy `CompilerInvocation`, which 
> is used to generate new command line. The new command line is what is then 
> used to create the real `CompilerInvocation`.
>
> As you've correctly discovered, this does not interact with the driver in any 
> way.
>
> I'll be sending an email to cfe-dev about this shortly. Your marshalling code 
> seems correct to me. If something does not work, check out the patch with 
> documentation D95790  and feel free to let 
> me know.

@jansvoboda11 Thanks. If it's okay with you I'll make the change to use the new 
marshalling infrastructure in a separate patch 
(https://reviews.llvm.org/D97327) because I may need to back port this patch to 
an older LLVM branch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96572

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

[PATCH] D97327: [NFC] Switch to auto marshalling infrastructure for `-fsanitize-address-destructor-kind=` flag.

[Dan Liew via Phabricator via cfe-commits]

delcypher created this revision.
delcypher added reviewers: jansvoboda11, arphaman, kubamracek, yln, aralisza.
Herald added a subscriber: dang.
delcypher requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This change simplifies `clang/lib/Frontend/CompilerInvocation.cpp`
because we no longer need to manually parse the flag and set codegen
options in the frontend. However, we still need to manually parse the
flag in the driver because:

- The marshalling infrastructure doesn't operate there.
- We need to do some platform specific checks in the driver that will likely 
never be supported by any kind of marshalling infrastructure.

rdar://71609176


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D97327

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/asan-destructor-kind.cpp


Index: clang/test/CodeGen/asan-destructor-kind.cpp
===
--- clang/test/CodeGen/asan-destructor-kind.cpp
+++ clang/test/CodeGen/asan-destructor-kind.cpp
@@ -3,7 +3,7 @@
 // RUN:   -fsanitize-address-destructor-kind=bad_arg -emit-llvm -o - \
 // RUN:   -triple x86_64-apple-macosx10.15 %s 2>&1 | \
 // RUN:   FileCheck %s --check-prefixes=CHECK-BAD-ARG
-// CHECK-BAD-ARG: unsupported argument 'bad_arg' to option 
'fsanitize-address-destructor-kind='
+// CHECK-BAD-ARG: invalid value 'bad_arg' in 
'-fsanitize-address-destructor-kind=bad_arg'
 
 // Default is global dtor
 // RUN: %clang_cc1 -fsanitize=address -emit-llvm -o - -triple 
x86_64-apple-macosx10.15 \
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1928,19 +1928,6 @@
 
   Opts.EmitVersionIdentMetadata = Args.hasFlag(OPT_Qy, OPT_Qn, true);
 
-  if (LangOptsRef.Sanitize.has(SanitizerKind::Address)) {
-if (Arg *A =
-Args.getLastArg(options::OPT_sanitize_address_destructor_kind_EQ)) 
{
-  auto destructorKind = AsanDtorKindFromString(A->getValue());
-  if (destructorKind == llvm::AsanDtorKind::Invalid) {
-Diags.Report(clang::diag::err_drv_unsupported_option_argument)
-<< A->getOption().getName() << A->getValue();
-  } else {
-Opts.setSanitizeAddressDtorKind(destructorKind);
-  }
-}
-  }
-
   if (Args.hasArg(options::OPT_ffinite_loops))
 Opts.FiniteLoops = CodeGenOptions::FiniteLoopsKind::Always;
   else if (Args.hasArg(options::OPT_fno_finite_loops))
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1501,11 +1501,17 @@
 " reports in partially sanitized programs at the cost of an 
increase in binary size">,
   NegFlag>,
   Group;
-def sanitize_address_destructor_kind_EQ : Joined<["-"], 
"fsanitize-address-destructor-kind=">,
-  MetaVarName<"">,
-  Flags<[CC1Option]>,
-  HelpText<"Set destructor type used in ASan instrumentation">,
-  Group;
+def sanitize_address_destructor_kind_EQ
+: Joined<["-"], "fsanitize-address-destructor-kind=">,
+  MetaVarName<"">,
+  Flags<[CC1Option]>,
+  HelpText<"Set destructor type used in ASan instrumentation">,
+  Group,
+  Values<"none,global">,
+  NormalizedValuesScope<"llvm::AsanDtorKind">,
+  NormalizedValues<["None", "Global"]>,
+  MarshallingInfoString, "Global">,
+  AutoNormalizeEnum;
 // Note: This flag was introduced when it was necessary to distinguish between
 //   ABI for correct codegen.  This is no longer needed, but the flag is
 //   not removed since targeting either ABI will behave the same.


Index: clang/test/CodeGen/asan-destructor-kind.cpp
===
--- clang/test/CodeGen/asan-destructor-kind.cpp
+++ clang/test/CodeGen/asan-destructor-kind.cpp
@@ -3,7 +3,7 @@
 // RUN:   -fsanitize-address-destructor-kind=bad_arg -emit-llvm -o - \
 // RUN:   -triple x86_64-apple-macosx10.15 %s 2>&1 | \
 // RUN:   FileCheck %s --check-prefixes=CHECK-BAD-ARG
-// CHECK-BAD-ARG: unsupported argument 'bad_arg' to option 'fsanitize-address-destructor-kind='
+// CHECK-BAD-ARG: invalid value 'bad_arg' in '-fsanitize-address-destructor-kind=bad_arg'
 
 // Default is global dtor
 // RUN: %clang_cc1 -fsanitize=address -emit-llvm -o - -triple x86_64-apple-macosx10.15 \
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1928,19 +1928,6 @@
 
   Opts.EmitVersionIdentMetadata = Args.hasFlag(OPT_Qy, OPT_Qn, true);
 
-  if (LangOptsRef.Sanitize.has(SanitizerKind::Address)) {
-if (Arg *A =
-

[PATCH] D96572: [Clang][ASan] Introduce `-fsanitize-address-destructor-kind=` driver & frontend option.

[Dan Liew via Phabricator via cfe-commits]

delcypher updated this revision to Diff 325872.
delcypher added a comment.

Rebase and fix merge conflict caused by 
f2f59d2a060788f17040ad924ee2d11da0e215c8 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96572

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Basic/Sanitizers.h
  clang/include/clang/Driver/Options.td
  clang/include/clang/Driver/SanitizerArgs.h
  clang/lib/Basic/Sanitizers.cpp
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/SanitizerArgs.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/asan-destructor-kind.cpp
  clang/test/Driver/fsanitize-address-destructor-kind.c

Index: clang/test/Driver/fsanitize-address-destructor-kind.c
===
--- /dev/null
+++ clang/test/Driver/fsanitize-address-destructor-kind.c
@@ -0,0 +1,20 @@
+// Option should not be passed to the frontend by default.
+// RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address %s \
+// RUN:   -### 2>&1 | \
+// RUN:   FileCheck %s
+// CHECK-NOT: -fsanitize-address-destructor-kind
+
+// RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=none %s -### 2>&1 | \
+// RUN:   FileCheck -check-prefix=CHECK-NONE-ARG %s
+// CHECK-NONE-ARG: "-fsanitize-address-destructor-kind=none"
+
+// RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=global %s -### 2>&1 | \
+// RUN:   FileCheck -check-prefix=CHECK-GLOBAL-ARG %s
+// CHECK-GLOBAL-ARG: "-fsanitize-address-destructor-kind=global"
+
+// RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=bad_arg %s -### 2>&1 | \
+// RUN:   FileCheck -check-prefix=CHECK-INVALID-ARG %s
+// CHECK-INVALID-ARG: error: unsupported argument 'bad_arg' to option 'fsanitize-address-destructor-kind='
Index: clang/test/CodeGen/asan-destructor-kind.cpp
===
--- /dev/null
+++ clang/test/CodeGen/asan-destructor-kind.cpp
@@ -0,0 +1,49 @@
+// Frontend rejects invalid option
+// RUN: not %clang_cc1 -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=bad_arg -emit-llvm -o - \
+// RUN:   -triple x86_64-apple-macosx10.15 %s 2>&1 | \
+// RUN:   FileCheck %s --check-prefixes=CHECK-BAD-ARG
+// CHECK-BAD-ARG: unsupported argument 'bad_arg' to option 'fsanitize-address-destructor-kind='
+
+// Default is global dtor
+// RUN: %clang_cc1 -fsanitize=address -emit-llvm -o - -triple x86_64-apple-macosx10.15 \
+// RUN:   -fno-legacy-pass-manager %s \
+// RUN:   | FileCheck %s --check-prefixes=CHECK-GLOBAL-DTOR
+//
+// RUN: %clang_cc1 -fsanitize=address -emit-llvm -o - -triple x86_64-apple-macosx10.15 \
+// RUN:   -flegacy-pass-manager %s \
+// RUN:   | FileCheck %s --check-prefixes=CHECK-GLOBAL-DTOR
+
+// Explictly ask for global dtor
+// RUN: %clang_cc1 -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=global -emit-llvm -o - \
+// RUN:   -triple x86_64-apple-macosx10.15 -fno-legacy-pass-manager %s | \
+// RUN:   FileCheck %s --check-prefixes=CHECK-GLOBAL-DTOR
+//
+// RUN: %clang_cc1 -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=global -emit-llvm -o - \
+// RUN:   -triple x86_64-apple-macosx10.15 -flegacy-pass-manager %s | \
+// RUN:   FileCheck %s --check-prefixes=CHECK-GLOBAL-DTOR
+
+// CHECK-GLOBAL-DTOR: llvm.global_dtor{{.+}}asan.module_dtor
+// CHECK-GLOBAL-DTOR: define internal void @asan.module_dtor
+
+// Explictly ask for no dtors
+// RUN: %clang_cc1 -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=none -emit-llvm -o - \
+// RUN:   -triple x86_64-apple-macosx10.15 -fno-legacy-pass-manager %s | \
+// RUN:   FileCheck %s --check-prefixes=CHECK-NONE-DTOR
+//
+// RUN: %clang_cc1 -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=none -emit-llvm -o - \
+// RUN:   -triple x86_64-apple-macosx10.15 -flegacy-pass-manager %s | \
+// RUN:   FileCheck %s --check-prefixes=CHECK-NONE-DTOR
+
+int global;
+
+int main() {
+  return global;
+}
+
+// CHECK-NONE-DTOR-NOT: llvm.global_dtor{{.+}}asan.module_dtor
+// CHECK-NONE-DTOR-NOT: define internal void @asan.module_dtor
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1928,6 +1928,19 @@
 
   Opts.EmitVersionIdentMetadata = Args.hasFlag(OPT_Qy, OPT_Qn, true);
 
+  if (LangOptsRef.Sanitize.has(SanitizerKind::Address)) {
+if (Arg *A =
+Args.getLastArg(options::OPT_sanitize_address_destructor_kind_EQ)) {
+  auto 

[PATCH] D96572: [Clang][ASan] Introduce `-fsanitize-address-destructor-kind=` driver & frontend option.

[Dan Liew via Phabricator via cfe-commits]

delcypher added inline comments.



Comment at: clang/include/clang/Driver/Options.td:1485
+def sanitize_address_destructor_kind_EQ : Joined<["-"], 
"fsanitize-address-destructor-kind=">,
+  MetaVarName<"">,
+  Flags<[CC1Option]>,

vitalybuka wrote:
> delcypher wrote:
> > delcypher wrote:
> > > vitalybuka wrote:
> > > > What is the difference between them and why it's need to be configured 
> > > > from outside?
> > > Good questions. 
> > > 
> > > > What is the difference between them
> > > 
> > > I'll add some documentation to explain this. But the two different modes 
> > > are introduced by https://reviews.llvm.org/D96571. Basically the default 
> > > ("global") emits ASan module destructor calls via `llvm.global_dtors` 
> > > which was the previous behaviour. The other mode "none" simply causes no 
> > > ASan module destructors to be emitted. I plan to introduce another mode 
> > > in a future patch.
> > > 
> > > > why it's need to be configured from outside?
> > > 
> > > At the bare minimum this option needs to be controllable by the driver so 
> > > that we can do the next patch (https://reviews.llvm.org/D96573) where 
> > > based on target details and other flags we set the ASan destructor kind. 
> > > This means that the frontend needs to be able to consume to option too.
> > > 
> > > It is technically not necessary for this new option to be a driver 
> > > option. However, I made it a driver option too to match the existing ASan 
> > > clang frontend options which are also driver options (e.g. 
> > > `-fsanitize-address-use-odr-indicator`).
> > @vitalybuka To expand on my answer a little more. 
> > `-fsanitize-address-destructor-kind=` is needed because we need to able to 
> > control how ASan's module are emitted and unfortunately the information 
> > required to make the decision on which kind to emit **is not available at 
> > the level of LLVM IR**. The information we need to be able to make the 
> > decision is only available in the Clang driver. This is why this patch 
> > wires everything up so that the driver can change how ASan module 
> > destructors are emitted.
> > @vitalybuka To expand on my answer a little more. 
> > `-fsanitize-address-destructor-kind=` is needed because we need to able to 
> > control how ASan's module are emitted and unfortunately the information 
> > required to make the decision on which kind to emit **is not available at 
> > the level of LLVM IR**. The information we need to be able to make the 
> > decision is only available in the Clang driver. This is why this patch 
> > wires everything up so that the driver can change how ASan module 
> > destructors are emitted.
> 
> I didn't check all patches yet, but we have -fsanitize=kernel-address and I 
> noticed that your patches mentioned kernel. Could be possible just use 
> -fsanitize=kernel-address and let ASan decide how to handle dtors for this 
> platform?
@vitalybuka Unfortunately using `-fsanitize=kernel-address` isn't a viable 
option. In a future patch I'll be adding another destructor kind where the 
decision to use it is **also unfortunately not known at the LLVM IR level** and 
is instead only known by Clang driver.

If you really don't want `-fsanitize-address-destructor-kind=` exposed as 
driver option (i.e. exposed to our users) we could make it a frontend (cc1) 
only option. However, that doesn't really seem to match the existing sanitizer 
frontend options which all seem to be driver options as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96572

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

[PATCH] D96572: [Clang][ASan] Introduce `-fsanitize-address-destructor-kind=` driver & frontend option.

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

@jansvoboda11 I noticed that the `Options.td` file is making use of some fancy 
mixins that mean the option gets automatically marshalled by the frontend into 
the codegen options :). I tried this out using the patch to this patch and all 
my tests seem to pass. Should I be using these mixins (I don't know how stable 
they are because this looks like a new feature). Am I using them properly? It 
seems that the driver still has to parse the option manually (which is actually 
what I want) but the frontend automagically parses the option and modifies the 
right codegen option for me.

  diff --git a/clang/include/clang/Driver/Options.td 
b/clang/include/clang/Driver/Options.td
  index 57aa469ff211..0e8ea7debfca 100644
  --- a/clang/include/clang/Driver/Options.td
  +++ b/clang/include/clang/Driver/Options.td
  @@ -1481,11 +1481,17 @@ defm sanitize_address_use_odr_indicator : 
BoolOption<"f", "sanitize-address-use-
   " reports in partially sanitized programs at the cost of an 
increase in binary size">,
 NegFlag>,
 Group;
  -def sanitize_address_destructor_kind_EQ : Joined<["-"], 
"fsanitize-address-destructor-kind=">,
  -  MetaVarName<"">,
  -  Flags<[CC1Option]>,
  -  HelpText<"Set destructor type used in ASan instrumentation">,
  -  Group;
  +def sanitize_address_destructor_kind_EQ
  +: Joined<["-"], "fsanitize-address-destructor-kind=">,
  +  MetaVarName<"">,
  +  Flags<[CC1Option]>,
  +  HelpText<"Set destructor type used in ASan instrumentation">,
  +  Group,
  +  Values<"none,global">,
  +  NormalizedValuesScope<"llvm::AsanDtorKind">,
  +  NormalizedValues<["None", "Global"]>,
  +  MarshallingInfoString, 
"Global">,
  +  AutoNormalizeEnum;
   // Note: This flag was introduced when it was necessary to distinguish 
between
   //   ABI for correct codegen.  This is no longer needed, but the flag is
   //   not removed since targeting either ABI will behave the same.
  diff --git a/clang/lib/Frontend/CompilerInvocation.cpp 
b/clang/lib/Frontend/CompilerInvocation.cpp
  index a6baf026a7e8..0a12705a9261 100644
  --- a/clang/lib/Frontend/CompilerInvocation.cpp
  +++ b/clang/lib/Frontend/CompilerInvocation.cpp
  @@ -1511,19 +1511,6 @@ bool 
CompilerInvocation::ParseCodeGenArgs(CodeGenOptions , ArgList ,
   
 Opts.EmitVersionIdentMetadata = Args.hasFlag(OPT_Qy, OPT_Qn, true);
   
  -  if (LangOptsRef.Sanitize.has(SanitizerKind::Address)) {
  -if (Arg *A =
  -
Args.getLastArg(options::OPT_sanitize_address_destructor_kind_EQ)) {
  -  auto destructorKind = AsanDtorKindFromString(A->getValue());
  -  if (destructorKind == llvm::AsanDtorKind::Invalid) {
  -Diags.Report(clang::diag::err_drv_unsupported_option_argument)
  -<< A->getOption().getName() << A->getValue();
  -  } else {
  -Opts.setSanitizeAddressDtorKind(destructorKind);
  -  }
  -}
  -  }
  -
 return Success;
   }
   
  diff --git a/clang/test/CodeGen/asan-destructor-kind.cpp 
b/clang/test/CodeGen/asan-destructor-kind.cpp
  index 2bdb782db2b5..567482b72643 100644
  --- a/clang/test/CodeGen/asan-destructor-kind.cpp
  +++ b/clang/test/CodeGen/asan-destructor-kind.cpp
  @@ -3,7 +3,7 @@
   // RUN:   -fsanitize-address-destructor-kind=bad_arg -emit-llvm -o - \
   // RUN:   -triple x86_64-apple-macosx10.15 %s 2>&1 | \
   // RUN:   FileCheck %s --check-prefixes=CHECK-BAD-ARG
  -// CHECK-BAD-ARG: unsupported argument 'bad_arg' to option 
'fsanitize-address-destructor-kind='
  +// CHECK-BAD-ARG: invalid value 'bad_arg' in 
'-fsanitize-address-destructor-kind=bad_arg'
   
   // Default is global dtor
   // RUN: %clang_cc1 -fsanitize=address -emit-llvm -o - -triple 
x86_64-apple-macosx10.15 \


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96572

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

[PATCH] D96572: [Clang][ASan] Introduce `-fsanitize-address-destructor-kind=` driver & frontend option.

[Dan Liew via Phabricator via cfe-commits]

delcypher added inline comments.



Comment at: clang/include/clang/Driver/Options.td:1485
+def sanitize_address_destructor_kind_EQ : Joined<["-"], 
"fsanitize-address-destructor-kind=">,
+  MetaVarName<"">,
+  Flags<[CC1Option]>,

delcypher wrote:
> vitalybuka wrote:
> > What is the difference between them and why it's need to be configured from 
> > outside?
> Good questions. 
> 
> > What is the difference between them
> 
> I'll add some documentation to explain this. But the two different modes are 
> introduced by https://reviews.llvm.org/D96571. Basically the default 
> ("global") emits ASan module destructor calls via `llvm.global_dtors` which 
> was the previous behaviour. The other mode "none" simply causes no ASan 
> module destructors to be emitted. I plan to introduce another mode in a 
> future patch.
> 
> > why it's need to be configured from outside?
> 
> At the bare minimum this option needs to be controllable by the driver so 
> that we can do the next patch (https://reviews.llvm.org/D96573) where based 
> on target details and other flags we set the ASan destructor kind. This means 
> that the frontend needs to be able to consume to option too.
> 
> It is technically not necessary for this new option to be a driver option. 
> However, I made it a driver option too to match the existing ASan clang 
> frontend options which are also driver options (e.g. 
> `-fsanitize-address-use-odr-indicator`).
@vitalybuka To expand on my answer a little more. 
`-fsanitize-address-destructor-kind=` is needed because we need to able to 
control how ASan's module are emitted and unfortunately the information 
required to make the decision on which kind to emit **is not available at the 
level of LLVM IR**. The information we need to be able to make the decision is 
only available in the Clang driver. This is why this patch wires everything up 
so that the driver can change how ASan module destructors are emitted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96572

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

[PATCH] D96572: [Clang][ASan] Introduce `-fsanitize-address-destructor-kind=` driver & frontend option.

[Dan Liew via Phabricator via cfe-commits]

delcypher updated this revision to Diff 323455.
delcypher added a comment.

Add documentation for `-fsanitize-address-destructor-kind=` option.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96572

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Basic/Sanitizers.h
  clang/include/clang/Driver/Options.td
  clang/include/clang/Driver/SanitizerArgs.h
  clang/lib/Basic/Sanitizers.cpp
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/SanitizerArgs.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/asan-destructor-kind.cpp
  clang/test/Driver/fsanitize-address-destructor-kind.c

Index: clang/test/Driver/fsanitize-address-destructor-kind.c
===
--- /dev/null
+++ clang/test/Driver/fsanitize-address-destructor-kind.c
@@ -0,0 +1,20 @@
+// Option should not be passed to the frontend by default.
+// RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address %s \
+// RUN:   -### 2>&1 | \
+// RUN:   FileCheck %s
+// CHECK-NOT: -fsanitize-address-destructor-kind
+
+// RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=none %s -### 2>&1 | \
+// RUN:   FileCheck -check-prefix=CHECK-NONE-ARG %s
+// CHECK-NONE-ARG: "-fsanitize-address-destructor-kind=none"
+
+// RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=global %s -### 2>&1 | \
+// RUN:   FileCheck -check-prefix=CHECK-GLOBAL-ARG %s
+// CHECK-GLOBAL-ARG: "-fsanitize-address-destructor-kind=global"
+
+// RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=bad_arg %s -### 2>&1 | \
+// RUN:   FileCheck -check-prefix=CHECK-INVALID-ARG %s
+// CHECK-INVALID-ARG: error: unsupported argument 'bad_arg' to option 'fsanitize-address-destructor-kind='
Index: clang/test/CodeGen/asan-destructor-kind.cpp
===
--- /dev/null
+++ clang/test/CodeGen/asan-destructor-kind.cpp
@@ -0,0 +1,49 @@
+// Frontend rejects invalid option
+// RUN: not %clang_cc1 -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=bad_arg -emit-llvm -o - \
+// RUN:   -triple x86_64-apple-macosx10.15 %s 2>&1 | \
+// RUN:   FileCheck %s --check-prefixes=CHECK-BAD-ARG
+// CHECK-BAD-ARG: unsupported argument 'bad_arg' to option 'fsanitize-address-destructor-kind='
+
+// Default is global dtor
+// RUN: %clang_cc1 -fsanitize=address -emit-llvm -o - -triple x86_64-apple-macosx10.15 \
+// RUN:   -fno-legacy-pass-manager %s \
+// RUN:   | FileCheck %s --check-prefixes=CHECK-GLOBAL-DTOR
+//
+// RUN: %clang_cc1 -fsanitize=address -emit-llvm -o - -triple x86_64-apple-macosx10.15 \
+// RUN:   -flegacy-pass-manager %s \
+// RUN:   | FileCheck %s --check-prefixes=CHECK-GLOBAL-DTOR
+
+// Explictly ask for global dtor
+// RUN: %clang_cc1 -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=global -emit-llvm -o - \
+// RUN:   -triple x86_64-apple-macosx10.15 -fno-legacy-pass-manager %s | \
+// RUN:   FileCheck %s --check-prefixes=CHECK-GLOBAL-DTOR
+//
+// RUN: %clang_cc1 -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=global -emit-llvm -o - \
+// RUN:   -triple x86_64-apple-macosx10.15 -flegacy-pass-manager %s | \
+// RUN:   FileCheck %s --check-prefixes=CHECK-GLOBAL-DTOR
+
+// CHECK-GLOBAL-DTOR: llvm.global_dtor{{.+}}asan.module_dtor
+// CHECK-GLOBAL-DTOR: define internal void @asan.module_dtor
+
+// Explictly ask for no dtors
+// RUN: %clang_cc1 -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=none -emit-llvm -o - \
+// RUN:   -triple x86_64-apple-macosx10.15 -fno-legacy-pass-manager %s | \
+// RUN:   FileCheck %s --check-prefixes=CHECK-NONE-DTOR
+//
+// RUN: %clang_cc1 -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=none -emit-llvm -o - \
+// RUN:   -triple x86_64-apple-macosx10.15 -flegacy-pass-manager %s | \
+// RUN:   FileCheck %s --check-prefixes=CHECK-NONE-DTOR
+
+int global;
+
+int main() {
+  return global;
+}
+
+// CHECK-NONE-DTOR-NOT: llvm.global_dtor{{.+}}asan.module_dtor
+// CHECK-NONE-DTOR-NOT: define internal void @asan.module_dtor
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1511,6 +1511,19 @@
 
   Opts.EmitVersionIdentMetadata = Args.hasFlag(OPT_Qy, OPT_Qn, true);
 
+  if (LangOptsRef.Sanitize.has(SanitizerKind::Address)) {
+if (Arg *A =
+Args.getLastArg(options::OPT_sanitize_address_destructor_kind_EQ)) {
+  auto destructorKind = AsanDtorKindFromString(A->getValue());
+  if (destructorKind == 

[PATCH] D96572: [Clang][ASan] Introduce `-fsanitize-address-destructor-kind=` driver & frontend option.

[Dan Liew via Phabricator via cfe-commits]

delcypher added inline comments.



Comment at: clang/include/clang/Driver/Options.td:1485
+def sanitize_address_destructor_kind_EQ : Joined<["-"], 
"fsanitize-address-destructor-kind=">,
+  MetaVarName<"">,
+  Flags<[CC1Option]>,

vitalybuka wrote:
> What is the difference between them and why it's need to be configured from 
> outside?
Good questions. 

> What is the difference between them

I'll add some documentation to explain this. But the two different modes are 
introduced by https://reviews.llvm.org/D96571. Basically the default ("global") 
emits ASan module destructor calls via `llvm.global_dtors` which was the 
previous behaviour. The other mode "none" simply causes no ASan module 
destructors to be emitted. I plan to introduce another mode in a future patch.

> why it's need to be configured from outside?

At the bare minimum this option needs to be controllable by the driver so that 
we can do the next patch (https://reviews.llvm.org/D96573) where based on 
target details and other flags we set the ASan destructor kind. This means that 
the frontend needs to be able to consume to option too.

It is technically not necessary for this new option to be a driver option. 
However, I made it a driver option too to match the existing ASan clang 
frontend options which are also driver options (e.g. 
`-fsanitize-address-use-odr-indicator`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96572

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

[PATCH] D96572: [Clang][ASan] Introduce `-fsanitize-address-destructor-kind=` driver & frontend option.

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

Note this patch depends on https://reviews.llvm.org/D96571


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96572

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

[PATCH] D96573: [Clang][ASan] Teach Clang to not emit ASan module destructors when compiling with `-mkernel` or `-fapple-kext`.

[Dan Liew via Phabricator via cfe-commits]

delcypher created this revision.
delcypher added reviewers: arphaman, kubamracek, yln, aralisza, kcc, vitalybuka.
delcypher requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

rdar://71609176


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D96573

Files:
  clang/lib/Driver/SanitizerArgs.cpp
  clang/test/Driver/darwin-asan-mkernel-kext.c


Index: clang/test/Driver/darwin-asan-mkernel-kext.c
===
--- /dev/null
+++ clang/test/Driver/darwin-asan-mkernel-kext.c
@@ -0,0 +1,15 @@
+// RUN: %clang -target x86_64-apple-darwin10 -fsanitize=address -mkernel -### \
+// RUN:   %s 2>&1 | FileCheck %s
+// RUN: %clang -target x86_64-apple-darwin10 -fsanitize=address -fapple-kext \
+// RUN:   -### %s 2>&1 | FileCheck %s
+// RUN: %clang -target x86_64-apple-darwin10 -fsanitize=address -fapple-kext \
+// RUN:   -mkernel -### %s 2>&1 | FileCheck %s
+
+// CHECK: "-fsanitize-address-destructor-kind=none"
+
+// Check it's possible to override the driver's decision.
+// RUN: %clang -target x86_64-apple-darwin10 -fsanitize=address -fapple-kext \
+// RUN:   -mkernel -### -fsanitize-address-destructor-kind=global %s 2>&1 | \
+// RUN:   FileCheck -check-prefix=CHECK-OVERRIDE %s
+
+// CHECK-OVERRIDE: "-fsanitize-address-destructor-kind=global"
Index: clang/lib/Driver/SanitizerArgs.cpp
===
--- clang/lib/Driver/SanitizerArgs.cpp
+++ clang/lib/Driver/SanitizerArgs.cpp
@@ -825,6 +825,12 @@
   AsanInvalidPointerSub = true;
 }
 
+if (TC.getTriple().isOSDarwin() &&
+(Args.hasArg(options::OPT_mkernel) ||
+ Args.hasArg(options::OPT_fapple_kext))) {
+  AsanDtorKind = llvm::AsanDtorKind::None;
+}
+
 if (const auto *Arg =
 Args.getLastArg(options::OPT_sanitize_address_destructor_kind_EQ)) 
{
   auto parsedAsanDtorKind = AsanDtorKindFromString(Arg->getValue());


Index: clang/test/Driver/darwin-asan-mkernel-kext.c
===
--- /dev/null
+++ clang/test/Driver/darwin-asan-mkernel-kext.c
@@ -0,0 +1,15 @@
+// RUN: %clang -target x86_64-apple-darwin10 -fsanitize=address -mkernel -### \
+// RUN:   %s 2>&1 | FileCheck %s
+// RUN: %clang -target x86_64-apple-darwin10 -fsanitize=address -fapple-kext \
+// RUN:   -### %s 2>&1 | FileCheck %s
+// RUN: %clang -target x86_64-apple-darwin10 -fsanitize=address -fapple-kext \
+// RUN:   -mkernel -### %s 2>&1 | FileCheck %s
+
+// CHECK: "-fsanitize-address-destructor-kind=none"
+
+// Check it's possible to override the driver's decision.
+// RUN: %clang -target x86_64-apple-darwin10 -fsanitize=address -fapple-kext \
+// RUN:   -mkernel -### -fsanitize-address-destructor-kind=global %s 2>&1 | \
+// RUN:   FileCheck -check-prefix=CHECK-OVERRIDE %s
+
+// CHECK-OVERRIDE: "-fsanitize-address-destructor-kind=global"
Index: clang/lib/Driver/SanitizerArgs.cpp
===
--- clang/lib/Driver/SanitizerArgs.cpp
+++ clang/lib/Driver/SanitizerArgs.cpp
@@ -825,6 +825,12 @@
   AsanInvalidPointerSub = true;
 }
 
+if (TC.getTriple().isOSDarwin() &&
+(Args.hasArg(options::OPT_mkernel) ||
+ Args.hasArg(options::OPT_fapple_kext))) {
+  AsanDtorKind = llvm::AsanDtorKind::None;
+}
+
 if (const auto *Arg =
 Args.getLastArg(options::OPT_sanitize_address_destructor_kind_EQ)) {
   auto parsedAsanDtorKind = AsanDtorKindFromString(Arg->getValue());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96572: [Clang][ASan] Introduce `-fsanitize-address-destructor-kind=` driver & frontend option.

[Dan Liew via Phabricator via cfe-commits]

delcypher updated this revision to Diff 323213.
delcypher added a comment.

clang-format fix


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96572

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Basic/Sanitizers.h
  clang/include/clang/Driver/Options.td
  clang/include/clang/Driver/SanitizerArgs.h
  clang/lib/Basic/Sanitizers.cpp
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/SanitizerArgs.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/asan-destructor-kind.cpp
  clang/test/Driver/fsanitize-address-destructor-kind.c

Index: clang/test/Driver/fsanitize-address-destructor-kind.c
===
--- /dev/null
+++ clang/test/Driver/fsanitize-address-destructor-kind.c
@@ -0,0 +1,20 @@
+// Option should not be passed to the frontend by default.
+// RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address %s \
+// RUN:   -### 2>&1 | \
+// RUN:   FileCheck %s
+// CHECK-NOT: -fsanitize-address-destructor-kind
+
+// RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=none %s -### 2>&1 | \
+// RUN:   FileCheck -check-prefix=CHECK-NONE-ARG %s
+// CHECK-NONE-ARG: "-fsanitize-address-destructor-kind=none"
+
+// RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=global %s -### 2>&1 | \
+// RUN:   FileCheck -check-prefix=CHECK-GLOBAL-ARG %s
+// CHECK-GLOBAL-ARG: "-fsanitize-address-destructor-kind=global"
+
+// RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=bad_arg %s -### 2>&1 | \
+// RUN:   FileCheck -check-prefix=CHECK-INVALID-ARG %s
+// CHECK-INVALID-ARG: error: unsupported argument 'bad_arg' to option 'fsanitize-address-destructor-kind='
Index: clang/test/CodeGen/asan-destructor-kind.cpp
===
--- /dev/null
+++ clang/test/CodeGen/asan-destructor-kind.cpp
@@ -0,0 +1,49 @@
+// Frontend rejects invalid option
+// RUN: not %clang_cc1 -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=bad_arg -emit-llvm -o - \
+// RUN:   -triple x86_64-apple-macosx10.15 %s 2>&1 | \
+// RUN:   FileCheck %s --check-prefixes=CHECK-BAD-ARG
+// CHECK-BAD-ARG: unsupported argument 'bad_arg' to option 'fsanitize-address-destructor-kind='
+
+// Default is global dtor
+// RUN: %clang_cc1 -fsanitize=address -emit-llvm -o - -triple x86_64-apple-macosx10.15 \
+// RUN:   -fno-legacy-pass-manager %s \
+// RUN:   | FileCheck %s --check-prefixes=CHECK-GLOBAL-DTOR
+//
+// RUN: %clang_cc1 -fsanitize=address -emit-llvm -o - -triple x86_64-apple-macosx10.15 \
+// RUN:   -flegacy-pass-manager %s \
+// RUN:   | FileCheck %s --check-prefixes=CHECK-GLOBAL-DTOR
+
+// Explictly ask for global dtor
+// RUN: %clang_cc1 -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=global -emit-llvm -o - \
+// RUN:   -triple x86_64-apple-macosx10.15 -fno-legacy-pass-manager %s | \
+// RUN:   FileCheck %s --check-prefixes=CHECK-GLOBAL-DTOR
+//
+// RUN: %clang_cc1 -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=global -emit-llvm -o - \
+// RUN:   -triple x86_64-apple-macosx10.15 -flegacy-pass-manager %s | \
+// RUN:   FileCheck %s --check-prefixes=CHECK-GLOBAL-DTOR
+
+// CHECK-GLOBAL-DTOR: llvm.global_dtor{{.+}}asan.module_dtor
+// CHECK-GLOBAL-DTOR: define internal void @asan.module_dtor
+
+// Explictly ask for no dtors
+// RUN: %clang_cc1 -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=none -emit-llvm -o - \
+// RUN:   -triple x86_64-apple-macosx10.15 -fno-legacy-pass-manager %s | \
+// RUN:   FileCheck %s --check-prefixes=CHECK-NONE-DTOR
+//
+// RUN: %clang_cc1 -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=none -emit-llvm -o - \
+// RUN:   -triple x86_64-apple-macosx10.15 -flegacy-pass-manager %s | \
+// RUN:   FileCheck %s --check-prefixes=CHECK-NONE-DTOR
+
+int global;
+
+int main() {
+  return global;
+}
+
+// CHECK-NONE-DTOR-NOT: llvm.global_dtor{{.+}}asan.module_dtor
+// CHECK-NONE-DTOR-NOT: define internal void @asan.module_dtor
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1511,6 +1511,19 @@
 
   Opts.EmitVersionIdentMetadata = Args.hasFlag(OPT_Qy, OPT_Qn, true);
 
+  if (LangOptsRef.Sanitize.has(SanitizerKind::Address)) {
+if (Arg *A =
+Args.getLastArg(options::OPT_sanitize_address_destructor_kind_EQ)) {
+  auto destructorKind = AsanDtorKindFromString(A->getValue());
+  if (destructorKind == llvm::AsanDtorKind::Invalid) {
+

[PATCH] D96572: [Clang][ASan] Introduce `-fsanitize-address-destructor-kind=` driver & frontend option.

[Dan Liew via Phabricator via cfe-commits]

delcypher created this revision.
delcypher added reviewers: arphaman, kubamracek, yln, aralisza, kcc, vitalybuka.
Herald added subscribers: dexonsmith, dang.
Herald added a reviewer: jansvoboda11.
delcypher requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

The new `-fsanitize-address-destructor-kind=` option allows control over how 
module
destructors are emitted by ASan.

The new option is consumed by both the driver and the frontend and is 
propagated into
codegen options by the frontend.

Both the legacy and new pass manager code have been updated to consume the new 
option
from the codegen options.

It would be nice if the new utility functions (`AsanDtorKindToString` and
`AsanDtorKindFromString`) could live in LLVM instead of Clang so they could be
consumed by other language frontends. Unfortunately that doesn't work because
the clang driver doesn't link against the LLVM instrumentation library.

rdar://71609176


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D96572

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Basic/Sanitizers.h
  clang/include/clang/Driver/Options.td
  clang/include/clang/Driver/SanitizerArgs.h
  clang/lib/Basic/Sanitizers.cpp
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/SanitizerArgs.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/asan-destructor-kind.cpp
  clang/test/Driver/fsanitize-address-destructor-kind.c

Index: clang/test/Driver/fsanitize-address-destructor-kind.c
===
--- /dev/null
+++ clang/test/Driver/fsanitize-address-destructor-kind.c
@@ -0,0 +1,20 @@
+// Option should not be passed to the frontend by default.
+// RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address %s \
+// RUN:   -### 2>&1 | \
+// RUN:   FileCheck %s
+// CHECK-NOT: -fsanitize-address-destructor-kind
+
+// RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=none %s -### 2>&1 | \
+// RUN:   FileCheck -check-prefix=CHECK-NONE-ARG %s
+// CHECK-NONE-ARG: "-fsanitize-address-destructor-kind=none"
+
+// RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=global %s -### 2>&1 | \
+// RUN:   FileCheck -check-prefix=CHECK-GLOBAL-ARG %s
+// CHECK-GLOBAL-ARG: "-fsanitize-address-destructor-kind=global"
+
+// RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=bad_arg %s -### 2>&1 | \
+// RUN:   FileCheck -check-prefix=CHECK-INVALID-ARG %s
+// CHECK-INVALID-ARG: error: unsupported argument 'bad_arg' to option 'fsanitize-address-destructor-kind='
Index: clang/test/CodeGen/asan-destructor-kind.cpp
===
--- /dev/null
+++ clang/test/CodeGen/asan-destructor-kind.cpp
@@ -0,0 +1,49 @@
+// Frontend rejects invalid option
+// RUN: not %clang_cc1 -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=bad_arg -emit-llvm -o - \
+// RUN:   -triple x86_64-apple-macosx10.15 %s 2>&1 | \
+// RUN:   FileCheck %s --check-prefixes=CHECK-BAD-ARG
+// CHECK-BAD-ARG: unsupported argument 'bad_arg' to option 'fsanitize-address-destructor-kind='
+
+// Default is global dtor
+// RUN: %clang_cc1 -fsanitize=address -emit-llvm -o - -triple x86_64-apple-macosx10.15 \
+// RUN:   -fno-legacy-pass-manager %s \
+// RUN:   | FileCheck %s --check-prefixes=CHECK-GLOBAL-DTOR
+//
+// RUN: %clang_cc1 -fsanitize=address -emit-llvm -o - -triple x86_64-apple-macosx10.15 \
+// RUN:   -flegacy-pass-manager %s \
+// RUN:   | FileCheck %s --check-prefixes=CHECK-GLOBAL-DTOR
+
+// Explictly ask for global dtor
+// RUN: %clang_cc1 -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=global -emit-llvm -o - \
+// RUN:   -triple x86_64-apple-macosx10.15 -fno-legacy-pass-manager %s | \
+// RUN:   FileCheck %s --check-prefixes=CHECK-GLOBAL-DTOR
+//
+// RUN: %clang_cc1 -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=global -emit-llvm -o - \
+// RUN:   -triple x86_64-apple-macosx10.15 -flegacy-pass-manager %s | \
+// RUN:   FileCheck %s --check-prefixes=CHECK-GLOBAL-DTOR
+
+// CHECK-GLOBAL-DTOR: llvm.global_dtor{{.+}}asan.module_dtor
+// CHECK-GLOBAL-DTOR: define internal void @asan.module_dtor
+
+// Explictly ask for no dtors
+// RUN: %clang_cc1 -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=none -emit-llvm -o - \
+// RUN:   -triple x86_64-apple-macosx10.15 -fno-legacy-pass-manager %s | \
+// RUN:   FileCheck %s --check-prefixes=CHECK-NONE-DTOR
+//
+// RUN: %clang_cc1 -fsanitize=address \
+// RUN:   -fsanitize-address-destructor-kind=none -emit-llvm -o - \
+// RUN:   -triple x86_64-apple-macosx10.15 -flegacy-pass-manager %s | \
+// RUN:   FileCheck %s --check-prefixes=CHECK-NONE-DTOR
+
+int global;
+

[PATCH] D72875: [clang][cmake] Include generated rst files in html built by docs-clang-html target

[Dan Liew via Phabricator via cfe-commits]

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

LGTM. Thanks for addressing all the issues I raised.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72875



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

[PATCH] D72875: [clang][cmake] Include generated rst files in html built by docs-clang-html target

[Dan Liew via Phabricator via cfe-commits]

delcypher requested changes to this revision.
delcypher added inline comments.
This revision now requires changes to proceed.



Comment at: clang/docs/CMakeLists.txt:93
 
+function (gen_rst_file output_file td_option source)
+  get_filename_component(TABLEGEN_INCLUDE_DIR 
"${CMAKE_CURRENT_SOURCE_DIR}/${source}" DIRECTORY)

`gen_rst_file` is a very ambiguous name. At call sites it might not be obvious 
what this function does. Something like `gen_rst_file_from_td` might be more 
helpful.



Comment at: clang/docs/CMakeLists.txt:94
+function (gen_rst_file output_file td_option source)
+  get_filename_component(TABLEGEN_INCLUDE_DIR 
"${CMAKE_CURRENT_SOURCE_DIR}/${source}" DIRECTORY)
+  list(APPEND LLVM_TABLEGEN_FLAGS "-I${TABLEGEN_INCLUDE_DIR}")

You might want to add a check that `${CMAKE_CURRENT_SOURCE_DIR}/${source}` 
exists in this function and error if it does not exist.



Comment at: clang/docs/CMakeLists.txt:97
+  clang_tablegen(${output_file} ${td_option} SOURCE ${source} TARGET 
"gen-${output_file}")
+  add_dependencies(docs-clang-html "gen-${output_file}")
+endfunction()

This `add_dependencies(...)` command is very fragile and is creating an 
implicit coupling between:

* The implementation of `add_sphinx_target()`.
* The declaration of the clang html sphinx target.
* The implementation of `gen_rst_file`.

`gen_rst_file` should probably take an optional argument for the target (in 
this case `docs-clang-html`) to add a dependency to.

This does not completely fix the dependency on `add_sphinx_target` 
implementation but it does mean that `gen_rst_file` isn't also dependent.



Comment at: clang/docs/CMakeLists.txt:104
 if (${SPHINX_OUTPUT_HTML})
-  add_sphinx_target(html clang)
+  add_sphinx_target(html clang SOURCE_DIR ${CMAKE_CURRENT_BINARY_DIR})
+

Minor nit: You might want to quote uses of `${CMAKE_CURRENT_BINARY_DIR}`, 
`${CMAKE_COMMAND}`, and `${CMAKE_CURRENT_SOURCE_DIR}`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72875



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

[PATCH] D71491: [ubsan] Check implicit casts in ObjC for-in statements

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

@vsk The compiler-rt side seems fine to me but I'm not very familiar with the 
Clang side of things. @arphaman @jfb  @rjmccall any thoughts?


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

https://reviews.llvm.org/D71491



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

[PATCH] D72875: [clang][cmake] Include generated rst files in html built by docs-clang-html target

[Dan Liew via Phabricator via cfe-commits]

delcypher added inline comments.



Comment at: llvm/cmake/modules/AddSphinxTarget.cmake:33
+  if (NOT ARG_SOURCE_DIR)
+set(ARG_SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR})
+  endif()

@tstellar I'm not 100% sure about this but I think you probably want 
`${CMAKE_CURRENT_SOURCE_DIR}` quoted. If it's not then if it contains spaces I 
think you might make `ARG_SOURCE_DIR` a list. Then we you go to print it you'll 
probably end up with list separators (i.e. `;`) in the command you pass the the 
sphinx binary.

I've not tested this though so I could be wrong.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72875



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

[PATCH] D71491: [ubsan] Check implicit casts in ObjC for-in statements

[Dan Liew via Phabricator via cfe-commits]

delcypher added inline comments.



Comment at: compiler-rt/lib/ubsan/ubsan_value.cpp:29
+const char *__ubsan::getObjCClassName(ValueHandle Pointer) {
+#if defined(__APPLE__)
+  // We need to query the ObjC runtime for some information, but do not want

vsk wrote:
> delcypher wrote:
> > vsk wrote:
> > > delcypher wrote:
> > > > The compiler-rt codebase tends to use `SANITIZER_MAC` macro (defined to 
> > > > be 1 if Apple otherwise it's 0) rather than `__APPLE__`.
> > > I see. That seems problematic, as it makes it tricky to add 
> > > macOS-specific (or iOS-specific) functionality down the road. I don't 
> > > think SANITIZER_MAC should be defined unless TARGET_OS_MACOS is true.
> > Sorry I should clarify. `SANITIZER_MAC` is poorly named but it is defined 
> > to be `1` for Apple platforms and `0`. I'm just pointing out the convention 
> > that exists today. You're absolutely right that we might want to do 
> > different things for different Apple platforms but I don't think we want to 
> > start doing a mass re-name until arm64e and arm64_32 support are completed 
> > landed in llvm's master.
> I think 'SANITIZER_MAC' is confusing, and my preference would be to not use 
> it. `__APPLE__` seems clearer to me, and (IIUC) the plan is to replace usage 
> of 'SANITIZER_MAC' with it down the line anyway?
There aren't any plans to do it right now but cleaning this up seems like a 
reasonable thing to do. If you take a look at 
`compiler-rt/lib/sanitizer_common/sanitizer_platform.h` you'll see that we 
actually have `SANITIZER_` for the other platforms that seems to be 
set as you'd expect. It's just `SANITIZER_MAC` that's badly named.

I've filed a radar for this issue (rdar://problem/58124919). So if you prefer 
to use `__APPLE__` could you leave a comment with something like

```
// TODO(dliew): Don't use unclear `SANITIZER_MAC` here. Instead wait for its 
replacement rdar://problem/58124919.
```

Note the `TODO():` style is enforced by the sanitizer specific linter so 
you have to put a name there.


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

https://reviews.llvm.org/D71491



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

[PATCH] D71491: [ubsan] Check implicit casts in ObjC for-in statements

[Dan Liew via Phabricator via cfe-commits]

delcypher added inline comments.



Comment at: compiler-rt/lib/ubsan/ubsan_value.cpp:29
+const char *__ubsan::getObjCClassName(ValueHandle Pointer) {
+#if defined(__APPLE__)
+  // We need to query the ObjC runtime for some information, but do not want

vsk wrote:
> delcypher wrote:
> > The compiler-rt codebase tends to use `SANITIZER_MAC` macro (defined to be 
> > 1 if Apple otherwise it's 0) rather than `__APPLE__`.
> I see. That seems problematic, as it makes it tricky to add macOS-specific 
> (or iOS-specific) functionality down the road. I don't think SANITIZER_MAC 
> should be defined unless TARGET_OS_MACOS is true.
Sorry I should clarify. `SANITIZER_MAC` is poorly named but it is defined to be 
`1` for Apple platforms and `0`. I'm just pointing out the convention that 
exists today. You're absolutely right that we might want to do different things 
for different Apple platforms but I don't think we want to start doing a mass 
re-name until arm64e and arm64_32 support are completed landed in llvm's master.


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

https://reviews.llvm.org/D71491



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

[PATCH] D71491: [ubsan] Check implicit casts in ObjC for-in statements

[Dan Liew via Phabricator via cfe-commits]

delcypher added inline comments.



Comment at: clang/lib/Driver/ToolChain.cpp:953
+  SanitizerKind::ImplicitConversion | SanitizerKind::Nullability |
+  SanitizerKind::LocalBounds | SanitizerKind::ObjCCast;
   if (getTriple().getArch() == llvm::Triple::x86 ||

`SanitizerKind::ObjCCast` doesn't seem to fit the comment above. It is platform 
dependent (only really works for Apple platforms) and it **does** require 
runtime support (i.e. the Objective-C runtime).


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

https://reviews.llvm.org/D71491



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

[PATCH] D71491: [ubsan] Check implicit casts in ObjC for-in statements

[Dan Liew via Phabricator via cfe-commits]

delcypher added inline comments.



Comment at: compiler-rt/lib/ubsan/ubsan_value.cpp:29
+const char *__ubsan::getObjCClassName(ValueHandle Pointer) {
+#if defined(__APPLE__)
+  // We need to query the ObjC runtime for some information, but do not want

The compiler-rt codebase tends to use `SANITIZER_MAC` macro (defined to be 1 if 
Apple otherwise it's 0) rather than `__APPLE__`.



Comment at: compiler-rt/lib/ubsan/ubsan_value.cpp:37
+
+  if (!AttemptedDlopen) {
+ObjCHandle = dlopen(

You might want some sort of lock here (or atomic variable) to ensure we don't 
race and try to `dlopen()` multiple times.


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

https://reviews.llvm.org/D71491



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