[PATCH] D153156: [Clang] CWG1473: do not err on the lack of space after operator""

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D153156#4600270 , @rupprecht wrote:

> of which there's an example in LLVM itself: 
> https://github.com/llvm/llvm-project/blob/6a0e536ccfef1f7bd64ee4153b4efc0aeecf28d4/clang/test/SemaCXX/cxx11-compat.cpp#L38

Sorry, I don't want to mislead: I just mean there's a handy example in clang's 
unit tests. I don't see any instances of this in LLVM non-test code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153156

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

[PATCH] D153156: [Clang] CWG1473: do not err on the lack of space after operator""

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

The other common breakage I'm seeing is code that hasn't yet migrated from the 
"PRI" format macros, of which there's an example in LLVM itself: 
https://github.com/llvm/llvm-project/blob/6a0e536ccfef1f7bd64ee4153b4efc0aeecf28d4/clang/test/SemaCXX/cxx11-compat.cpp#L38

e.g.: https://godbolt.org/z/v3boc9E6T

> IMHO, people should stop using -Wno-reserved-user-defined-literal and 
> exploiting the addition of a whitespace to mingle pre-c++11 and post-c++11 
> code.

I mostly agree with this, at least the spirit of it, but do we usually remove 
the `-Wno` ability to suppress a common error because we don't like it? (Or 
justify it post-facto if we accidentally did that?) If removing this is 
important for some other aspect of clang development, it would be nice to have 
notice ahead of time that this option will go away. I guess +1 to what James 
said.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153156

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

[PATCH] D153156: [Clang] CWG1473: do not err on the lack of space after operator""

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D153156#4599106 , @aaron.ballman 
wrote:

> In D153156#4598988 , @rZhBoYao 
> wrote:
>
>> In D153156#4598915 , 
>> @steelannelida wrote:
>>
>>> Unfortunately the option -Wno-reserved-user-defined-literal fails after 
>>> this:
>>>
>>>   #define MYTHING "_something_"
>>>   
>>>   const char* f() {
>>> return "ONE"MYTHING"TWO";
>>>   }
>>>
>>>   $ clang -Wno-reserved-user-defined-literal repro.cxx
>>>   repro.cxx:4:15: error: no matching literal operator for call to 
>>> 'operator""MYTHING' with arguments of types 'const char *' and 'unsigned 
>>> long', and no matching literal operator template
>>>   4 |   return "ONE"MYTHING"TWO";
>>> |   ^
>>>   1 error generated.
>>
>> This is conforming right? Correct me if I'm wrong. My reading of 
>> https://eel.is/c++draft/lex.pptoken#3.3 is that "ONE"MYTHING"TWO" is a 
>> single preprocessing-token during phase 3 
>> (https://eel.is/c++draft/lex.phases#1.3). Can @aaron.ballman confirm this?
>
> The diagnostic behavior is correct. `MYTHING` doesn't get expanded until 
> phase 4 (http://eel.is/c++draft/lex.phases#1.4), so this appears as 
> `"ONE"MYTHING` as a single preprocessor token: 
> https://eel.is/c++draft/lex.ext#nt:user-defined-string-literal and that token 
> is an invalid UDL.

IIUC, the question is not whether the diagnostic is correct, but rather why 
`-Wno-reserved-user-defined-literal` does not workaround the breakage. Is that 
right?

An example of this in the wild is older versions of swig: 
https://github.com/swig/swig/blob/939dd5e1c8c17e5f8b38747bf18e9041ab5f377e/Source/Modules/php.cxx#L1724


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153156

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

[PATCH] D154014: [SpecialCaseList] Use Globs instead of Regex

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D154014#4457883 , @MaskRay wrote:

>> This is a breaking change since some SCLs might use .* or (abc|def) which 
>> are supported regexes but not valid globs. Since we have just cut clang 16.x 
>> this is a good time to make this change.
>
> My user has some ignore lists, but there is no `^[a-z]+:.*\(` or `^[a-z]+:\.` 
> occurrence, so this change is likely safe for us.

I think I'm looking at the same lists, and I see plenty of `.*` in the 
sanitizer exclusion lists. Also a few cases of `(abc|def|...)`. IIUC, those 
would both be broken by this -- instead of `.*` meaning "any character any 
number of times" (regex) it would mean "dot followed by any number of 
characters" (glob), right? And the `(abc|...)` would just be that literal text, 
not matching the individual parts?

If my understanding of that is correct, I don't think this is a good change -- 
there's possibly plenty of configs out there that assume this is regex, and 
there doesn't seem to be sufficient motivation to just break those. But I can 
see that globs are useful for the examples posted in the patch description. Is 
it possible to have some middle ground, e.g. default to regex but allow a 
config at the top of sanitizer lists to interpret future patterns as globs 
instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154014

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

[PATCH] D150226: [Clang] Remove ability to downgrade warning on the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D150226#4401188 , @jyknight wrote:

> In D150226#4400782 , @rupprecht 
> wrote:
>
>> As a general question/feature request: is there a way to have specific 
>> warnings apply even for system headers? It would be nice if I could check 
>> what breaks when by adding `-Wsystem-error=enum-constexpr-conversion` to the 
>> global build flags. Rebuilding clang w/ this patched in also works, but is a 
>> little more difficult/noisy.
>
> I think there's no per-warning flag (you can turn on _all_ of the enabled 
> warnings in system headers with `-Wsystem-headers`), but by modifying the 
> clang sources you can add `ShowInSystemHeader` to the diagnostic.
> E.g.
>
>def warn_constexpr_unscoped_enum_out_of_range : Warning<
>  "integer value %0 is outside the valid range of values [%1, %2] for this 
> "
>   -  "enumeration type">, DefaultError, 
> InGroup>;
>   +  "enumeration type">, DefaultError, 
> InGroup>, ShowInSystemHeader;

Thanks for that pointer. It's definitely a lot simpler/general to do that than 
to patch a change like this in, but still requires rebuilding clang. My most 
recent attempt didn't find anything `-Wenum-constexpr-conversion` related, but 
did find a bunch of other breakages that I assume are related to other recent 
clang changes (usually existing issues with the code that need to be cleaned 
up, but maybe a few are clang bugs). The fact that `ShowInSystemHeader` already 
exists makes me hopeful that it isn't too hard to support this, so I filed 
https://github.com/llvm/llvm-project/issues/63180.

I suppose including this warning in `ShowInSystemHeader` with your diff above 
would be a good first step anyway, right? If we're about to make it a hard 
error anyway, then making it a `ShowInSystemHeader` error first would ease that 
transition.


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

https://reviews.llvm.org/D150226

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

[PATCH] D150226: [Clang] Remove ability to downgrade warning on the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D150226#4353907 , @aaron.ballman 
wrote:

> In D150226#4353863 , @jyknight 
> wrote:
>
>> When looking for errors in existing codebases, don't forget that this 
>> diagnostic is currently suppressed by default in system headers. So this 
>> patch is moving from "no diagnostics for code in system headers" to 
>> "unconditional hard error in system headers". Just removing 
>> `-Wno-enum-constexpr-conversion` from your build flags is insufficient to 
>> test that this patch won't break code!
>>
>> I haven't done any tests, but I'm rather skeptical that this change is going 
>> to be viable without breaking stuff, still.
>
> +1 to needing to test against system headers, but also: if we find any system 
> headers that would be broken by this, we should proactively alert the owners 
> of those headers so that they understand there's urgency to getting the fixes 
> into their headers so that the entire ecosystem isn't held back. 
> Alternatively, if it's just one problematic system header in an LTS release 
> somewhere, we could perhaps put in a compat hack for just that header so we 
> can move forward.

As a general question/feature request: is there a way to have specific warnings 
apply even for system headers? It would be nice if I could check what breaks 
when by adding `-Wsystem-error=enum-constexpr-conversion` to the global build 
flags. Rebuilding clang w/ this patched in also works, but is a little more 
difficult/noisy.


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

https://reviews.llvm.org/D150226

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

[PATCH] D150875: Make dereferencing a void* a hard-error instead of warn-as-error

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

I found one use here: 
https://github.com/ericniebler/range-v3/blob/48dc2eb666c07e6afc8ec5edf7db9a5c6cde7a56/include/range/v3/functional/invoke.hpp#L51


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

https://reviews.llvm.org/D150875

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

[PATCH] D150226: [Clang] Remove ability to downgrade warning on the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D150226#4338264 , @shafik wrote:

> In D150226#4336755 , @rupprecht 
> wrote:
>
>> We're still using `-Wno-enum-constexpr-conversion`, although I'm not sure if 
>> we need that or if we just forgot to remove it after doing some cleanup. I'm 
>> trying it out now. (Sorry, I'm not sure we were aware that having a way to 
>> turn this off was just temporary).
>
> Please let me know how things look on your end.

It's not that bad, and all remaining breakages have fixes sent out now, but 
still waiting to land. A week or two is probably good enough for us. But I 
don't know about the rest of OSS mentioned in other comments.

(FYI, I'll be out for a few weeks, and I might not have a chance to follow up 
before then)


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

https://reviews.llvm.org/D150226

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

[PATCH] D150226: [Clang] Remove ability to downgrade warning on the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

We're still using `-Wno-enum-constexpr-conversion`, although I'm not sure if we 
need that or if we just forgot to remove it after doing some cleanup. I'm 
trying it out now. (Sorry, I'm not sure we were aware that having a way to turn 
this off was just temporary).

BTW, LLVM itself still uses this flag in openmp: 
https://github.com/llvm/llvm-project/blob/c2ce2a509f74a85a3c0ef4b9d6d79fbacc7e8bdf/openmp/cmake/HandleOpenMPOptions.cmake#L34


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

https://reviews.llvm.org/D150226

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

[PATCH] D146987: [Assignment Tracking] Enable by default

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D146987#4263048 , @MaskRay wrote:

> The latest reland 3820e9a2b29a2e268319ed6635da0d59e18d736d 
>  still 
> caused some issues to our internal workloads. Stay tuned...
>
> So far we have only found crashes with ThinLTO workloads.

More specifically, thinlto + split debug.

I'm not sure if my reduction is the same issue as @aeubanks has. Here's mine: 
F27123766: repro-D146987.ll 

Repro w/ `clang -O1 -c reduced.ll -o /dev/null`

I don't see any assertions going off for this one. The stack trace I have just 
points to `llvm::Value::stripPointerCasts`. Lemme know if you need more info, 
but I'm hoping that file reproduces for you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146987

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

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D144285#4163004 , @cor3ntin wrote:

> If however we find this change to disruptive, we should inform WG21.

Thanks for the explanation, I think I understand the issue now. I got totally 
thrown off by the title -- it's not about literally writing 
`static_assert(false)`, it's about deferring `static_assert` evaluation to 
template instantiations. Being able to write `static_assert(false)` (or any 
falsy constant expression) is just the common use case for this.

So possibly the most trivial example of something that used to break, but now 
builds:

  template 
  void Fail() {
  static_assert(false, "my assertion failed");
  }

... but will still fail as soon as you invoke `Fail()` somewhere.

It doesn't look like there's a lot of impact from this, and the breakages are 
corner cases like this. It might be worth mentioning this one case to WG21 but 
I'm not sure what I would change about the wording.




Comment at: clang/www/cxx_dr_status.html:14915
   
 https://wg21.link/cwg2518;>2518
 review

Is it possible to make this link point to 
https://cplusplus.github.io/CWG/issues/2518.html? This link is inaccessible to 
anyone not on ISO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144285

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

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

Here's one change this patch causes on "real" code (invalid code, but something 
a user might try to compile): we see is a static_assert in gmock that now fails 
to report a useful error message: https://godbolt.org/z/sPr1PYT9d

Previously we saw `error: static assertion failed due to requirement 
<...snip...>: This method does not take 2 arguments. Parenthesize all types 
with unprotected commas.`. It still fails to compile, but without the nice 
error message.

My guess, although I haven't looked too closely, is that it's due to the 
`static_assert(false)` here: 
https://github.com/google/googletest/blob/2d4f208765af7fa376b878860a7677ecc0bc390a/googlemock/include/gmock/gmock-function-mocker.h#L149


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144285

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

[PATCH] D143840: [clang] Add the check of membership for the issue #58674 and improve the lookup process

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

This has already been reverted, but I found a breakage (not a crash) caused by 
this:

  #include 
  
  class Base {
   protected:
int member;
  };
  
  template 
  struct Subclass : public Parent {
static_assert(std::is_base_of::value,
  "Parent not derived from Base");
int func() { return Base::member; }
  };
  
  using Impl = Subclass;
  
  int use() {
Impl i;
return i.func();
  }

gcc/msvc both accept this. But clang says:

  :8:29: error: 'member' is a protected member of 'Base'
int func() { return Base::member; }
  ^
  :15:12: note: in instantiation of member function 
'Subclass::func' requested here
return i.func();
 ^
  :3:7: note: must name member using the type of the current context 
'Subclass'
int member;
^

A fix is to write `Parent::member` instead of `Base::member`, but I'm wondering 
what the spec actually says about this, and if clang is right to reject it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143840

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

[PATCH] D124351: [Clang] Implement Change scope of lambda trailing-return-type

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht accepted this revision as: rupprecht.
rupprecht added a comment.

All the things that were broken before are no longer broken, so LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124351

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

[PATCH] D124351: [Clang] Implement Change scope of lambda trailing-return-type

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D124351#4102679 , @aaron.ballman 
wrote:

> In D124351#4102634 , @eaeltsin 
> wrote:
>
>>> Looks like we fail to enter the appropriate context somewhere (my guess is 
>>> that it might be specific to the attribute but it's hard to say without 
>>> picking around), definitely a bug
>>>
>>> I'll be away the next 2 weeks, I'll look at it when I get back. Maybe we 
>>> should track it in an issue though.
>>
>> Should we revert this then?
>
> Yes, I think we should. We should also file an issue so @cor3ntin doesn't 
> lose track of the issue. Anyone want to volunteer to do that? (I can do it 
> early next week otherwise.)

Done in 74ce297045bac4bc475b8e762d2a1ea19bb16d3c 
 and filed 
https://github.com/llvm/llvm-project/issues/60518 to track re-landing this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124351

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

[PATCH] D124351: [Clang] Implement Change scope of lambda trailing-return-type

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

Hi, me again :)

I ran into an interesting build breakage from this, I can't tell if it's a 
legitimate breakage based on reading P2036R3 and P2579R0 (mostly I'm not a 
language lawyer).

  struct StringLiteral {
template 
StringLiteral(const char ()[N])
__attribute__((enable_if(N > 0 && N == __builtin_strlen(array) + 1,
 "invalid string literal")));
  };
  
  struct Message {
Message(StringLiteral);
  };
  
  void Func1() {
auto x = Message("x");  // Note: this is fine
  
// Note: "xx\0" to force a different type, StringLiteral<3>, otherwise this
// successfully builds.
auto y = [&](decltype(Message("xx"))) {};
  
// ^ fails with: repro.cc:18:13: error: reference to local variable 'array'
// declared in enclosing function 'StringLiteral::StringLiteral<3>'
  
(void)x;
(void)y;
  }

https://godbolt.org/z/M4E6jKxxn

Does this look like an intended breakage from this patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124351

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

[PATCH] D142449: [clang] Fix linking to LLVMTestingAnnotations in standalone build

[Jordan Rupprecht via Phabricator via cfe-commits]

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

Thanks!


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

https://reviews.llvm.org/D142449

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

[PATCH] D141175: [test] Split out Annotations from `TestingSupport`

[Jordan Rupprecht 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 rG3432f4bf86e7: [test] Split out Annotations from 
`TestingSupport` (authored by rupprecht).

Changed prior to commit:
  https://reviews.llvm.org/D141175?vs=488381=488762#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141175

Files:
  clang-tools-extra/clangd/unittests/Annotations.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/FindTargetTests.cpp
  clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
  clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
  clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
  clang-tools-extra/include-cleaner/unittests/CMakeLists.txt
  clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp
  clang-tools-extra/include-cleaner/unittests/LocateSymbolTest.cpp
  clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
  clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
  clang-tools-extra/pseudo/unittests/BracketTest.cpp
  clang-tools-extra/pseudo/unittests/CMakeLists.txt
  clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
  clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp
  clang/docs/tools/clang-formatted-files.txt
  clang/unittests/AST/CMakeLists.txt
  clang/unittests/AST/DeclTest.cpp
  clang/unittests/AST/SourceLocationTest.cpp
  clang/unittests/Analysis/FlowSensitive/CMakeLists.txt
  clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp
  clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp
  clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp
  clang/unittests/Analysis/FlowSensitive/TestingSupport.h
  clang/unittests/Analysis/FlowSensitive/TransferBranchTest.cpp
  clang/unittests/Sema/CMakeLists.txt
  clang/unittests/Sema/CodeCompleteTest.cpp
  clang/unittests/Tooling/CMakeLists.txt
  clang/unittests/Tooling/SourceCodeTest.cpp
  clang/unittests/Tooling/Syntax/CMakeLists.txt
  clang/unittests/Tooling/Syntax/TokensTest.cpp
  clang/unittests/Tooling/Syntax/TreeTestBase.cpp
  clang/unittests/Tooling/Syntax/TreeTestBase.h
  llvm/include/llvm/Testing/Annotations/Annotations.h
  llvm/include/llvm/Testing/Support/Annotations.h
  llvm/lib/Testing/Annotations/Annotations.cpp
  llvm/lib/Testing/Annotations/CMakeLists.txt
  llvm/lib/Testing/CMakeLists.txt
  llvm/lib/Testing/Support/Annotations.cpp
  llvm/lib/Testing/Support/CMakeLists.txt
  llvm/unittests/Support/AnnotationsTest.cpp
  llvm/unittests/Support/CMakeLists.txt
  llvm/unittests/Testing/Annotations/AnnotationsTest.cpp
  llvm/unittests/Testing/Annotations/CMakeLists.txt
  llvm/unittests/Testing/CMakeLists.txt
  utils/bazel/llvm-project-overlay/clang/unittests/BUILD.bazel
  utils/bazel/llvm-project-overlay/llvm/BUILD.bazel

Index: utils/bazel/llvm-project-overlay/llvm/BUILD.bazel
===
--- utils/bazel/llvm-project-overlay/llvm/BUILD.bazel
+++ utils/bazel/llvm-project-overlay/llvm/BUILD.bazel
@@ -4516,9 +4516,7 @@
 "lib/Testing/Support/*.cpp",
 "lib/Testing/Support/*.h",
 ]),
-hdrs = glob([
-"include/llvm/Testing/Support/*.h",
-]),
+hdrs = glob(["include/llvm/Testing/Support/*.h"]),
 copts = llvm_copts,
 deps = [
 ":Support",
@@ -4528,6 +4526,15 @@
 ],
 )
 
+cc_library(
+name = "TestingAnnotations",
+testonly = True,
+srcs = ["lib/Testing/Annotations/Annotations.cpp"],
+hdrs = ["include/llvm/Testing/Annotations/Annotations.h"],
+copts = llvm_copts,
+deps = [":Support"],
+)
+
 
 # Begin testonly binary utilities
 
Index: utils/bazel/llvm-project-overlay/clang/unittests/BUILD.bazel
===
--- utils/bazel/llvm-project-overlay/clang/unittests/BUILD.bazel
+++ utils/bazel/llvm-project-overlay/clang/unittests/BUILD.bazel
@@ -29,6 +29,7 @@
 "//clang:tooling",
 "//llvm:Core",
 "//llvm:Support",
+"//llvm:TestingAnnotations",
 "//llvm:TestingSupport",
 "//third-party/unittest:gmock",
 "//third-party/unittest:gtest",
@@ -136,6 +137,7 @@
 "//clang:tooling",
 "//llvm:Support",
 "//llvm:TestingADT",
+"//llvm:TestingAnnotations",
 "//llvm:TestingSupport",
 "//third-party/unittest:gmock",
 "//third-party/unittest:gtest",
@@ -342,6 +344,7 @@
 "//clang:parse",
 "//clang:sema",
 "//clang:tooling",
+"//llvm:TestingAnnotations",
 "//llvm:TestingSupport",
 "//third-party/unittest:gmock",
 "//third-party/unittest:gtest",
@@ -420,6 +423,7 @@
 "//clang:tooling_refactoring",
  

[PATCH] D141175: [test] Split out Annotations from `TestingSupport`

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht updated this revision to Diff 488381.
rupprecht added a comment.

- Remove redundant comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141175

Files:
  clang-tools-extra/clangd/unittests/Annotations.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/FindTargetTests.cpp
  clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
  clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
  clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
  clang-tools-extra/include-cleaner/unittests/CMakeLists.txt
  clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp
  clang-tools-extra/include-cleaner/unittests/LocateSymbolTest.cpp
  clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
  clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
  clang-tools-extra/pseudo/unittests/BracketTest.cpp
  clang-tools-extra/pseudo/unittests/CMakeLists.txt
  clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
  clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp
  clang/docs/tools/clang-formatted-files.txt
  clang/unittests/AST/CMakeLists.txt
  clang/unittests/AST/DeclTest.cpp
  clang/unittests/AST/SourceLocationTest.cpp
  clang/unittests/Analysis/FlowSensitive/CMakeLists.txt
  clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp
  clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp
  clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp
  clang/unittests/Analysis/FlowSensitive/TestingSupport.h
  clang/unittests/Analysis/FlowSensitive/TransferBranchTest.cpp
  clang/unittests/Sema/CMakeLists.txt
  clang/unittests/Sema/CodeCompleteTest.cpp
  clang/unittests/Tooling/CMakeLists.txt
  clang/unittests/Tooling/SourceCodeTest.cpp
  clang/unittests/Tooling/Syntax/CMakeLists.txt
  clang/unittests/Tooling/Syntax/TokensTest.cpp
  clang/unittests/Tooling/Syntax/TreeTestBase.cpp
  clang/unittests/Tooling/Syntax/TreeTestBase.h
  llvm/include/llvm/Testing/Annotations/Annotations.h
  llvm/include/llvm/Testing/Support/Annotations.h
  llvm/lib/Testing/Annotations/Annotations.cpp
  llvm/lib/Testing/Annotations/CMakeLists.txt
  llvm/lib/Testing/CMakeLists.txt
  llvm/lib/Testing/Support/Annotations.cpp
  llvm/lib/Testing/Support/CMakeLists.txt
  llvm/unittests/Support/AnnotationsTest.cpp
  llvm/unittests/Support/CMakeLists.txt
  llvm/unittests/Testing/Annotations/AnnotationsTest.cpp
  llvm/unittests/Testing/Annotations/CMakeLists.txt
  llvm/unittests/Testing/CMakeLists.txt
  utils/bazel/llvm-project-overlay/llvm/BUILD.bazel

Index: utils/bazel/llvm-project-overlay/llvm/BUILD.bazel
===
--- utils/bazel/llvm-project-overlay/llvm/BUILD.bazel
+++ utils/bazel/llvm-project-overlay/llvm/BUILD.bazel
@@ -4450,18 +4450,26 @@
 "lib/Testing/Support/*.cpp",
 "lib/Testing/Support/*.h",
 ]),
-hdrs = glob([
-"include/llvm/Testing/Support/*.h",
-]),
+hdrs = glob(["include/llvm/Testing/Support/*.h"]),
 copts = llvm_copts,
 deps = [
 ":Support",
+":TestingAnnotations",
 ":config",
 "//third-party/unittest:gmock",
 "//third-party/unittest:gtest",
 ],
 )
 
+cc_library(
+name = "TestingAnnotations",
+testonly = True,
+srcs = ["lib/Testing/Annotations/Annotations.cpp"],
+hdrs = ["include/llvm/Testing/Annotations/Annotations.h"],
+copts = llvm_copts,
+deps = [":Support"],
+)
+
 
 # Begin testonly binary utilities
 
Index: llvm/unittests/Testing/CMakeLists.txt
===
--- llvm/unittests/Testing/CMakeLists.txt
+++ llvm/unittests/Testing/CMakeLists.txt
@@ -1,2 +1,3 @@
 add_subdirectory(ADT)
+add_subdirectory(Annotations)
 add_subdirectory(Support)
Index: llvm/unittests/Testing/Annotations/CMakeLists.txt
===
--- /dev/null
+++ llvm/unittests/Testing/Annotations/CMakeLists.txt
@@ -0,0 +1,10 @@
+set(LLVM_LINK_COMPONENTS
+  Support
+  TestingAnnotations
+  )
+
+add_llvm_unittest(TestingAnnotationTests
+  AnnotationsTest.cpp
+  )
+
+target_link_libraries(TestingAnnotationTests PRIVATE LLVMTestingAnnotations)
Index: llvm/unittests/Testing/Annotations/AnnotationsTest.cpp
===
--- llvm/unittests/Testing/Annotations/AnnotationsTest.cpp
+++ llvm/unittests/Testing/Annotations/AnnotationsTest.cpp
@@ -5,7 +5,7 @@
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===--===//
-#include "llvm/Testing/Support/Annotations.h"
+#include "llvm/Testing/Annotations/Annotations.h"
 #include "gmock/gmock.h"
 

[PATCH] D141175: [bazel] Split out Annotations from `TestingSupport`

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D141175#4038017 , @GMNGeoffrey 
wrote:

> It seems like the same logic would extend to the CMake build. Could we make 
> the same change there?

The simplest (only?) way to do that is to have it literally in a separate 
directory, so I did that. It's a bit large now but mechanical.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141175

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

[PATCH] D141175: [bazel] Split out Annotations from `TestingSupport`

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht updated this revision to Diff 488366.
rupprecht added a comment.
Herald added subscribers: cfe-commits, kadircet, arphaman, hiraditya.
Herald added projects: clang, clang-tools-extra.

- Move annotations to a separate package entirely


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141175

Files:
  clang-tools-extra/clangd/unittests/Annotations.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/FindTargetTests.cpp
  clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
  clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
  clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
  clang-tools-extra/include-cleaner/unittests/CMakeLists.txt
  clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp
  clang-tools-extra/include-cleaner/unittests/LocateSymbolTest.cpp
  clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
  clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
  clang-tools-extra/pseudo/unittests/BracketTest.cpp
  clang-tools-extra/pseudo/unittests/CMakeLists.txt
  clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
  clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp
  clang/docs/tools/clang-formatted-files.txt
  clang/unittests/AST/CMakeLists.txt
  clang/unittests/AST/DeclTest.cpp
  clang/unittests/AST/SourceLocationTest.cpp
  clang/unittests/Analysis/FlowSensitive/CMakeLists.txt
  clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp
  clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp
  clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp
  clang/unittests/Analysis/FlowSensitive/TestingSupport.h
  clang/unittests/Analysis/FlowSensitive/TransferBranchTest.cpp
  clang/unittests/Sema/CMakeLists.txt
  clang/unittests/Sema/CodeCompleteTest.cpp
  clang/unittests/Tooling/CMakeLists.txt
  clang/unittests/Tooling/SourceCodeTest.cpp
  clang/unittests/Tooling/Syntax/CMakeLists.txt
  clang/unittests/Tooling/Syntax/TokensTest.cpp
  clang/unittests/Tooling/Syntax/TreeTestBase.cpp
  clang/unittests/Tooling/Syntax/TreeTestBase.h
  llvm/include/llvm/Testing/Annotations/Annotations.h
  llvm/include/llvm/Testing/Support/Annotations.h
  llvm/lib/Testing/Annotations/Annotations.cpp
  llvm/lib/Testing/Annotations/CMakeLists.txt
  llvm/lib/Testing/CMakeLists.txt
  llvm/lib/Testing/Support/Annotations.cpp
  llvm/lib/Testing/Support/CMakeLists.txt
  llvm/unittests/Support/AnnotationsTest.cpp
  llvm/unittests/Support/CMakeLists.txt
  llvm/unittests/Testing/Annotations/AnnotationsTest.cpp
  llvm/unittests/Testing/Annotations/CMakeLists.txt
  llvm/unittests/Testing/CMakeLists.txt
  utils/bazel/llvm-project-overlay/llvm/BUILD.bazel

Index: utils/bazel/llvm-project-overlay/llvm/BUILD.bazel
===
--- utils/bazel/llvm-project-overlay/llvm/BUILD.bazel
+++ utils/bazel/llvm-project-overlay/llvm/BUILD.bazel
@@ -4450,18 +4450,27 @@
 "lib/Testing/Support/*.cpp",
 "lib/Testing/Support/*.h",
 ]),
-hdrs = glob([
-"include/llvm/Testing/Support/*.h",
-]),
+hdrs = glob(["include/llvm/Testing/Support/*.h"]),
 copts = llvm_copts,
 deps = [
 ":Support",
+":TestingAnnotations",
 ":config",
 "//third-party/unittest:gmock",
 "//third-party/unittest:gtest",
 ],
 )
 
+# Split out of TestingSupport to allow using this with a different gmock/gtest version.
+cc_library(
+name = "TestingAnnotations",
+testonly = True,
+srcs = ["lib/Testing/Annotations/Annotations.cpp"],
+hdrs = ["include/llvm/Testing/Annotations/Annotations.h"],
+copts = llvm_copts,
+deps = [":Support"],
+)
+
 
 # Begin testonly binary utilities
 
Index: llvm/unittests/Testing/CMakeLists.txt
===
--- llvm/unittests/Testing/CMakeLists.txt
+++ llvm/unittests/Testing/CMakeLists.txt
@@ -1,2 +1,3 @@
 add_subdirectory(ADT)
+add_subdirectory(Annotations)
 add_subdirectory(Support)
Index: llvm/unittests/Testing/Annotations/CMakeLists.txt
===
--- /dev/null
+++ llvm/unittests/Testing/Annotations/CMakeLists.txt
@@ -0,0 +1,10 @@
+set(LLVM_LINK_COMPONENTS
+  Support
+  TestingAnnotations
+  )
+
+add_llvm_unittest(TestingAnnotationTests
+  AnnotationsTest.cpp
+  )
+
+target_link_libraries(TestingAnnotationTests PRIVATE LLVMTestingAnnotations)
Index: llvm/unittests/Testing/Annotations/AnnotationsTest.cpp
===
--- llvm/unittests/Testing/Annotations/AnnotationsTest.cpp
+++ llvm/unittests/Testing/Annotations/AnnotationsTest.cpp
@@ -5,7 +5,7 @@
 // SPDX-License-Identifier: Apache-2.0 

[PATCH] D136554: Implement CWG2631

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D136554#4024628 , @rupprecht wrote:

> I still see one behavior change (actually it was there before, but I missed 
> it in the test results), but as far as I can tell, it's a good one? If I 
> reduce it too much, I get the warning with the baseline toolchain, so it's 
> not erroneous AFAICT. Although I won't pretend I know all the intricacies of 
> `static` and `inline`.

I missed another place in the unreduced repro that was referencing this method, 
and that was the trick. I reduced the newly-enabled warning case to this:

  // a.h
  static const std::pair& GetFakePairRef() {
static constexpr std::pair fake = {1.0, 2.0};
return fake;
  }
  
  struct Metadata {
const std::pair& data = GetFakePairRef();
  };
  
  // main.cc, compiled with -Wunused-function
  #include "a.h"
  int main() { return 0; }

Again, still LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136554

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

[PATCH] D136554: Implement CWG2631

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht accepted this revision.
rupprecht added a comment.

I still see one behavior change (actually it was there before, but I missed it 
in the test results), but as far as I can tell, it's a good one? If I reduce it 
too much, I get the warning with the baseline toolchain, so it's not erroneous 
AFAICT. Although I won't pretend I know all the intricacies of `static` and 
`inline`.

  // a.h
  static const std::pair& GetFakePair() {
static constexpr std::pair kFakePair = {123.0, 456.0};
return kFakePair;
  }
  
  // b.h
  #include "a.h"
  
  class X {
void Foo(..., const std::pair& x = GetFakePair()) const;
  };
  
  // main.cc, compiled with -Wunused-function
  #include "b.h"
  int main() { return 0; }

Yields this error:

  In file included from main.cc:9:
  In file included from ./b.h:16:
  ./a.h:40:41: error: 'static' function 'GetFakePair' declared in header file 
should be declared 'static inline' [-Werror,-Wunneeded-internal-declaration]
  static const std::pair& GetFakePair() {

Unless I'm missing something, it seems that, in regards to this warning, this 
change is just enabling an existing warning to have more coverage, and 
therefore this change looks ready to ship. Thanks for letting me provide all my 
repros before landing this one again :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136554

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

[PATCH] D136554: Implement CWG2631

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

I threw this at the "test everything" test (some millions of targets) and it 
found only one breakage, so this is very very close. Without further ado, here 
is this silly looking thing:

File `blah.h`:

  #include 
  #include 
  
  template 
  struct Base {
virtual ~Base() = default;
  };
  
  class Impl : public Base {};
  
  struct ImplHolder {
std::unique_ptr> impl = std::make_unique();
  };
  
  struct GetImplCreators {
std::function impl_creator = []() { return ImplHolder{}; };
  };
  
  void UseImpl(GetImplCreators impl_creators = GetImplCreators{});

And `blah.cc`:

  #include "blah.h"
  
  void UseImpl(GetImplCreators impl_creators) {}

And lastly, `blah_main.cc`:

  #include "blah.h"
  
  void Func1() { UseImpl(); }
  // Note: it also fails when we explicitly specify the default arg, in case 
that is interesting.
  // void Func2() { UseImpl(GetImplCreators()); }
  
  int main(int argc, char* argv[]) { return 0; }

And here's the LLD output:

  ld: error: undefined hidden symbol: 
std::__u::__unique_if::__unique_single std::__u::make_unique()
  >>> referenced by blah_main.cc
  >>>   blah_main.o:(ImplHolder 
std::__u::__function::__policy_invoker::__call_impl>(std::__u::__function::__policy_storage const*))
  
  ld: error: undefined hidden symbol: std::__u::unique_ptr>::~unique_ptr()
  >>> referenced by blah_main.cc
  >>>   blah_main.o:(ImplHolder 
std::__u::__function::__policy_invoker::__call_impl>(std::__u::__function::__policy_storage const*))

I'm out of time today so I don't have a repro script to offer, but I can dig 
into that when I get back next year if the source-only example is insufficient.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136554

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

[PATCH] D136554: Implement CWG2631

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

I'm not sure what to make of the new failure when I try it out this time. Given 
a source like this:

  #include 
  
  struct Options {
std::function identity = [](bool x) -> bool { return x; };
  };
  
  struct Wrapper {
explicit Wrapper(const Options& options = Options()) {}
  };
  
  void Func() { Options options; }

The object file reports many fewer symbols as a result of this patch, but one 
new problematic one. Before:

   T Func()
   W 
std::__1::__function::__func, bool 
(bool)>::target_type() const
   W 
std::__1::__function::__func, bool 
(bool)>::target(std::type_info const&) const
   W 
std::__1::__function::__func, bool 
(bool)>::__clone(std::__1::__function::__base*) const
   W 
std::__1::__function::__func, bool (bool)>::__clone() 
const
   W std::__1::__function::__base::~__base[abi:v16]()
   W 
std::__1::__function::__func, bool 
(bool)>::destroy_deallocate()
   W 
std::__1::__function::__func, bool (bool)>::destroy()
   W 
std::__1::__function::__func, bool (bool)>::~__func()
   W 
std::__1::__function::__func, bool 
(bool)>::operator()(bool&&)
   V typeinfo for Options::identity::'lambda'(bool)
   V typeinfo for std::__1::__function::__base
   V typeinfo for 
std::__1::__function::__func, bool (bool)>
   V typeinfo name for Options::identity::'lambda'(bool)
   V typeinfo name for std::__1::__function::__base
   V typeinfo name for 
std::__1::__function::__func, bool (bool)>
   U vtable for __cxxabiv1::__class_type_info
   U vtable for __cxxabiv1::__si_class_type_info
   V vtable for 
std::__1::__function::__func, bool (bool)>
   U operator delete(void*)
   U operator new(unsigned long)

And after:

   T Func()
   U std::__1::function::function(Options::identity::'lambda'(bool))

The undefined symbols before are all provided by libc++, so those are fine. 
After, the new undefined symbol for the lambda cannot be resolved. Depending on 
how the linker is invoked, this may or may not be fine -- IIUC the linker can 
sometimes recognize that it doesn't actually need the undefined symbol, so it 
doesn't matter if it can't find it.

Here is the build script I'm using, although you will likely need to tweak it 
for your own environment:

  ${CLANG} \
-std=gnu++17 -stdlib=libc++ \
-c main.cc \
-o /tmp/main.o
  
  echo "main.o symbols:"
  llvm-nm -C /tmp/main.o
  
  echo Building b.cc
  
  ${CLANG} \
-std=gnu++17 -stdlib=libc++ \
-O1 -fno-exceptions \
-c b.cc \
-o /tmp/b.o
  
  echo "b.o symbols:"
  llvm-nm -C /tmp/b.o
  
  echo Linking, and expecting success
  
  ${CLANG} \
-fuse-ld=lld \
-o /tmp/main \
/tmp/main.o \
-Wl,--start-lib /tmp/b.o -Wl,--end-lib \
/usr/lib/x86_64-linux-gnu/libc++.so
  
  echo Linking, and expecting failure with D136554
  
  ${CLANG} \
-fuse-ld=lld \
-o /tmp/main \
/tmp/main.o \
/tmp/b.o \
/usr/lib/x86_64-linux-gnu/libc++.so \
-Wl,--export-dynamic

(Here `b.cc` is the first snippet above, and `main.cc` is a trivial file with 
just `int main(int argc, char* argv[]) { return 0; }`)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136554

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

[PATCH] D136554: Implement CWG2631

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D136554#4018870 , @rupprecht wrote:

> All good now! The latest revision of this patch doesn't seem to break 
> anything, unless I ran our tests wrong. From my perspective this is OK to 
> reland now.

... and yep, I was holding it wrong. There are still build failures, this time 
in linking, where LLD reports some symbols are undefined. I'm not certain yet 
if this really a problem with this patch (e.g. sometimes we see files break 
like this when users put their template code in the .cpp files and not the .h 
files), but the source code doesn't look immediately wrong. Once again, I'll 
try to reduce something.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136554

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

[PATCH] D136554: Implement CWG2631

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

All good now! The latest revision of this patch doesn't seem to break anything, 
unless I ran our tests wrong. From my perspective this is OK to reland now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136554

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

[PATCH] D136554: Implement CWG2631

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D136554#4013463 , @rupprecht wrote:

> Glad the test case made sense to you, it was convoluted to me :)
>
> Still seeing one more error, and it's not modules-related so I might be able 
> to get it reduced today. Generally, it looks like this:
>
>   struct Inner {
> Foo& foo;
> const std::unique_ptr<...> x = blah(blah(
> ()));
>   };
>   
>   class Outer {
>private:
> Foo foo_;
> Inner inner{foo_};
>   }
>
> With the error being:
>
>   error: 'Inner::foo' is not a member of class 'Outer'
> ()));
>
> I think this build failure is wrong? `foo` should be referring to the 
> definition inside `Inner`, but clang seems to be expecting it to refer to 
> something in `Outer`.
>
> Is it expected that this patch will cause some previously "working" code to 
> no longer build? At some point I expect to hand you a reduction that's 
> actually a bug in the user code.

Fully reduced as:

  template  int c(a, b);
  struct d {
static d e(const char * = __builtin_FILE());
  };
  struct f {
f(d = d::e());
  };
  struct h {
int 
int blah = c(g, f());
  };
  struct k {
k();
int i;
h j{i};
  };
  k::k() {}

With the error: `repro.cc:10:16: error: 'h::g' is not a member of class 'k'`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136554

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

[PATCH] D136554: Implement CWG2631

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

Glad the test case made sense to you, it was convoluted to me :)

Still seeing one more error, and it's not modules-related so I might be able to 
get it reduced today. Generally, it looks like this:

  struct Inner {
Foo& foo;
const std::unique_ptr<...> x = blah(blah(
()));
  };
  
  class Outer {
   private:
Foo foo_;
Inner inner{foo_};
  }

With the error being:

  error: 'Inner::foo' is not a member of class 'Outer'
()));

I think this build failure is wrong? `foo` should be referring to the 
definition inside `Inner`, but clang seems to be expecting it to refer to 
something in `Outer`.

Is it expected that this patch will cause some previously "working" code to no 
longer build? At some point I expect to hand you a reduction that's actually a 
bug in the user code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136554

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

[PATCH] D136554: Implement CWG2631

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

Sorry for the delay, I was out on vacation for a bit. I have a repro for this 
new issue now: F25778542: repro.tar.gz 

  $ CLANG=~/dev/clang ./repro.sh
  ++ dirname /tmp/repro/repro.sh
  + DIR=/tmp/repro
  + : /tmp/D136554
  + rm -rf /tmp/D136554
  + mkdir -p /tmp/D136554
  + cd /tmp/D136554
  + cp /tmp/repro/lib.h /tmp/repro/outer.h /tmp/repro/x.h /tmp/repro/y.h 
/tmp/repro/z.h /tmp/repro/lib.cppmap /tmp/repro/outer.cppmap 
/tmp/repro/x.cppmap /tmp/repro/y.cppmap /tmp/repro/z.cppmap .
  + COMMON_ARGS=('-Wno-mismatched-tags' '-Xclang=-fmodules-embed-all-files' 
'-Xclang=-fmodules-local-submodule-visibility' '-fmodules' 
'-fno-implicit-modules' '-fno-implicit-module-maps' '-std=gnu++17' 
'-Xclang=-fmodule-map-file-home-is-cwd')
  + export COMMON_ARGS
  + /home/rupprecht/dev/clang -Wno-mismatched-tags 
-Xclang=-fmodules-embed-all-files -Xclang=-fmodules-local-submodule-visibility 
-fmodules -fno-implicit-modules -fno-implicit-module-maps -std=gnu++17 
-Xclang=-fmodule-map-file-home-is-cwd -fmodule-name=x 
-fmodule-map-file=x.cppmap -fmodule-map-file=lib.cppmap -xc++ 
-Xclang=-emit-module -c x.cppmap -o x.pcm
  + /home/rupprecht/dev/clang -Wno-mismatched-tags 
-Xclang=-fmodules-embed-all-files -Xclang=-fmodules-local-submodule-visibility 
-fmodules -fno-implicit-modules -fno-implicit-module-maps -std=gnu++17 
-Xclang=-fmodule-map-file-home-is-cwd -fmodule-name=y 
-fmodule-map-file=y.cppmap -fmodule-map-file=lib.cppmap 
-fmodule-map-file=z.cppmap -xc++ -Xclang=-emit-module -c y.cppmap -o y.pcm
  + /home/rupprecht/dev/clang -Wno-mismatched-tags 
-Xclang=-fmodules-embed-all-files -Xclang=-fmodules-local-submodule-visibility 
-fmodules -fno-implicit-modules -fno-implicit-module-maps -std=gnu++17 
-Xclang=-fmodule-map-file-home-is-cwd -fmodule-name=outer 
-fmodule-map-file=outer.cppmap -Xclang=-fmodule-file=x.pcm 
-Xclang=-fmodule-file=y.pcm -fmodule-map-file=lib.cppmap 
-fmodule-map-file=x.cppmap -fmodule-map-file=y.cppmap -xc++-header 
-fsyntax-only -c outer.h -o outer.h.processed
  In module 'x':
  ./lib.h:5:25: error: 'Foo' has different definitions in different modules; 
first difference is definition in module 'x.x.h' found data member 'kConstant' 
with an initializer
static constexpr char kConstant = '+';
~~^~~
  ./lib.h:5:25: note: but in 'y.y.h' found data member 'kConstant' with a 
different initializer
static constexpr char kConstant = '+';
~~^~~
  1 error generated.



In D136554#4000654 , @cor3ntin wrote:

> In D136554#4000363 , @rupprecht 
> wrote:
>
>> I applied this version of the patch and the crash is now gone 
>>
>> However, now I get this inexplicable error -- I'm not entirely sure it's 
>> related, maybe I'm holding it wrong:
>>
>>   In module '':
>>   foo.h$line:$num: error: 'foo::FooClass' has different definitions in 
>> different modules; first difference is definition in module 'something.h' 
>> found data member 'kFooDelimiter' with an initializer
>> static constexpr char kFooDelimiter = '+';
>> ~~^~~
>>   foo.h:$line:$num note: but in 'other.h' found data member 'kFooDelimiter' 
>> with a different initializer
>> static constexpr char kFooDelimiter = '+';
>> ~~^~~
>>
>> The definition seems straightforward:
>>
>>   class FooClass final {
>> ...
>> static constexpr char kFooDelimiter = '+';
>> ...
>>   };
>
> This is *very* surprising to me.
> I could explain it if  the member was not static though, as it would be the 
> kind of things the patch affects. But static data members are handled very 
> differently.
>
> Was that liking chrome? It didn't come up in my tests

No, it's some other internal target. There isn't any way for you to be able to 
test against these targets, so unfortunately the best I can offer is I'll patch 
in any updated versions of this patch, see what breaks, and try to reduce it 
when reporting.

There's barely any code in the repro I attached, it's just a bunch of 
header/module layering. My guess is that clang is usually able to see that the 
two definitions in different modules is the same and therefore dedupes them, 
but this change adds some unique bit of information that makes clang think it's 
different. Note that the headers need to be in a particular order to break.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136554

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

[PATCH] D139741: [clang][CodeGen] Use base subobject type layout for potentially-overlapping fields

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D139741#4000201 , @rupprecht wrote:

> I'm not sure what the libcxx failure was that caused you to revert this, but 
> we also saw a clang crasher as a result of this. `clang/lib/AST/Decl.cpp:4300 
> in unsigned int clang::FieldDecl::getBitWidthValue(const ASTContext &) const: 
> isBitField() && "not a bitfield"`. I'll try to reduce it.

Here:

  template 
  struct Foo {
Foo(F, X, Y y) : y_(y) {}
Y y_;
union FooHolder {
  int foo;
};
[[no_unique_address]] FooHolder foo_holder_;
  };
  template 
  void MakeFoo(Factory f, X x, Y y) {
Foo(f, x, y);
  }
  void Start() {
MakeFoo(0, int(), [] {});
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139741

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

[PATCH] D136554: Implement CWG2631

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

I applied this version of the patch and the crash is now gone 

However, now I get this inexplicable error -- I'm not entirely sure it's 
related, maybe I'm holding it wrong:

  In module '':
  foo.h$line:$num: error: 'foo::FooClass' has different definitions in 
different modules; first difference is definition in module 'something.h' found 
data member 'kFooDelimiter' with an initializer
static constexpr char kFooDelimiter = '+';
~~^~~
  foo.h:$line:$num note: but in 'other.h' found data member 'kFooDelimiter' 
with a different initializer
static constexpr char kFooDelimiter = '+';
~~^~~

The definition seems straightforward:

  class FooClass final {
...
static constexpr char kFooDelimiter = '+';
...
  };


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136554

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

[PATCH] D140058: [clang-format][NFC] Turn on some code-changing options one by one

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D140058#4000311 , @rupprecht wrote:

> This is causing a test failure: 
> https://buildkite.com/llvm-project/upstream-bazel/builds/48607#0185190c-43f8-43ff-b8bd-fa8ce0b6e2f5
> (and likewise running `ninja check-clang-unit` locally, but I don't have a 
> buildbot link to that)

Fixed in 5a06334c51aa75d7f044785a495cf2de5bf13a9c 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140058

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

[PATCH] D140058: [clang-format][NFC] Turn on some code-changing options one by one

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

This is causing a test failure: 
https://buildkite.com/llvm-project/upstream-bazel/builds/48607#0185190c-43f8-43ff-b8bd-fa8ce0b6e2f5
(and likewise running `ninja check-clang-unit` locally, but I don't have a 
buildbot link to that)




Comment at: clang/lib/Format/Format.cpp:3408
   Passes.emplace_back([&](const Environment ) {
-return BracesInserter(Env, Expanded).process(/*SkipAnnotation=*/true);
+return BracesInserter(Env, S).process(/*SkipAnnotation=*/true);
   });

This looks like use-after-free. S is captured by reference but the body of the 
lambda executes after this scope block has ended.

BracesInserter itself takes it by ref but then stores a copy, but by the time 
the constructor runs, the reference is already dangling.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140058

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

[PATCH] D139741: [clang][CodeGen] Use base subobject type layout for potentially-overlapping fields

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

I'm not sure what the libcxx failure was that caused you to revert this, but we 
also saw a clang crasher as a result of this. `clang/lib/AST/Decl.cpp:4300 in 
unsigned int clang::FieldDecl::getBitWidthValue(const ASTContext &) const: 
isBitField() && "not a bitfield"`. I'll try to reduce it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139741

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

[PATCH] D136554: Implement CWG2631

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

Here's a well-formed reproducer:

  struct MyStringView {
MyStringView(const char *);
  };
  
  struct SourceLocation {
static SourceLocation current(const char * = __builtin_FILE(),
  unsigned int = __builtin_LINE());
  };
  
  template 
  int Wrap(T);
  
  template 
  struct Fixed {
using name = MyStringView;
  };
  
  struct Metadata {
Metadata(SourceLocation = SourceLocation::current());
  };
  
  struct Name {
Name(int *);
  };
  
  template 
  struct C {
static C New(Name, typename Fixed::name..., Metadata);
  };
  struct X {
int ();
  };
  struct Foo {
X x;
int i = Wrap(C::New((), "", "", Metadata()));
  };
  void func() { new Foo{}; }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136554

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

[PATCH] D136554: Implement CWG2631

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

Actually, that assertion failure is pre-existing. However, this is newly 
failing in a no-asserts clang, so I wonder if something about this patch is 
just surfacing an existing bug in clang. Anyway, I hope to have a better repro 
by EOD.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136554

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

[PATCH] D136554: Implement CWG2631

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

Ugh, I left cvise running overnight and forgot to include the validity check by 
building with a previous clang, so my reduction is invalid. I'm going to run it 
again, but here's the invalid crasher in the meantime:

  struct SourceLocation {
static SourceLocation current(const char * = __builtin_FILE()) {
  struct Metadata {
Metadata(SourceLocation = current());
namespace struct {
  int x = z(x, Metadata())
} y {
}

stack trace:

  clang: /home/rupprecht/src/llvm-project/clang/include/clang/AST/Type.h:752: 
const ExtQualsTypeCommonBase *clang::QualType::getCommonPtr() const: Assertion 
`!isNull() && "Cannot retrieve a NULL type pointer"' failed.
  PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ 
and include the crash backtrace, preprocessed source, and associated run script.
  Stack dump:
  0.  Program arguments: bin/clang -c /tmp/repro.cc -std=c++17 -o 
/tmp/repro.o
  1.   parser at end of file
  2.  /tmp/repro.cc:1:1: parsing struct/union/class body 'SourceLocation'
  3.  /tmp/repro.cc:2:66: parsing function body 'SourceLocation::current'
  4.  /tmp/repro.cc:2:66: in compound statement ('{}')
   #0 0x5645aaceaaba llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) 
/home/rupprecht/src/llvm-project/llvm/lib/Support/Unix/Signals.inc:567:11
   #1 0x5645aaceac6b PrintStackTraceSignalHandler(void*) 
/home/rupprecht/src/llvm-project/llvm/lib/Support/Unix/Signals.inc:641:1
   #2 0x5645aace92f6 llvm::sys::RunSignalHandlers() 
/home/rupprecht/src/llvm-project/llvm/lib/Support/Signals.cpp:104:5
   #3 0x5645aacea3ae llvm::sys::CleanupOnSignal(unsigned long) 
/home/rupprecht/src/llvm-project/llvm/lib/Support/Unix/Signals.inc:367:1
   #4 0x5645aac01187 (anonymous 
namespace)::CrashRecoveryContextImpl::HandleCrash(int, unsigned long) 
/home/rupprecht/src/llvm-project/llvm/lib/Support/CrashRecoveryContext.cpp:0:7
   #5 0x5645aac015dc CrashRecoverySignalHandler(int) 
/home/rupprecht/src/llvm-project/llvm/lib/Support/CrashRecoveryContext.cpp:398:1
   #6 0x7f34de23daa0 (/lib/x86_64-linux-gnu/libc.so.6+0x3daa0)
   #7 0x7f34de28957c __pthread_kill_implementation 
./nptl/pthread_kill.c:44:76
   #8 0x7f34de23da02 gsignal ./signal/../sysdeps/posix/raise.c:27:6
   #9 0x7f34de228469 abort ./stdlib/abort.c:81:7
  #10 0x7f34de228395 _nl_load_domain ./intl/loadmsgcat.c:1177:9
  #11 0x7f34de236ab2 (/lib/x86_64-linux-gnu/libc.so.6+0x36ab2)
  #12 0x5645ab1dae17 clang::QualType::getCommonPtr() const 
/home/rupprecht/src/llvm-project/clang/include/clang/AST/Type.h:0:5
  #13 0x5645ab1dada5 clang::QualType::getTypePtr() const 
/home/rupprecht/src/llvm-project/clang/include/clang/AST/Type.h:6622:26
  #14 0x5645ab1da995 clang::QualType::operator->() const 
/home/rupprecht/src/llvm-project/clang/include/clang/AST/Type.h:794:5
  #15 0x5645b071fe92 clang::computeDependence(clang::CXXThisExpr*) 
/home/rupprecht/src/llvm-project/clang/lib/AST/ComputeDependence.cpp:312:43
  #16 0x5645aed30c16 clang::CXXThisExpr::CXXThisExpr(clang::SourceLocation, 
clang::QualType, bool) 
/home/rupprecht/src/llvm-project/clang/include/clang/AST/ExprCXX.h:1154:19
  #17 0x5645af7f7c53 clang::Sema::BuildCXXThisExpr(clang::SourceLocation, 
clang::QualType, bool) 
/home/rupprecht/src/llvm-project/clang/lib/Sema/SemaExprCXX.cpp:1400:30
  #18 0x5645af7d46a8 
clang::TreeTransform::RebuildCXXThisExpr(clang::SourceLocation,
 clang::QualType, bool) 
/home/rupprecht/src/llvm-project/clang/lib/Sema/TreeTransform.h:3196:22
  #19 0x5645af7b09ae 
clang::TreeTransform::TransformCXXThisExpr(clang::CXXThisExpr*)
 /home/rupprecht/src/llvm-project/clang/lib/Sema/TreeTransform.h:12139:23
  #20 0x5645af7a9ec5 
clang::TreeTransform::TransformExpr(clang::Expr*)
 
/home/rupprecht/src/llvm-build/dev/tools/clang/include/clang/AST/StmtNodes.inc:907:1
  #21 0x5645af7b5097 
clang::TreeTransform::TransformMemberExpr(clang::MemberExpr*)
 /home/rupprecht/src/llvm-project/clang/lib/Sema/TreeTransform.h:11224:34
  #22 0x5645af7aa857 
clang::TreeTransform::TransformExpr(clang::Expr*)
 
/home/rupprecht/src/llvm-build/dev/tools/clang/include/clang/AST/StmtNodes.inc:1249:1
  #23 0x5645af7b945a 
clang::TreeTransform::TransformRecoveryExpr(clang::RecoveryExpr*)
 /home/rupprecht/src/llvm-project/clang/lib/Sema/TreeTransform.h:10929:36
  #24 0x5645af7aade4 
clang::TreeTransform::TransformExpr(clang::Expr*)
 
/home/rupprecht/src/llvm-build/dev/tools/clang/include/clang/AST/StmtNodes.inc:1431:1
  #25 0x5645af71b9fe 
clang::TreeTransform::TransformInitializer(clang::Expr*,
 bool) /home/rupprecht/src/llvm-project/clang/lib/Sema/TreeTransform.h:4006:25
  #26 0x5645af533e03 
clang::Sema::BuildCXXDefaultInitExpr(clang::SourceLocation, clang::FieldDecl*) 
/home/rupprecht/src/llvm-project/clang/lib/Sema/SemaExpr.cpp:6090:19
  #27 0x5645af995bcf (anonymous 

[PATCH] D136554: Implement CWG2631

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

Looks like the latest reland still has some issue remaining. With asserts 
enabled, I get: `assert.h assertion failed at 
clang/include/clang/AST/Type.h:752 in const ExtQualsTypeCommonBase 
*clang::QualType::getCommonPtr() const: !isNull() && "Cannot retrieve a NULL 
type pointer"`. I'm going to work on reducing it now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136554

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

[PATCH] D129755: Thread safety analysis: Support copy-elided production of scoped capabilities through arbitrary calls

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D129755#3869081 , @aaronpuchert 
wrote:

> In D129755#3866887 , @rupprecht 
> wrote:
>
>> I might have a better answer in a day or two of how widespread this is 
>> beyond just the core files that seem to make the world break when we enable 
>> this. We can just fix the bugs it if it's only a few of them, but it might 
>> be difficult if we have too many.
>
> The good news is that for now we've only seen the second category of issues, 
> for which a flag to restore the old behavior would be feasible. I can't say 
> for certain whether that would make all the issues here disappear, but it 
> definitely seems so.

We're about done with our cleanup. Our fix count is at 34, and should be final 
unless there are surprises. I'm not sure if others would benefit from having 
this warning pushed to a subflag, but we don't need it anymore.

While making some fixes, I ran across a strange false positive which I filed as 
https://github.com/llvm/llvm-project/issues/58535. I think it's another 
situation where the code is too complex for thread analysis to be feasible, but 
I also didn't see it documented as one of the common cases that isn't supported.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129755

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

[PATCH] D129755: Thread safety analysis: Support copy-elided production of scoped capabilities through arbitrary calls

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D129755#3866213 , @aaronpuchert 
wrote:

> Are you seeing warnings because of the different treatment of copy-elided 
> construction, or because we've started to consider `CXXConstructorCall`s 
> outside of the initializer of a `DeclStmt`?

I'm not familiar with internal Clang APIs so I'm not entirely sure how to 
answer that, but I can share sketches of the breakages I've addressed so far:

1. (2x) Returning a MutexLock-like structure, e.g.

  MutexLock Lock() {
return MutexLock();
  }

this is documented in the test as a known false positive. Is that something you 
plan to address some day, or is it beyond the limits of thread safety analysis? 
(Or theoretically possible, but then compile times would be too much if you 
have to do more analysis?) Anyway, I applied 
`__attribute__((no_thread_safety_analysis))` to the method.

2. (3x) deferred/offloaded unlocking, e.g.

  void Func() {
auto *lock = new MutexLock();
auto cleanup = [lock]() { delete lock; };
cleanup();
  } // Error that mu is not unlocked

I assume this is beyond the scope of thread safety analysis -- `cleanup()` in 
the internal case actually happens in a different TU (the "cleanup" gets passed 
to some generic scheduler thingy), which is even more complex than this trivial 
example. In one of the cases the unlocking would happen on some other thread, 
which I guess is a little suspicious, but should still be guaranteed to execute 
(I didn't dig _too_ deep there). Anyway, I also suppressed analysis here.

3. Missed lock annotation on the constructor, take 1

  struct Foo {
explicit Foo(Mutex *mu) EXCLUSIVE_LOCKS_REQUIRED(mu);
  };
  
  Something Func() {
SomethingLikeMutexLock lock();
lock.Release(); // !!!
return Get(Foo());
  }

Glaringly obvious bug that wasn't being caught before this patch. Moving the 
call to `Foo()` earlier while the lock is still held fixes the bug.

4. Missed lock annotation on the constructor, take 2

  struct Foo {
explicit Foo(Mutex *mu) EXCLUSIVE_LOCKS_REQUIRED(mu);
  };
  void X() {
auto lock = MutexLock();
auto f = MakeFoo();
  }
  Foo Y() {
return Foo();
  }

`X` grabs the lock, but `Y` warns that we don't have `mu` held. In this case, 
`mu->AssertHeld()` suffices (the mutex pattern is a little more complicated, 
and the pattern is used elsewhere, but was missed in this method).

5. Alternate `std::make_unique` implementation (this one still has me puzzled)

  template 
  inline std::unique_ptr MyMakeUnique(Args &&...args) {
return std::unique_ptr(new T(std::forward(args)...));
  }
  
  static Mutex y_mu;
  
  class Z {
   public:
Z() EXCLUSIVE_LOCKS_REQUIRED(y_mu) {}
  };
  
  std::unique_ptr Y() {
auto lock = MutexLock(_mu);
return MyMakeUnique();
  }

Returning  `std::make_unique()` instead of the custom helper works. Why? No 
idea. `MyMakeUnique` is a c++11 compatibility helper -- std::unique_ptr is in 
c++11 but std::make_unique is only in c++14 -- and fortunately this code 
doesn't need it anymore. The implementation seems to be identical to the one in 
libc++, so my best guess is threading analysis has some special handling for 
some std:: methods.

I might have a better answer in a day or two of how widespread this is beyond 
just the core files that seem to make the world break when we enable this. We 
can just fix the bugs it if it's only a few of them, but it might be difficult 
if we have too many.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129755

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

[PATCH] D129755: Thread safety analysis: Support copy-elided production of scoped capabilities through arbitrary calls

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

I'm seeing a fair number of breakages from this patch (not really sure how many 
we truly have, I've hit ~5-10 so far in widely used libraries, but I suspect we 
have far more in the long tail). They're all valid (most are just adding 
missing thread safety annotations, etc., so far one breakage caught a real 
bug), but it would help if we could disable just these new ones. How 
invasive/annoying would it be to carve out a subwarning for this category of 
bugs and call it something like `-Wthread-safety-elision`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129755

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

[PATCH] D112732: [ASan] Process functions in Asan module pass

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:1189
+MPM.addPass(ModuleAddressSanitizerPass(
+CompileKernel, Recover, ModuleUseAfterScope, UseOdrIndicator,
+DestructorKind, UseAfterScope, UseAfterReturn));

rupprecht wrote:
> I'm seeing build errors here, and I'm not sure what this is supposed to be?
> 
> ```
> /home/rupprecht/src/llvm-project/clang/lib/CodeGen/BackendUtil.cpp:1189:37: 
> error: use of undeclared identifier 'ModuleUseAfterScope'; did you mean 
> 'UseAfterScope'?
> CompileKernel, Recover, ModuleUseAfterScope, UseOdrIndicator,
> ^~~
> UseAfterScope
> ```
> 
> https://buildkite.com/llvm-project/upstream-bazel-rbe/builds/11193#bbebbf99-a2c1-4959-b6f7-f7fb816591c0
I see it was reverted already, sorry!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112732

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

[PATCH] D112732: [ASan] Process functions in Asan module pass

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:1189
+MPM.addPass(ModuleAddressSanitizerPass(
+CompileKernel, Recover, ModuleUseAfterScope, UseOdrIndicator,
+DestructorKind, UseAfterScope, UseAfterReturn));

I'm seeing build errors here, and I'm not sure what this is supposed to be?

```
/home/rupprecht/src/llvm-project/clang/lib/CodeGen/BackendUtil.cpp:1189:37: 
error: use of undeclared identifier 'ModuleUseAfterScope'; did you mean 
'UseAfterScope'?
CompileKernel, Recover, ModuleUseAfterScope, UseOdrIndicator,
^~~
UseAfterScope
```

https://buildkite.com/llvm-project/upstream-bazel-rbe/builds/11193#bbebbf99-a2c1-4959-b6f7-f7fb816591c0


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112732

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

[PATCH] D104261: Thread safety analysis: Always warn when dropping locks on back edges

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D104261#2844636 , @aaronpuchert 
wrote:

> In D104261#2841356 , @delesley 
> wrote:
>
>> since it's restricted to relockable managed locks, I'm not too worried...
>
> Not quite, it affects scoped locks with explicit unlock, which was supported 
> before D49885 .
>
> @rupprecht, can you still test patches on Google's code? Would be good to 
> know if this breaks anything.

Thanks for the heads up. I ran this on the same targets that broke in D84604 
 (in case that's what you're looking for), and 
those continue to pass. I can try further testing of other targets, but that 
may take a little longer, so I have no problem with you landing this as-is and 
I can follow up if there are problems elsewhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104261

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

[PATCH] D101191: [InstCombine] Fully disable select to and/or i1 folding

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D101191#2788596 , @spatel wrote:

> In D101191#2783570 , @rupprecht 
> wrote:
>
>> In D101191#2782963 , @rupprecht 
>> wrote:
>>
>>> The issue I'm seeing seems more directly caused by SLP vectorization, as it 
>>> goes away with `-fno-slp-vectorize`. This patch merely unblocks that bad 
>>> optimization AFAICT.
>>
>> Filed as http://llvm.org/PR50500
>
> We needed SLP to get to the problem state in that example, but the bug was in 
> instcombine/instsimplify. 
> Should be fixed with:
> 7bb8bfa0622b 
> 

Thanks Sanjay! This fixes the unreduced miscompile we're seeing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101191

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

[PATCH] D101191: [InstCombine] Fully disable select to and/or i1 folding

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D101191#2782963 , @rupprecht wrote:

> The issue I'm seeing seems more directly caused by SLP vectorization, as it 
> goes away with `-fno-slp-vectorize`. This patch merely unblocks that bad 
> optimization AFAICT.

Filed as http://llvm.org/PR50500


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101191

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

[PATCH] D101191: [InstCombine] Fully disable select to and/or i1 folding

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

The issue I'm seeing seems more directly caused by SLP vectorization, as it 
goes away with `-fno-slp-vectorize`. This patch merely unblocks that bad 
optimization AFAICT.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101191

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

[PATCH] D101191: [InstCombine] Fully disable select to and/or i1 folding

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

FYI, I'm seeing what I think is a miscompile that bisects to this patch. 
Greatly simplified, the problematic snippet is this:

  struct Stats {
int a;
int b;
int a_or_b;
  };
  
  bool x = ...
  bool y = ...
  Stats stats;
  stats.a = x ? 1 : 0;
  stats.b = y ? 1 : 0;
  stats.a_or_b = (x || y) ? 1 : 0;

What we see is that when x is false and y is true, a is 0 (expected), b is 1 
(expected), but a_or_b is 0 (unexpected).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101191

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

[PATCH] D100776: [clang/Basic] Make TargetInfo.h not use DataLayout again

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

Landed 44e2247dcd04f3421164b085094eb575270564ba 
 to fix 
LLDB. If you decide to go in a different direction again, please adjust that 
fix accordingly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100776

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

[PATCH] D99414: [clang][tooling] Create SourceManager for DiagnosticsEngine before command-line parsing

[Jordan Rupprecht via Phabricator via cfe-commits]

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

Thanks, the error message is much better now!

When applying this patch locally and also undoing the fix in our internal tool 
(i.e. change it to `-ferror-limit=-1`), the error I get is:

  message: "invalid integral value \'-1\' in \'-ferror-limit -1\'"

which, you know, actually tells me what to fix instead of giving some weird 
internal assertion :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99414

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

[PATCH] D98980: [CodeGen] Don't crash on for loops with cond variables and no increment

[Jordan Rupprecht via Phabricator via cfe-commits]

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

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98980

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

[PATCH] D98816: PR49585: Emit the jump destination for a for loop 'continue' from within the scope of the condition variable.

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

I tried creating an IR repro, but: running `-S -emit-llvm` with the old PM 
crashes before it can generate anything, and running with the new PM seems to 
generate invalid IR (branches to %for.inc, which does not exist). I suspect 
this is not an optimizer bug, but crashing in the optimizer because invalid IR 
has been fed to it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98816

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

[PATCH] D98816: PR49585: Emit the jump destination for a for loop 'continue' from within the scope of the condition variable.

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

The error message I posted earlier was when using O1 
 + new PM, but it crashes with the 
old one & no optimizations:

  $ cat /tmp/repro.cc
  void a() {
for (; int b = 0;) continue;
  }
  $ clang -c /tmp/repro.cc -o /tmp/repro.o
  Referring to a basic block in another function!
br label %for.inc
  in function _Z1av
  fatal error: error in backend: Broken function found, compilation aborted!
  PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash 
backtrace, preprocessed source, and associated run script.
  Stack dump:
  0.  Program arguments: clang -c /tmp/repro.cc -o /tmp/repro.o
  1.   parser at end of file
  2.  Per-function optimization
  3.  Running pass 'Module Verifier' on function '@_Z1av'
  ...
  $ clang -fno-legacy-pass-manager -O1 -c /tmp/repro.cc -o /tmp/repro.o
  clang: 
/home/rupprecht/src/llvm-project/llvm/include/llvm/Support/Casting.h:104: 
static bool llvm::isa_impl_cl::doit(const From *) [To = llvm::InvokeInst, From = const llvm::Instruction 
*]: Assertion `Val && "isa<> used on a null pointer"' failed.
  PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash 
backtrace, preprocessed source, and associated run script.
  Stack dump:
  0.  Program arguments: bin/clang -fno-legacy-pass-manager -O1 -c 
/tmp/repro.cc -o /tmp/repro.o
  1.   parser at end of file
  2.  Optimizer
  ...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98816

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

[PATCH] D98816: PR49585: Emit the jump destination for a for loop 'continue' from within the scope of the condition variable.

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

We're seeing a crash in the optimizer after this patch, with the following 
logged in assert builds: `assert.h assertion failed at 
llvm/include/llvm/Support/Casting.h:104 in static bool 
llvm::isa_impl_cl::doit(const From 
*) [To = llvm::InvokeInst, From = const llvm::Instruction *]: Val && "isa<> 
used on a null pointer"`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98816

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

[PATCH] D97009: [CUDA] fix builtin constraints for PTX 7.2

[Jordan Rupprecht via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1a368ae3b78d: [CUDA] fix builtin constraints for PTX 7.2 
(authored by tra, committed by rupprecht).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97009

Files:
  clang/include/clang/Basic/BuiltinsNVPTX.def
  clang/test/CodeGen/builtins-nvptx-sm_70.cu


Index: clang/test/CodeGen/builtins-nvptx-sm_70.cu
===
--- clang/test/CodeGen/builtins-nvptx-sm_70.cu
+++ clang/test/CodeGen/builtins-nvptx-sm_70.cu
@@ -6,6 +6,11 @@
 // RUN:-fcuda-is-device -target-feature +ptx61 -DPTX61 \
 // RUN:-S -emit-llvm -o - -x cuda %s \
 // RUN:   | FileCheck -check-prefixes=CHECK_M16,CHECK_M32_M8 %s
+// Make sure builtins still work with the latest combination of GPU & PTX.
+// RUN: %clang_cc1 -triple nvptx64-unknown-unknown -target-cpu sm_86 \
+// RUN:-fcuda-is-device -target-feature +ptx72 -DPTX61 \
+// RUN:-S -emit-llvm -o - -x cuda %s \
+// RUN:   | FileCheck -check-prefixes=CHECK_M16,CHECK_M32_M8 %s
 // RUN: %clang_cc1 -triple nvptx-unknown-unknown -target-cpu sm_60 \
 // RUN:   -DPTX61 -fcuda-is-device -S -o /dev/null -x cuda -verify=pre-sm_70 %s
 // RUN: %clang_cc1 -triple nvptx-unknown-unknown \
Index: clang/include/clang/Basic/BuiltinsNVPTX.def
===
--- clang/include/clang/Basic/BuiltinsNVPTX.def
+++ clang/include/clang/Basic/BuiltinsNVPTX.def
@@ -38,7 +38,9 @@
 #pragma push_macro("PTX65")
 #pragma push_macro("PTX70")
 #pragma push_macro("PTX71")
-#define PTX71 "ptx71"
+#pragma push_macro("PTX72")
+#define PTX72 "ptx72"
+#define PTX71 "ptx71|" PTX72
 #define PTX70 "ptx70|" PTX71
 #define PTX65 "ptx65|" PTX70
 #define PTX64 "ptx64|" PTX65
@@ -740,3 +742,4 @@
 #pragma pop_macro("PTX65")
 #pragma pop_macro("PTX70")
 #pragma pop_macro("PTX71")
+#pragma pop_macro("PTX72")


Index: clang/test/CodeGen/builtins-nvptx-sm_70.cu
===
--- clang/test/CodeGen/builtins-nvptx-sm_70.cu
+++ clang/test/CodeGen/builtins-nvptx-sm_70.cu
@@ -6,6 +6,11 @@
 // RUN:-fcuda-is-device -target-feature +ptx61 -DPTX61 \
 // RUN:-S -emit-llvm -o - -x cuda %s \
 // RUN:   | FileCheck -check-prefixes=CHECK_M16,CHECK_M32_M8 %s
+// Make sure builtins still work with the latest combination of GPU & PTX.
+// RUN: %clang_cc1 -triple nvptx64-unknown-unknown -target-cpu sm_86 \
+// RUN:-fcuda-is-device -target-feature +ptx72 -DPTX61 \
+// RUN:-S -emit-llvm -o - -x cuda %s \
+// RUN:   | FileCheck -check-prefixes=CHECK_M16,CHECK_M32_M8 %s
 // RUN: %clang_cc1 -triple nvptx-unknown-unknown -target-cpu sm_60 \
 // RUN:   -DPTX61 -fcuda-is-device -S -o /dev/null -x cuda -verify=pre-sm_70 %s
 // RUN: %clang_cc1 -triple nvptx-unknown-unknown \
Index: clang/include/clang/Basic/BuiltinsNVPTX.def
===
--- clang/include/clang/Basic/BuiltinsNVPTX.def
+++ clang/include/clang/Basic/BuiltinsNVPTX.def
@@ -38,7 +38,9 @@
 #pragma push_macro("PTX65")
 #pragma push_macro("PTX70")
 #pragma push_macro("PTX71")
-#define PTX71 "ptx71"
+#pragma push_macro("PTX72")
+#define PTX72 "ptx72"
+#define PTX71 "ptx71|" PTX72
 #define PTX70 "ptx70|" PTX71
 #define PTX65 "ptx65|" PTX70
 #define PTX64 "ptx64|" PTX65
@@ -740,3 +742,4 @@
 #pragma pop_macro("PTX65")
 #pragma pop_macro("PTX70")
 #pragma pop_macro("PTX71")
+#pragma pop_macro("PTX72")
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97009: [CUDA] fix builtin constraints for PTX 7.2

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

This fixes a build error we're seeing, so I'd like to land this in a bit if 
that's OK.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97009

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

[PATCH] D84673: [clang][cli] Port DiagnosticOpts to new option parsing system

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

I ran into this commit when integrating commits today (specifically, 
97100646d1b4526de1eac3aacdb0b098739c6ec9 
) -- 
there's nothing wrong with this patch AFAICT, but I'm wondering if the error 
messaging/handling could be improved somehow.

tl;dr I reduced an example in D94468 

We have an internal user creating a ToolInvocation and attaching a 
DiagnosticConsumer to it. They were also using `-ferror-limit=-1`, which if I 
understand now, may be meaningless -- `-ferror-limit=0` is used to mean 
"unlimited error messages", which is probably what they meant.

Anyway, with the DiagnosticConsumer attached, the test crashed with an 
assertion failure `"Assertion 'SourceMgr && "SourceManager not set!"` failed.", 
which really wasn't helpful at all. After much longer than I care to admit, I 
thought to remove the `setDiagnosticConsumer`, and was able to find the 
`"invalid integral value '-1' in '-ferror-limit -1'"` error in the logs, and I 
was able to find the real issue pretty quickly after that.

I don't think the usage was out of the ordinary, so I created D94468 
 as an example if you have time to take a 
look, to save the next person that may run into this issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84673

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

[PATCH] D94468: [test] Demonstrate bad error message

[Jordan Rupprecht via Phabricator via cfe-commits]

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

This should not be committed, but demonstrates an issue with cascading errors 
after D84673 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D94468

Files:
  clang/unittests/Tooling/ToolingTest.cpp


Index: clang/unittests/Tooling/ToolingTest.cpp
===
--- clang/unittests/Tooling/ToolingTest.cpp
+++ clang/unittests/Tooling/ToolingTest.cpp
@@ -221,6 +221,54 @@
   EXPECT_TRUE(Invocation.run());
 }
 
+class LogDiagnosticConsumer : public DiagnosticConsumer {
+public:
+  LogDiagnosticConsumer() = default;
+
+  void HandleDiagnostic(clang::DiagnosticsEngine::Level,
+const clang::Diagnostic ) override {
+llvm::SmallString<40> diagnostic_string;
+
+info.FormatDiagnostic(diagnostic_string);
+Diagnostics += diagnostic_string.str();
+
+// The next expression triggers:
+//   Assertion `SourceMgr && "SourceManager not set!"' failed.
+// When commented out, will log the diagnostic as normal, which is an
+// invalid flag:
+//   "invalid integral value '-1' in '-ferror-limit -1'"
+info.getSourceManager();
+  }
+  std::string Diagnostics;
+};
+
+TEST(ToolInvocation, DiagCallback) {
+  llvm::IntrusiveRefCntPtr OverlayFileSystem(
+  new llvm::vfs::OverlayFileSystem(llvm::vfs::getRealFileSystem()));
+  llvm::IntrusiveRefCntPtr InMemoryFileSystem(
+  new llvm::vfs::InMemoryFileSystem);
+  OverlayFileSystem->pushOverlay(InMemoryFileSystem);
+  llvm::IntrusiveRefCntPtr Files(
+  new FileManager(FileSystemOptions(), OverlayFileSystem));
+  std::vector Args;
+  Args.push_back("tool-executable");
+  // Note: intentional error; user probably meant -ferror-limit=0.
+  Args.push_back("-ferror-limit=-1");
+  Args.push_back("-fsyntax-only");
+  Args.push_back("test.cpp");
+  clang::tooling::ToolInvocation Invocation(
+  Args, std::make_unique(), Files.get());
+  InMemoryFileSystem->addFile(
+  "test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int main(){}\n"));
+
+  LogDiagnosticConsumer LDC;
+  Invocation.setDiagnosticConsumer();
+
+  EXPECT_TRUE(Invocation.run());
+
+  EXPECT_EQ("", LDC.Diagnostics);
+}
+
 struct VerifyEndCallback : public SourceFileCallbacks {
   VerifyEndCallback() : BeginCalled(0), EndCalled(0), Matched(false) {}
   bool handleBeginSource(CompilerInstance ) override {


Index: clang/unittests/Tooling/ToolingTest.cpp
===
--- clang/unittests/Tooling/ToolingTest.cpp
+++ clang/unittests/Tooling/ToolingTest.cpp
@@ -221,6 +221,54 @@
   EXPECT_TRUE(Invocation.run());
 }
 
+class LogDiagnosticConsumer : public DiagnosticConsumer {
+public:
+  LogDiagnosticConsumer() = default;
+
+  void HandleDiagnostic(clang::DiagnosticsEngine::Level,
+const clang::Diagnostic ) override {
+llvm::SmallString<40> diagnostic_string;
+
+info.FormatDiagnostic(diagnostic_string);
+Diagnostics += diagnostic_string.str();
+
+// The next expression triggers:
+//   Assertion `SourceMgr && "SourceManager not set!"' failed.
+// When commented out, will log the diagnostic as normal, which is an
+// invalid flag:
+//   "invalid integral value '-1' in '-ferror-limit -1'"
+info.getSourceManager();
+  }
+  std::string Diagnostics;
+};
+
+TEST(ToolInvocation, DiagCallback) {
+  llvm::IntrusiveRefCntPtr OverlayFileSystem(
+  new llvm::vfs::OverlayFileSystem(llvm::vfs::getRealFileSystem()));
+  llvm::IntrusiveRefCntPtr InMemoryFileSystem(
+  new llvm::vfs::InMemoryFileSystem);
+  OverlayFileSystem->pushOverlay(InMemoryFileSystem);
+  llvm::IntrusiveRefCntPtr Files(
+  new FileManager(FileSystemOptions(), OverlayFileSystem));
+  std::vector Args;
+  Args.push_back("tool-executable");
+  // Note: intentional error; user probably meant -ferror-limit=0.
+  Args.push_back("-ferror-limit=-1");
+  Args.push_back("-fsyntax-only");
+  Args.push_back("test.cpp");
+  clang::tooling::ToolInvocation Invocation(
+  Args, std::make_unique(), Files.get());
+  InMemoryFileSystem->addFile(
+  "test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int main(){}\n"));
+
+  LogDiagnosticConsumer LDC;
+  Invocation.setDiagnosticConsumer();
+
+  EXPECT_TRUE(Invocation.run());
+
+  EXPECT_EQ("", LDC.Diagnostics);
+}
+
 struct VerifyEndCallback : public SourceFileCallbacks {
   VerifyEndCallback() : BeginCalled(0), EndCalled(0), Matched(false) {}
   bool handleBeginSource(CompilerInstance ) override {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87194: Thread safety analysis: Use access specifiers to decide about scope

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D87194#2369485 , @aaronpuchert 
wrote:

> @rupprecht, maybe you can try it again?

Some more interesting errors this time :)

The ones I originally saw look correct now (i.e. it's flagging the things that 
are valid, but not the things out of visibility). I tried building the rest of 
this package, and I guess scoping isn't considered in this case though?

  class Foo {
   public:
static void Bar();
   private:
struct Params {
  Mutex mu_;
}
static Params* GetParams();
  };
  
  // In the .cc file:
  void Foo::Bar() {
Params& params = *GetParams();
MutexLock lock(params.mu_);  // error: acquiring mutex 'params.mu_' 
requires negative capability '!params.mu_'
  }
  
  /* static */ Params* Foo::GetParams() {
static Params params;
return 
  }

On one hand, it's totally valid. On the other hand, annotating the method like 
`static void Bar() REQUIRES(!params.mu_);` isn't possible because `params` is a 
local variable.

(I'm new to threading analysis, so maybe I'm just using the wrong annotations)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87194

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

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D84604#2364568 , @aaronpuchert 
wrote:

> In D84604#2363768 , @rupprecht wrote:
>
>> I applied D87194  locally and rebuilt the 
>> original source, and not only am I seeing the original issue (also firing on 
>> `DoThings()` when it should only be on `DoStuff()`), I'm also seeing: 
>> `error: acquiring mutex 'lock' requires negative capability '!lock' 
>> [-Werror,-Wthread-safety-negative]`, where `lock` is a local variable, 
>> defined as `MutexLock lock(mutex_)`.
>
> Oh yes, I need to rebase this, sorry if I wasted your time. This is still on 
> top of the bug that @lebedev.ri pointed out in D84604#2262745 
>  and on top of the bug that you 
> pointed out.

No worries, it was only a minute to patch, and then a few more minutes to take 
a snack break while it rebuilt :)

>> I'll work on getting a better repro for this.
>
> Maybe wait a bit with that, I'll add you as reviewer when I've done the 
> rebase and then you can try it again. I hope to have covered both locals and 
> static members now.
>
> Another issue is linkage, but I'll have to read up on that a bit.

Yep, I'm not qualified to actually review the code, but I'd be happy to test 
any patches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84604

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

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D84604#2363445 , @aaronpuchert 
wrote:

> Pushed a fix in rGbbed8cfe80cd27d3a47d877c7608d9be4e487d97 
> . For 
> now we just consider all static members as inaccessible, so we'll treat them 
> as we did before this change.

I can confirm this fixes the breakage -- thanks!

> I have proposed making the check stronger for non-static members in D87194 
> , perhaps it makes sense to extend this to 
> static members as well so that it fires on `DoStuff()` again.

I applied D87194  locally and rebuilt the 
original source, and not only am I seeing the original issue (also firing on 
`DoThings()` when it should only be on `DoStuff()`), I'm also seeing: `error: 
acquiring mutex 'lock' requires negative capability '!lock' 
[-Werror,-Wthread-safety-negative]`, where `lock` is a local variable, defined 
as `MutexLock lock(mutex_)`.

I'll work on getting a better repro for this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84604

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

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

I'm seeing failures which I think are due to this patch -- I don't have a nice 
godbolt repro yet, but it's something like:

  foo.h: 
  class Foo {
   public:
static void DoStuff();  // Grabs mu_
  
  private:
static std::vector blah_ GUARDED_BY(mu_);
static Mutex mu_;
  };
  
  bar.h:
  class Bar {
static void DoThings(); // calls Foo::DoStuff()
  };

Because `DoStuff()` grabs `mu_`, it needs the lock, and this patch gives an 
error like `requires negative capability 'mu_'`. That's fine, and in fact 
better analysis is welcome.

However, I'm also seeing the same error for `DoThings()`, which doesn't make 
sense. When I add it, I then get errors that `mu_` is private, and therefore 
needs to be made public or friended for the thread analysis -- which is not at 
all a good change (the mutex should be encapsulated in the class). Even though 
it's "global" in the linkage sense, it's not visible outside of the class.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84604

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

[PATCH] D88640: [Format] Don't treat compound extension headers (foo.proto.h) as foo.cc main file.

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

It looks like this fix caused a different regression in not accepting 
`name..h` as the main header for `name..cc`, e.g.:

  $ cat /tmp/foo.bar.cc
  #include "a.h"
  #include "z.h"
  #include "foo.bar.h"
  
  $ clang-format /tmp/foo.bar.cc  # Before
  #include "foo.bar.h"
  
  #include "a.h"
  #include "z.h"
  
  $ clang-format /tmp/foo.bar.cc  # After
  #include "a.h"
  #include "foo.bar.h"
  #include "z.h"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88640

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

[PATCH] D86290: Move all fields of '-cc1' option related classes into def file databases

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

Thanks for the revert.

In addition to the one eabi test, we saw widespread msan 
use-of-uninitialized-value errors from here: 
https://github.com/llvm/llvm-project/blob/master/llvm/lib/Target/ARM/ARMTargetMachine.cpp#L229.
 I think it would explain the eabi test failure.

See also: 
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap-msan/builds/21663/steps/check-clang%20msan/logs/stdio


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86290

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

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

[Jordan Rupprecht via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG60a8a504f16d: [llvm-objdump] Print file format in lowercase 
to match GNU output. (authored by rupprecht).

Changed prior to commit:
  https://reviews.llvm.org/D74433?vs=243991=244189#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74433

Files:
  clang/test/Modules/pch_container.m
  lld/test/COFF/savetemps.ll
  llvm/test/CodeGen/AArch64/arm64-simplest-elf.ll
  llvm/test/CodeGen/ARM/Windows/trivial-gnu-object.ll
  llvm/test/CodeGen/BPF/reloc-btf-2.ll
  llvm/test/CodeGen/BPF/reloc-btf.ll
  llvm/test/CodeGen/BPF/reloc.ll
  llvm/test/Object/AMDGPU/objdump.s
  llvm/test/Object/X86/objdump-disassembly-inline-relocations.test
  llvm/test/Object/X86/objdump-label.test
  llvm/test/Object/X86/objdump-trivial-object.test
  llvm/test/Object/dynamic-reloc.test
  llvm/test/Object/objdump-symbol-table.test
  llvm/test/tools/llvm-objdump/X86/disassemble-section-name.s
  llvm/test/tools/llvm-objdump/X86/elf-disassemble-symbol-labels-exec.test
  llvm/test/tools/llvm-objdump/X86/elf-dynamic-relocs.test
  llvm/test/tools/llvm-objdump/X86/output-ordering.test
  llvm/test/tools/llvm-objdump/X86/warn-missing-disasm-func.test
  llvm/test/tools/llvm-objdump/all-headers.test
  llvm/test/tools/llvm-objdump/archive-headers.test
  llvm/test/tools/llvm-objdump/file-headers-coff.test
  llvm/test/tools/llvm-objdump/file-headers-elf.test
  llvm/test/tools/llvm-objdump/non-archive-object.test
  llvm/test/tools/llvm-objdump/relocations-in-nonreloc.test
  llvm/tools/llvm-objdump/llvm-objdump.cpp

Index: llvm/tools/llvm-objdump/llvm-objdump.cpp
===
--- llvm/tools/llvm-objdump/llvm-objdump.cpp
+++ llvm/tools/llvm-objdump/llvm-objdump.cpp
@@ -2168,7 +2168,7 @@
   outs() << A->getFileName() << "(" << O->getFileName() << ")";
 else
   outs() << O->getFileName();
-outs() << ":\tfile format " << O->getFileFormatName() << "\n\n";
+outs() << ":\tfile format " << O->getFileFormatName().lower() << "\n\n";
   }
 
   if (StartAddress.getNumOccurrences() || StopAddress.getNumOccurrences())
Index: llvm/test/tools/llvm-objdump/relocations-in-nonreloc.test
===
--- llvm/test/tools/llvm-objdump/relocations-in-nonreloc.test
+++ llvm/test/tools/llvm-objdump/relocations-in-nonreloc.test
@@ -7,7 +7,7 @@
 # RUN: yaml2obj --docnum=3 %s -o %t3
 # RUN: llvm-objdump -r %t3 | FileCheck %s -DFILE=%t3 --check-prefixes=FMT,REL --implicit-check-not={{.}}
 
-# FMT: [[FILE]]: file format ELF64-x86-64
+# FMT: [[FILE]]: file format elf64-x86-64
 
 # REL:  RELOCATION RECORDS FOR []:
 # REL-NEXT: OFFSET   TYPE VALUE
Index: llvm/test/tools/llvm-objdump/non-archive-object.test
===
--- llvm/test/tools/llvm-objdump/non-archive-object.test
+++ llvm/test/tools/llvm-objdump/non-archive-object.test
@@ -2,7 +2,7 @@
 # RUN: llvm-objdump -a %t | FileCheck %s
 
 # If this test has not crashed, then this test passed.
-# CHECK: file format ELF64-x86-64
+# CHECK: file format elf64-x86-64
 
 !ELF
 FileHeader:
Index: llvm/test/tools/llvm-objdump/file-headers-elf.test
===
--- llvm/test/tools/llvm-objdump/file-headers-elf.test
+++ llvm/test/tools/llvm-objdump/file-headers-elf.test
@@ -10,7 +10,7 @@
   Machine: EM_X86_64
   Entry:   0x123456789abcde
 
-# ELF64: [[FILE]]: file format ELF64-x86-64
+# ELF64: [[FILE]]: file format elf64-x86-64
 # ELF64: architecture: x86_64
 # ELF64: start address: 0x00123456789abcde
 
@@ -18,7 +18,7 @@
 # RUN: llvm-objdump -f %t-i386 | FileCheck -DFILE=%t-i386 %s -check-prefix ELF32
 # RUN: llvm-objdump -file-headers %t-i386 | FileCheck %s -DFILE=%t-i386 -check-prefix ELF32
 
-# ELF32: [[FILE]]: file format ELF32-i386
+# ELF32: [[FILE]]: file format elf32-i386
 # ELF32: architecture: i386
 # ELF32: start address: 0x12345678
 
Index: llvm/test/tools/llvm-objdump/file-headers-coff.test
===
--- llvm/test/tools/llvm-objdump/file-headers-coff.test
+++ llvm/test/tools/llvm-objdump/file-headers-coff.test
@@ -9,6 +9,6 @@
 sections:
 symbols:
 
-# CHECK: [[FILE]]: file format COFF-i386
+# CHECK: [[FILE]]: file format coff-i386
 # CHECK: architecture: i386
 # CHECK: start address: 0x
Index: llvm/test/tools/llvm-objdump/archive-headers.test
===
--- llvm/test/tools/llvm-objdump/archive-headers.test
+++ llvm/test/tools/llvm-objdump/archive-headers.test
@@ -1,21 +1,21 @@
 # RUN: llvm-objdump -a %p/Inputs/liblong_filenames.a | FileCheck %s
 # RUN: llvm-objdump -archive-headers %p/Inputs/liblong_filenames.a | FileCheck %s
 
-# CHECK: 

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

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht updated this revision to Diff 243991.
rupprecht marked an inline comment as done.
rupprecht added a comment.

- Use StringRef::lower()


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74433

Files:
  clang/test/Modules/pch_container.m
  lld/test/COFF/savetemps.ll
  llvm/test/CodeGen/AArch64/arm64-simplest-elf.ll
  llvm/test/CodeGen/ARM/Windows/trivial-gnu-object.ll
  llvm/test/CodeGen/BPF/reloc-btf-2.ll
  llvm/test/CodeGen/BPF/reloc-btf.ll
  llvm/test/CodeGen/BPF/reloc.ll
  llvm/test/Object/AMDGPU/objdump.s
  llvm/test/Object/X86/objdump-disassembly-inline-relocations.test
  llvm/test/Object/X86/objdump-label.test
  llvm/test/Object/X86/objdump-trivial-object.test
  llvm/test/Object/dynamic-reloc.test
  llvm/test/Object/objdump-symbol-table.test
  llvm/test/tools/llvm-objdump/X86/disassemble-section-name.s
  llvm/test/tools/llvm-objdump/X86/elf-disassemble-symbol-labels-exec.test
  llvm/test/tools/llvm-objdump/X86/elf-dynamic-relocs.test
  llvm/test/tools/llvm-objdump/X86/output-ordering.test
  llvm/test/tools/llvm-objdump/X86/warn-missing-disasm-func.test
  llvm/test/tools/llvm-objdump/all-headers.test
  llvm/test/tools/llvm-objdump/archive-headers.test
  llvm/test/tools/llvm-objdump/file-headers-coff.test
  llvm/test/tools/llvm-objdump/file-headers-elf.test
  llvm/test/tools/llvm-objdump/non-archive-object.test
  llvm/test/tools/llvm-objdump/relocations-in-nonreloc.test
  llvm/tools/llvm-objdump/llvm-objdump.cpp

Index: llvm/tools/llvm-objdump/llvm-objdump.cpp
===
--- llvm/tools/llvm-objdump/llvm-objdump.cpp
+++ llvm/tools/llvm-objdump/llvm-objdump.cpp
@@ -2172,7 +2172,7 @@
   outs() << A->getFileName() << "(" << O->getFileName() << ")";
 else
   outs() << O->getFileName();
-outs() << ":\tfile format " << O->getFileFormatName() << "\n\n";
+outs() << ":\tfile format " << O->getFileFormatName().lower() << "\n\n";
   }
 
   if (StartAddress.getNumOccurrences() || StopAddress.getNumOccurrences())
Index: llvm/test/tools/llvm-objdump/relocations-in-nonreloc.test
===
--- llvm/test/tools/llvm-objdump/relocations-in-nonreloc.test
+++ llvm/test/tools/llvm-objdump/relocations-in-nonreloc.test
@@ -7,7 +7,7 @@
 # RUN: yaml2obj --docnum=3 %s -o %t3
 # RUN: llvm-objdump -r %t3 | FileCheck %s -DFILE=%t3 --check-prefixes=FMT,REL --implicit-check-not={{.}}
 
-# FMT: [[FILE]]: file format ELF64-x86-64
+# FMT: [[FILE]]: file format elf64-x86-64
 
 # REL:  RELOCATION RECORDS FOR []:
 # REL-NEXT: 0123 R_X86_64_NONE *ABS*+0x141
Index: llvm/test/tools/llvm-objdump/non-archive-object.test
===
--- llvm/test/tools/llvm-objdump/non-archive-object.test
+++ llvm/test/tools/llvm-objdump/non-archive-object.test
@@ -2,7 +2,7 @@
 # RUN: llvm-objdump -a %t | FileCheck %s
 
 # If this test has not crashed, then this test passed.
-# CHECK: file format ELF64-x86-64
+# CHECK: file format elf64-x86-64
 
 !ELF
 FileHeader:
Index: llvm/test/tools/llvm-objdump/file-headers-elf.test
===
--- llvm/test/tools/llvm-objdump/file-headers-elf.test
+++ llvm/test/tools/llvm-objdump/file-headers-elf.test
@@ -10,7 +10,7 @@
   Machine: EM_X86_64
   Entry:   0x123456789abcde
 
-# ELF64: [[FILE]]: file format ELF64-x86-64
+# ELF64: [[FILE]]: file format elf64-x86-64
 # ELF64: architecture: x86_64
 # ELF64: start address: 0x00123456789abcde
 
@@ -18,7 +18,7 @@
 # RUN: llvm-objdump -f %t-i386 | FileCheck -DFILE=%t-i386 %s -check-prefix ELF32
 # RUN: llvm-objdump -file-headers %t-i386 | FileCheck %s -DFILE=%t-i386 -check-prefix ELF32
 
-# ELF32: [[FILE]]: file format ELF32-i386
+# ELF32: [[FILE]]: file format elf32-i386
 # ELF32: architecture: i386
 # ELF32: start address: 0x12345678
 
Index: llvm/test/tools/llvm-objdump/file-headers-coff.test
===
--- llvm/test/tools/llvm-objdump/file-headers-coff.test
+++ llvm/test/tools/llvm-objdump/file-headers-coff.test
@@ -9,6 +9,6 @@
 sections:
 symbols:
 
-# CHECK: [[FILE]]: file format COFF-i386
+# CHECK: [[FILE]]: file format coff-i386
 # CHECK: architecture: i386
 # CHECK: start address: 0x
Index: llvm/test/tools/llvm-objdump/archive-headers.test
===
--- llvm/test/tools/llvm-objdump/archive-headers.test
+++ llvm/test/tools/llvm-objdump/archive-headers.test
@@ -1,21 +1,21 @@
 # RUN: llvm-objdump -a %p/Inputs/liblong_filenames.a | FileCheck %s
 # RUN: llvm-objdump -archive-headers %p/Inputs/liblong_filenames.a | FileCheck %s
 
-# CHECK: {{.*}}liblong_filenames.a(1.o): file format ELF64-x86-64
+# CHECK: {{.*}}liblong_filenames.a(1.o): file format elf64-x86-64
 # CHECK: rw-r--r-- 204299/200  1416 

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

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D74433#1870647 , @MaskRay wrote:

> Wait. I wonder whether we can change llvm-readobj to use lower case names as 
> well. The following should be updated:
>
>   StringRef ELFObjectFile::getFileFormatName() const {
> bool IsLittleEndian = ELFT::TargetEndianness == support::little;
> switch (EF.getHeader()->e_ident[ELF::EI_CLASS]) {
> case ELF::ELFCLASS32:
>   switch (EF.getHeader()->e_machine) {
>   case ELF::EM_386:
> return "ELF32-i386";
>   case ELF::EM_IAMCU:
> return "ELF32-iamcu";
>   case ELF::EM_X86_64:
> return "ELF32-x86-64";
>   case ELF::EM_ARM:
>
>
> These are almost bfdnames, except that they use upper cases. I don't find a 
> compelling argument using upper case `ELF`, so I vote for changing these 
> `ELF` to `elf`. @jhenderson  @grimar @sbc100 thoughts?
>
>   // lib/Object/WasmObjectFile.cpp
>   StringRef WasmObjectFile::getFileFormatName() const { return "WASM"; }
>


Yep... I was originally thinking we could update those to be lowercase, but 
that's kinda a broader change, and I imagine most llvm developers are used to 
seeing "ELF64-..." and I'm not sure it's worth changing all the tools just so 
that objdump can be more GNU compatible.

I personally wouldn't mind it. If others are in favor of it too, I'm all for it 
and can make the change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74433



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

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

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht created this revision.
rupprecht added reviewers: MaskRay, jhenderson.
Herald added subscribers: llvm-commits, cfe-commits, kerbowa, nhaehnle, jvesely.
Herald added a reviewer: alexshap.
Herald added projects: clang, LLVM.

GNU objdump prints the file format in lowercase, e.g. `elf64-x86-64`. 
llvm-objdump prints `ELF64-x86-64` right now, even though piping that into 
llvm-objcopy refuses that as a valid arch to use.

As an example of a problem this causes, see: 
https://github.com/ClangBuiltLinux/linux/issues/779


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D74433

Files:
  clang/test/Modules/pch_container.m
  lld/test/COFF/savetemps.ll
  llvm/test/CodeGen/AArch64/arm64-simplest-elf.ll
  llvm/test/CodeGen/ARM/Windows/trivial-gnu-object.ll
  llvm/test/CodeGen/BPF/reloc-btf-2.ll
  llvm/test/CodeGen/BPF/reloc-btf.ll
  llvm/test/CodeGen/BPF/reloc.ll
  llvm/test/Object/AMDGPU/objdump.s
  llvm/test/Object/X86/objdump-disassembly-inline-relocations.test
  llvm/test/Object/X86/objdump-label.test
  llvm/test/Object/X86/objdump-trivial-object.test
  llvm/test/Object/dynamic-reloc.test
  llvm/test/Object/objdump-symbol-table.test
  llvm/test/tools/llvm-objdump/X86/disassemble-section-name.s
  llvm/test/tools/llvm-objdump/X86/elf-disassemble-symbol-labels-exec.test
  llvm/test/tools/llvm-objdump/X86/elf-dynamic-relocs.test
  llvm/test/tools/llvm-objdump/X86/output-ordering.test
  llvm/test/tools/llvm-objdump/X86/warn-missing-disasm-func.test
  llvm/test/tools/llvm-objdump/all-headers.test
  llvm/test/tools/llvm-objdump/archive-headers.test
  llvm/test/tools/llvm-objdump/file-headers-coff.test
  llvm/test/tools/llvm-objdump/file-headers-elf.test
  llvm/test/tools/llvm-objdump/non-archive-object.test
  llvm/test/tools/llvm-objdump/relocations-in-nonreloc.test
  llvm/tools/llvm-objdump/llvm-objdump.cpp

Index: llvm/tools/llvm-objdump/llvm-objdump.cpp
===
--- llvm/tools/llvm-objdump/llvm-objdump.cpp
+++ llvm/tools/llvm-objdump/llvm-objdump.cpp
@@ -2172,7 +2172,9 @@
   outs() << A->getFileName() << "(" << O->getFileName() << ")";
 else
   outs() << O->getFileName();
-outs() << ":\tfile format " << O->getFileFormatName() << "\n\n";
+outs() << ":\tfile format ";
+printLowerCase(O->getFileFormatName(), outs());
+outs() << "\n\n";
   }
 
   if (StartAddress.getNumOccurrences() || StopAddress.getNumOccurrences())
Index: llvm/test/tools/llvm-objdump/relocations-in-nonreloc.test
===
--- llvm/test/tools/llvm-objdump/relocations-in-nonreloc.test
+++ llvm/test/tools/llvm-objdump/relocations-in-nonreloc.test
@@ -7,7 +7,7 @@
 # RUN: yaml2obj --docnum=3 %s -o %t3
 # RUN: llvm-objdump -r %t3 | FileCheck %s -DFILE=%t3 --check-prefixes=FMT,REL --implicit-check-not={{.}}
 
-# FMT: [[FILE]]: file format ELF64-x86-64
+# FMT: [[FILE]]: file format elf64-x86-64
 
 # REL:  RELOCATION RECORDS FOR []:
 # REL-NEXT: 0123 R_X86_64_NONE *ABS*+0x141
Index: llvm/test/tools/llvm-objdump/non-archive-object.test
===
--- llvm/test/tools/llvm-objdump/non-archive-object.test
+++ llvm/test/tools/llvm-objdump/non-archive-object.test
@@ -2,7 +2,7 @@
 # RUN: llvm-objdump -a %t | FileCheck %s
 
 # If this test has not crashed, then this test passed.
-# CHECK: file format ELF64-x86-64
+# CHECK: file format elf64-x86-64
 
 !ELF
 FileHeader:
Index: llvm/test/tools/llvm-objdump/file-headers-elf.test
===
--- llvm/test/tools/llvm-objdump/file-headers-elf.test
+++ llvm/test/tools/llvm-objdump/file-headers-elf.test
@@ -10,7 +10,7 @@
   Machine: EM_X86_64
   Entry:   0x123456789abcde
 
-# ELF64: [[FILE]]: file format ELF64-x86-64
+# ELF64: [[FILE]]: file format elf64-x86-64
 # ELF64: architecture: x86_64
 # ELF64: start address: 0x00123456789abcde
 
@@ -18,7 +18,7 @@
 # RUN: llvm-objdump -f %t-i386 | FileCheck -DFILE=%t-i386 %s -check-prefix ELF32
 # RUN: llvm-objdump -file-headers %t-i386 | FileCheck %s -DFILE=%t-i386 -check-prefix ELF32
 
-# ELF32: [[FILE]]: file format ELF32-i386
+# ELF32: [[FILE]]: file format elf32-i386
 # ELF32: architecture: i386
 # ELF32: start address: 0x12345678
 
Index: llvm/test/tools/llvm-objdump/file-headers-coff.test
===
--- llvm/test/tools/llvm-objdump/file-headers-coff.test
+++ llvm/test/tools/llvm-objdump/file-headers-coff.test
@@ -9,6 +9,6 @@
 sections:
 symbols:
 
-# CHECK: [[FILE]]: file format COFF-i386
+# CHECK: [[FILE]]: file format coff-i386
 # CHECK: architecture: i386
 # CHECK: start address: 0x
Index: llvm/test/tools/llvm-objdump/archive-headers.test
===
--- llvm/test/tools/llvm-objdump/archive-headers.test
+++ 

[PATCH] D74070: [Clang] Don't let gen crash diagnostics fail when '#pragma clang __debug crash' is used

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

FYI, temporarily reverted this in fafddbd956dbe439787f6d717c247e648bb07ff5 
 since it 
was causing failures in `Clang :: Driver/crash-report.c`. I didn't see the 
failure on all buildbots (otherwise I could have just marked it XFAIL), though 
I may have been looking in the wrong place.

Sample breakage: 
http://lab.llvm.org:8011/builders/clang-with-thin-lto-ubuntu/builds/21321/steps/test-stage1-compiler/logs/stdio

Logs:

  Command Output (stderr):
  --
  
/home/buildslave/ps4-buildslave1/clang-with-thin-lto-ubuntu/llvm-project/clang/test/Driver/crash-report.c:48:11:
 error: CHECK: expected string not found in input
  // CHECK: Preprocessed source(s) and associated run script(s) are located at:
^
  :1:1: note: scanning from here
  
/home/buildslave/ps4-buildslave1/clang-with-thin-lto-ubuntu/llvm-project/clang/test/Driver/crash-report.c:50:1:
 error: unknown type name 'BAZ'
  ^


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74070



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

[PATCH] D74076: [Clang][Driver] Remove -M group options before generating crash diagnostics

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

FYI - reverted this too since I had conflicts when also reverting D74070 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74076



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

[PATCH] D71554: [llvm-ranlib] Handle -D and -U command line flag

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D71554#1789360 , @arichardson wrote:

> Also handle -h/-v as short options. Does the adjusted test look okay?


Sorry, didn't have time to take a second look before the holiday break -- yep, 
looks good, I didn't anticipate so many buildbot failures. Would be nice if the 
precommit bot tested Windows too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71554



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

[PATCH] D71635: [clang] Rename -frounding-math to -fexperimental-rounding-math and add -frounding-math back as a gcc-compat arg.

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht abandoned this revision.
rupprecht added a comment.

In D71635#1790611 , @MaskRay wrote:

> Before:
>
>   % clang -frounding-math -fsyntax-only -x c /dev/null
>   clang-10: warning: Support for floating point control option frounding-math 
> is incomplete and experimental [-Wexperimental-float-control]
>
>
> CC1 will do rounding math things.
>
> After
>
>   % clang -frounding-math -fsyntax-only -x c /dev/null
>   clang-10: warning: optimization flag '-frounding-math' is not supported 
> [-Wignored-optimization-argument]
>
>
> CC1 will not do rounding math things. -fexperimental-rounding-math if the 
> user really wants to use the feature.
>
> Is my understanding correct? If yes, this patch seems pretty reasonable to 
> me, because -frounding-math is currently incomplete/unsafe.
>
> You may consider not changing CC1 options as they are not user facing.


I landed D71671  instead, as apparently the 
-frounding-math option is safe, although perhaps incomplete


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71635



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

[PATCH] D71671: [clang] Remove -Wexperimental-float-control.

[Jordan Rupprecht via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG553a727f5f64: [clang] Remove -Wexperimental-float-control. 
(authored by rupprecht).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71671

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/Driver/ToolChains/Clang.cpp


Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -2440,15 +2440,7 @@
 switch (optID) {
 default:
   break;
-case options::OPT_frounding_math:
-case options::OPT_ftrapping_math:
-case options::OPT_ffp_exception_behavior_EQ:
-  D.Diag(clang::diag::warn_drv_experimental_fp_control_incomplete_opt)
-  << A->getOption().getName();
-  break;
 case options::OPT_ffp_model_EQ: {
-  D.Diag(clang::diag::warn_drv_experimental_fp_control_incomplete_opt)
-  << A->getOption().getName();
   // If -ffp-model= is seen, reset to fno-fast-math
   HonorINFs = true;
   HonorNaNs = true;
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -1137,9 +1137,6 @@
 // Warning for the experimental-isel options.
 def ExperimentalISel : DiagGroup<"experimental-isel">;
 
-// Warning for the experimental float control options.
-def ExperimentalFloatControl : DiagGroup<"experimental-float-control">;
-
 // A warning group specifically for warnings related to function
 // multiversioning.
 def FunctionMultiVersioning : DiagGroup<"function-multiversion">;
Index: clang/include/clang/Basic/DiagnosticDriverKinds.td
===
--- clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -441,10 +441,6 @@
   "-fexperimental-isel support is incomplete for this architecture at the 
current optimization level">,
   InGroup;
 
-def warn_drv_experimental_fp_control_incomplete_opt : Warning<
-  "Support for floating point control option %0 is incomplete and 
experimental">,
-  InGroup;
-
 def warn_drv_moutline_unsupported_opt : Warning<
   "The '%0' architecture does not support -moutline; flag ignored">,
   InGroup;


Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -2440,15 +2440,7 @@
 switch (optID) {
 default:
   break;
-case options::OPT_frounding_math:
-case options::OPT_ftrapping_math:
-case options::OPT_ffp_exception_behavior_EQ:
-  D.Diag(clang::diag::warn_drv_experimental_fp_control_incomplete_opt)
-  << A->getOption().getName();
-  break;
 case options::OPT_ffp_model_EQ: {
-  D.Diag(clang::diag::warn_drv_experimental_fp_control_incomplete_opt)
-  << A->getOption().getName();
   // If -ffp-model= is seen, reset to fno-fast-math
   HonorINFs = true;
   HonorNaNs = true;
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -1137,9 +1137,6 @@
 // Warning for the experimental-isel options.
 def ExperimentalISel : DiagGroup<"experimental-isel">;
 
-// Warning for the experimental float control options.
-def ExperimentalFloatControl : DiagGroup<"experimental-float-control">;
-
 // A warning group specifically for warnings related to function
 // multiversioning.
 def FunctionMultiVersioning : DiagGroup<"function-multiversion">;
Index: clang/include/clang/Basic/DiagnosticDriverKinds.td
===
--- clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -441,10 +441,6 @@
   "-fexperimental-isel support is incomplete for this architecture at the current optimization level">,
   InGroup;
 
-def warn_drv_experimental_fp_control_incomplete_opt : Warning<
-  "Support for floating point control option %0 is incomplete and experimental">,
-  InGroup;
-
 def warn_drv_moutline_unsupported_opt : Warning<
   "The '%0' architecture does not support -moutline; flag ignored">,
   InGroup;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62731: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D62731#1789122 , @rupprecht wrote:

> In D62731#1788902 , @andrew.w.kaylor 
> wrote:
>
> > In D62731#1788838 , @rupprecht 
> > wrote:
> >
> > > It seems the discussion of whether or not this is incomplete died out -- 
> > > I'd prefer to assume it is incomplete if there is no consensus. Mailed 
> > > D71635  to rename `-frounding-math` to 
> > > `-fexperimental-rounding-math`.
> > >
> > > Alternatively we could remove the warning. I still don't see a good 
> > > argument for the middle ground of having it called `-frounding-math` but 
> > > also generate a warning.
> >
> >
> > It's definitely incomplete but the results will not be any worse than you 
> > get when -frounding-math is ignored.
> >
> > My preference would be to change the text of the warning that is issued but 
> > allow -frounding-math to be enabled by this commit without requiring an 
> > additional option.
>
>
> If other reviewers agree, then let's just remove the warning. I can send a 
> patch tomorrow unless someone else wants to do that.


Mailed D71671  to do this.

If neither patch is acceptable, then I would like to revert this commit 
instead, as we are having issues with this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62731



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

[PATCH] D71671: [clang] Remove -Wexperimental-float-control.

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht created this revision.
rupprecht added reviewers: mibintc, chandlerc, echristo, rjmccall, kpn, 
erichkeane, rsmith, andrew.w.kaylor.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Per D62731 , the behavior of clang with 
`-frounding-math` is no worse than when the rounding flag was completely 
ignored, so remove this unnecessary warning.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71671

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/Driver/ToolChains/Clang.cpp


Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -2456,15 +2456,7 @@
 switch (optID) {
 default:
   break;
-case options::OPT_frounding_math:
-case options::OPT_ftrapping_math:
-case options::OPT_ffp_exception_behavior_EQ:
-  D.Diag(clang::diag::warn_drv_experimental_fp_control_incomplete_opt)
-  << A->getOption().getName();
-  break;
 case options::OPT_ffp_model_EQ: {
-  D.Diag(clang::diag::warn_drv_experimental_fp_control_incomplete_opt)
-  << A->getOption().getName();
   // If -ffp-model= is seen, reset to fno-fast-math
   HonorINFs = true;
   HonorNaNs = true;
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -1137,9 +1137,6 @@
 // Warning for the experimental-isel options.
 def ExperimentalISel : DiagGroup<"experimental-isel">;
 
-// Warning for the experimental float control options.
-def ExperimentalFloatControl : DiagGroup<"experimental-float-control">;
-
 // A warning group specifically for warnings related to function
 // multiversioning.
 def FunctionMultiVersioning : DiagGroup<"function-multiversion">;
Index: clang/include/clang/Basic/DiagnosticDriverKinds.td
===
--- clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -441,10 +441,6 @@
   "-fexperimental-isel support is incomplete for this architecture at the 
current optimization level">,
   InGroup;
 
-def warn_drv_experimental_fp_control_incomplete_opt : Warning<
-  "Support for floating point control option %0 is incomplete and 
experimental">,
-  InGroup;
-
 def warn_drv_moutline_unsupported_opt : Warning<
   "The '%0' architecture does not support -moutline; flag ignored">,
   InGroup;


Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -2456,15 +2456,7 @@
 switch (optID) {
 default:
   break;
-case options::OPT_frounding_math:
-case options::OPT_ftrapping_math:
-case options::OPT_ffp_exception_behavior_EQ:
-  D.Diag(clang::diag::warn_drv_experimental_fp_control_incomplete_opt)
-  << A->getOption().getName();
-  break;
 case options::OPT_ffp_model_EQ: {
-  D.Diag(clang::diag::warn_drv_experimental_fp_control_incomplete_opt)
-  << A->getOption().getName();
   // If -ffp-model= is seen, reset to fno-fast-math
   HonorINFs = true;
   HonorNaNs = true;
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -1137,9 +1137,6 @@
 // Warning for the experimental-isel options.
 def ExperimentalISel : DiagGroup<"experimental-isel">;
 
-// Warning for the experimental float control options.
-def ExperimentalFloatControl : DiagGroup<"experimental-float-control">;
-
 // A warning group specifically for warnings related to function
 // multiversioning.
 def FunctionMultiVersioning : DiagGroup<"function-multiversion">;
Index: clang/include/clang/Basic/DiagnosticDriverKinds.td
===
--- clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -441,10 +441,6 @@
   "-fexperimental-isel support is incomplete for this architecture at the current optimization level">,
   InGroup;
 
-def warn_drv_experimental_fp_control_incomplete_opt : Warning<
-  "Support for floating point control option %0 is incomplete and experimental">,
-  InGroup;
-
 def warn_drv_moutline_unsupported_opt : Warning<
   "The '%0' architecture does not support -moutline; flag ignored">,
   InGroup;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62731: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht closed this revision.
rupprecht added a comment.

In D62731#1788902 , @andrew.w.kaylor 
wrote:

> In D62731#1788838 , @rupprecht wrote:
>
> > It seems the discussion of whether or not this is incomplete died out -- 
> > I'd prefer to assume it is incomplete if there is no consensus. Mailed 
> > D71635  to rename `-frounding-math` to 
> > `-fexperimental-rounding-math`.
> >
> > Alternatively we could remove the warning. I still don't see a good 
> > argument for the middle ground of having it called `-frounding-math` but 
> > also generate a warning.
>
>
> It's definitely incomplete but the results will not be any worse than you get 
> when -frounding-math is ignored.
>
> My preference would be to change the text of the warning that is issued but 
> allow -frounding-math to be enabled by this commit without requiring an 
> additional option.


If other reviewers agree, then let's just remove the warning. I can send a 
patch tomorrow unless someone else wants to do that.

> I would also very much like to see this patch re-committed. It's currently in 
> the "approved" state. If anyone objects to this being committed, please use 
> the "request changes" action to indicate this.

It is already re-committed. 7f9b5138470db1dc58f3bc05631284c653c9ed7a 
 reapplied 
it, but IIUC it was not closed in phabricator due to leading whitespace in the 
commit message:

  Reapply af57dbf12e54 "Add support for options -frounding-math, 
ftrapping-math, -ffp-model=, and -ffp-exception-behavior="
  ...
  Differential Revision: https://reviews.llvm.org/D62731

The "Differential" needs to be the first thing, whitespace cannot come before 
it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62731



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

[PATCH] D62731: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

It seems the discussion of whether or not this is incomplete died out -- I'd 
prefer to assume it is incomplete if there is no consensus. Mailed D71635 
 to rename `-frounding-math` to 
`-fexperimental-rounding-math`.

Alternatively we could remove the warning. I still don't see a good argument 
for the middle ground of having it called `-frounding-math` but also generate a 
warning.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62731



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

[PATCH] D71635: [clang] Rename -frounding-math to -fexperimental-rounding-math and add -frounding-math back as a gcc-compat arg.

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht created this revision.
rupprecht added reviewers: mibintc, chandlerc, echristo, rjmccall, kpn, 
erichkeane, rsmith, andrew.w.kaylor.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

D62731  implements an incomplete/experimental 
version of rounding math, but since the flag doesn't have "experimental" in the 
name, the feedback that it is experimental is in the form of 
-Wexperimental-float-control.

Since this flag is already being accepted as a gcc compatability arg, put the 
current implementation being `-f[no-]experimental-rounding-math` instead, and 
allow `-frounding-math` as an ignored arg. Because "experimental" is now in the 
flag name, remove the warning -- it's now clear that the feature is 
incomplete/experimental when invoking it.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71635

Files:
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CodeGen/fpconstrained.c
  clang/test/CodeGen/fpconstrained.cpp
  clang/test/Driver/clang_f_opts.c
  clang/test/Driver/fp-model.c

Index: clang/test/Driver/fp-model.c
===
--- clang/test/Driver/fp-model.c
+++ clang/test/Driver/fp-model.c
@@ -43,9 +43,9 @@
 // RUN:   | FileCheck --check-prefix=WARN9 %s
 // WARN9: warning: overriding '-ffp-model=strict' option with '-fno-honor-nans' [-Woverriding-t-option]
 
-// RUN: %clang -### -ffp-model=strict -fno-rounding-math -c %s 2>&1 \
+// RUN: %clang -### -ffp-model=strict -fno-experimental-rounding-math -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=WARNa %s
-// WARNa: warning: overriding '-ffp-model=strict' option with '-fno-rounding-math' [-Woverriding-t-option]
+// WARNa: warning: overriding '-ffp-model=strict' option with '-fno-experimental-rounding-math' [-Woverriding-t-option]
 
 // RUN: %clang -### -ffp-model=strict -fno-signed-zeros -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=WARNb %s
@@ -70,12 +70,12 @@
 // RUN: %clang -### -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-NOROUND %s
 // CHECK-NOROUND: "-cc1"
-// CHECK-NOROUND: "-fno-rounding-math"
+// CHECK-NOROUND: "-fno-experimental-rounding-math"
 
-// RUN: %clang -### -frounding-math -c %s 2>&1 \
+// RUN: %clang -### -fexperimental-rounding-math -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-ROUND --implicit-check-not ffp-exception-behavior=strict %s
 // CHECK-ROUND: "-cc1"
-// CHECK-ROUND: "-frounding-math"
+// CHECK-ROUND: "-fexperimental-rounding-math"
 
 // RUN: %clang -### -ftrapping-math -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-TRAP %s
@@ -93,7 +93,7 @@
 // CHECK-FPM-FAST: "-mreassociate"
 // CHECK-FPM-FAST: "-freciprocal-math"
 // CHECK-FPM-FAST: "-ffp-contract=fast"
-// CHECK-FPM-FAST: "-fno-rounding-math"
+// CHECK-FPM-FAST: "-fno-experimental-rounding-math"
 // CHECK-FPM-FAST: "-ffast-math"
 // CHECK-FPM-FAST: "-ffinite-math-only"
 
@@ -101,37 +101,37 @@
 // RUN:   | FileCheck --check-prefix=CHECK-FPM-PRECISE %s
 // CHECK-FPM-PRECISE: "-cc1"
 // CHECK-FPM-PRECISE: "-ffp-contract=fast"
-// CHECK-FPM-PRECISE: "-fno-rounding-math"
+// CHECK-FPM-PRECISE: "-fno-experimental-rounding-math"
 
 // RUN: %clang -### -nostdinc -ffp-model=strict -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FPM-STRICT %s
 // CHECK-FPM-STRICT: "-cc1"
 // CHECK-FPM-STRICT: "-ftrapping-math"
-// CHECK-FPM-STRICT: "-frounding-math"
+// CHECK-FPM-STRICT: "-fexperimental-rounding-math"
 // CHECK-FPM-STRICT: "-ffp-exception-behavior=strict"
 
 // RUN: %clang -### -nostdinc -ftrapping-math -ffp-exception-behavior=ignore -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-TRAP-IGNORE %s
 // CHECK-TRAP-IGNORE: "-cc1"
-// CHECK-TRAP-IGNORE: "-fno-rounding-math"
+// CHECK-TRAP-IGNORE: "-fno-experimental-rounding-math"
 // CHECK-TRAP-IGNORE: "-ffp-exception-behavior=ignore"
 
 
 // RUN: %clang -### -nostdinc -ffp-exception-behavior=strict -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FEB-STRICT %s
 // CHECK-FEB-STRICT: "-cc1"
-// CHECK-FEB-STRICT: "-fno-rounding-math"
+// CHECK-FEB-STRICT: "-fno-experimental-rounding-math"
 // CHECK-FEB-STRICT: "-ffp-exception-behavior=strict"
 
 // RUN: %clang -### -nostdinc -ffp-exception-behavior=maytrap -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FEB-MAYTRAP %s
 // CHECK-FEB-MAYTRAP: "-cc1"
-// CHECK-FEB-MAYTRAP: "-fno-rounding-math"
+// CHECK-FEB-MAYTRAP: "-fno-experimental-rounding-math"
 // CHECK-FEB-MAYTRAP: "-ffp-exception-behavior=maytrap"
 
 // RUN: %clang -### -nostdinc -ffp-exception-behavior=ignore -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FEB-IGNORE %s
 // CHECK-FEB-IGNORE: "-cc1"
-// CHECK-FEB-IGNORE: "-fno-rounding-math"
+// CHECK-FEB-IGNORE: "-fno-experimental-rounding-math"
 // CHECK-FEB-IGNORE: 

[PATCH] D71554: [llvm-ranlib] Handle -D and -U command line flag

[Jordan Rupprecht via Phabricator via cfe-commits]

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

Looks good for D/U, but looks like --help and --version options are also 
supported as combined short args; do you mind adding those too while you're 
here?

Thanks for the patch!




Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:68
+  -h --help - Display available options
+  --version - Display the version of this program
+  -D- Use zero for timestamps and uids/gids (default)

`-v --version`



Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1164-1176
+} else if (arg.consume_front("-")) {
+  // Handle the -D/-U flag
+  while (!arg.empty()) {
+if (arg.front() == 'D') {
+  Deterministic = true;
+} else if (arg.front() == 'U') {
+  Deterministic = false;

```
  } else if (arg.front() == 'h') {
printHelpMessage();
return 0;
  } else if (arg.front() == 'v') {
cl::PrintVersionMessage();
return 0;
  }
```

Actually, `ranlib -vh` and `ranlib -hv` (on my machine at least) both print the 
help message. I don't think we need *that* level of compatibility though -- we 
can go with first-one-wins.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71554



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

[PATCH] D62731: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:444
 
+def warn_drv_experimental_fp_control_incomplete_opt : Warning<
+  "Support for floating point control option %0 is incomplete and 
experimental">,

andrew.w.kaylor wrote:
> rupprecht wrote:
> > mibintc wrote:
> > > @kpn thought it would be a good idea to add a Warning that the 
> > > implementation of float control is experimental and partially 
> > > implemented.  That's what this is for.
> > Instead of adding a warning, I'd like to propose `-frounding-math` not be 
> > enabled unless an additional flag (e.g. `-fexperimental-float-control`) is 
> > passed. Or maybe this feature should be called 
> > `-f[no-]experimental-rounding-math` instead.
> > 
> > There are plenty of builds that are already specifying `-frounding-math` 
> > (e.g. they also support building w/ a compiler such as gcc that implements 
> > this), and adding this experimental/incomplete implementation is a surprise 
> > to those builds.
> > 
> > If I'm wrong and it's completely safe to ignore the warning (maybe it's 
> > incomplete but not in any unsafe way), we should just not have it at all.
> You raise an interesting point about people who might be using 
> -frounding-math already. However, if they are using this flag because they 
> also sometimes build with a compiler such as gcc that supports the flag, they 
> are currently getting incorrect behavior from clang. Without this patch, 
> clang silently ignores the option and the optimizer silently ignores the fact 
> that the program may be changing the rounding mode dynamically. The user may 
> or may not be aware of this.
> 
> With this patch such a user is likely to observe two effects: (1) their code 
> will suddenly get slower, and (2) it will probably start behaving correctly 
> with regard to rounding mode changes. The rounding behavior will definitely 
> not get worse. I think the warning is useful as an indication that something 
> has changed. I don't think requiring an additional option should be necessary.
> However, if they are using this flag because they also sometimes build with a 
> compiler such as gcc that supports the flag, they are currently getting 
> incorrect behavior from clang

Incorrect, yes, but also likely valid behavior. "experimental" seems to imply a 
miscompile when using this option should not be unexpected.

As I suggested before: if I'm wrong, and this behavior is only going to make 
the code more correct (as you suggest), can we remove the warning that this 
must be ack'd explicitly by adding `-Wno-experimental-float-control` to builds? 
I don't understand the motivation for the warning.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62731



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

[PATCH] D62731: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:444
 
+def warn_drv_experimental_fp_control_incomplete_opt : Warning<
+  "Support for floating point control option %0 is incomplete and 
experimental">,

mibintc wrote:
> @kpn thought it would be a good idea to add a Warning that the implementation 
> of float control is experimental and partially implemented.  That's what this 
> is for.
Instead of adding a warning, I'd like to propose `-frounding-math` not be 
enabled unless an additional flag (e.g. `-fexperimental-float-control`) is 
passed. Or maybe this feature should be called 
`-f[no-]experimental-rounding-math` instead.

There are plenty of builds that are already specifying `-frounding-math` (e.g. 
they also support building w/ a compiler such as gcc that implements this), and 
adding this experimental/incomplete implementation is a surprise to those 
builds.

If I'm wrong and it's completely safe to ignore the warning (maybe it's 
incomplete but not in any unsafe way), we should just not have it at all.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62731



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

[PATCH] D62731: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Jordan Rupprecht via Phabricator via cfe-commits]

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



Comment at: clang/lib/Sema/SemaExpr.cpp:13047
+  F->setUsesFPIntrin(true);
+  printf("Enclosing function uses fp intrinsics\n");
+}

rupprecht wrote:
> Looks like this is leftover debugging? I'm seeing log spam compiling some 
> files -- this message repeated hundreds of times. I'll go ahead and create a 
> patch that nukes this.
Sorry for the noise, looks like f4a7d5659df7cb56c1baa34a39e9fe2639472741 
already did this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62731



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

[PATCH] D62731: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:13047
+  F->setUsesFPIntrin(true);
+  printf("Enclosing function uses fp intrinsics\n");
+}

Looks like this is leftover debugging? I'm seeing log spam compiling some files 
-- this message repeated hundreds of times. I'll go ahead and create a patch 
that nukes this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62731



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

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

Just wanna say huge +1 for this patch. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67122



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

[PATCH] D68669: [llvm-objdump][WIP] Make llvm-objdump -h compatible with GNU objdump.

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht planned changes to this revision.
rupprecht added a comment.

Note: herald added reviewers, but this patch is just to provide context. I'll 
send the real patches for review in the coming days.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68669



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

[PATCH] D68669: [llvm-objdump][WIP] Make llvm-objdump -h compatible with GNU objdump.

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht created this revision.
Herald added subscribers: llvm-commits, cfe-commits, seiya, arphaman, 
jakehehrlich, aheejin, arichardson, sbc100, emaste.
Herald added a reviewer: espindola.
Herald added a reviewer: alexshap.
Herald added a reviewer: jhenderson.
Herald added projects: clang, LLVM.
rupprecht planned changes to this revision.
rupprecht added a comment.

Note: herald added reviewers, but this patch is just to provide context. I'll 
send the real patches for review in the coming days.


Note: this patch is large and not intended for submission as-is. Instead, this 
patch presents a poor implementation that makes llvm-objdump GNU compatibile 
for this option (with all existing tests passing, but few added tests, hacky 
code, etc.), and will serve as context for smaller changes to be submitted 
separately with more careful review.

llvm-objdump -h was implemented to be similar to readelf -S (see rL141579 
). However, it is not completely compatible 
with that, and anyone that does want headers displayed that way can use 
llvm-readelf -S now that it exists. Make llvm-objdump compatible with GNU 
objdump instead.

A brief overview of changes:

- Add file offset (with implementations for all filetypes supported by 
llvm-readobj).
- Add 2**n section alignment column (with implementations for all filetypes 
supported by llvm-readobj).
- Section numbers are not the actual section numbers, but something different, 
corresponding to libbfd section numbers. llvm-readelf -s prints the actual 
section numbers if those are desired.
- Filter out certain sections like symtabs/strtabs/relocs. The actual logic is 
a lot more convoluted (and probably isn't fully compatibile, but is pretty 
close).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D68669

Files:
  clang/test/Modules/pch_container.m
  lld/test/ELF/bss-start-common.s
  lld/test/ELF/edata-etext.s
  lld/test/ELF/edata-no-bss.s
  lld/test/ELF/emit-relocs-gc.s
  lld/test/ELF/gc-sections-metadata.s
  lld/test/ELF/init_fini_priority.s
  lld/test/ELF/invalid-fde-rel.s
  lld/test/ELF/linkerscript/addr.test
  lld/test/ELF/linkerscript/align-empty.test
  lld/test/ELF/linkerscript/align1.test
  lld/test/ELF/linkerscript/align2.test
  lld/test/ELF/linkerscript/align3.test
  lld/test/ELF/linkerscript/at2.test
  lld/test/ELF/linkerscript/constructor.test
  lld/test/ELF/linkerscript/define.test
  lld/test/ELF/linkerscript/double-bss.test
  lld/test/ELF/linkerscript/eh-frame-emit-relocs.s
  lld/test/ELF/linkerscript/emit-reloc-section-names.s
  lld/test/ELF/linkerscript/expr-sections.test
  lld/test/ELF/linkerscript/input-sec-dup.s
  lld/test/ELF/linkerscript/insert-after.test
  lld/test/ELF/linkerscript/insert-before.test
  lld/test/ELF/linkerscript/memory-include.test
  lld/test/ELF/linkerscript/memory.s
  lld/test/ELF/linkerscript/memory3.s
  lld/test/ELF/linkerscript/memory4.test
  lld/test/ELF/linkerscript/memory5.test
  lld/test/ELF/linkerscript/multi-sections-constraint.s
  lld/test/ELF/linkerscript/non-absolute2.test
  lld/test/ELF/linkerscript/numbers.s
  lld/test/ELF/linkerscript/orphan.s
  lld/test/ELF/linkerscript/orphans.s
  lld/test/ELF/linkerscript/out-of-order-section-in-region.test
  lld/test/ELF/linkerscript/out-of-order.s
  lld/test/ELF/linkerscript/output-section-include.test
  lld/test/ELF/linkerscript/region-alias.s
  lld/test/ELF/linkerscript/repsection-va.s
  lld/test/ELF/linkerscript/section-include.test
  lld/test/ELF/linkerscript/sections-constraint.s
  lld/test/ELF/linkerscript/sections-gc2.s
  lld/test/ELF/linkerscript/sections-keep.s
  lld/test/ELF/linkerscript/sections-sort.s
  lld/test/ELF/linkerscript/sections.s
  lld/test/ELF/linkerscript/sizeof.s
  lld/test/ELF/linkerscript/symbol-only.test
  lld/test/ELF/linkerscript/va.s
  lld/test/ELF/linkerscript/wildcards.s
  lld/test/ELF/linkerscript/wildcards2.s
  lld/test/ELF/relocatable-sections.s
  lld/test/ELF/relocatable.s
  lld/test/ELF/relro-omagic.s
  lld/test/ELF/section-name.s
  lld/test/ELF/sectionstart-noallochdr.s
  lld/test/ELF/sectionstart.s
  lld/test/ELF/strip-all.s
  lld/test/ELF/synthetic-got.s
  llvm/test/MC/COFF/assoc-private.s
  llvm/test/Object/objdump-no-sectionheaders.test
  llvm/test/Object/objdump-sectionheaders.test
  llvm/test/ObjectYAML/CodeView/sections.yaml
  llvm/test/tools/llvm-objcopy/ELF/symtab-error-on-remove-strtab.test
  llvm/test/tools/llvm-objdump/X86/adjust-vma.test
  llvm/test/tools/llvm-objdump/X86/macho-section-headers.test
  llvm/test/tools/llvm-objdump/X86/phdrs-lma.test
  llvm/test/tools/llvm-objdump/X86/phdrs-lma2.test
  llvm/test/tools/llvm-objdump/X86/section-index.s
  llvm/test/tools/llvm-objdump/section-filter.test
  llvm/test/tools/llvm-objdump/wasm.txt
  llvm/test/tools/llvm-objdump/xcoff-section-headers.test
  llvm/tools/llvm-objdump/llvm-objdump.cpp
  llvm/tools/llvm-objdump/llvm-objdump.h

Index: llvm/tools/llvm-objdump/llvm-objdump.h

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

There's definitely a lot of new findings this creates, but it's hard to say 
exactly how many root causes there are due to the way test failures are (not) 
grouped well in the way I'm testing. So far they all seem like true positives, 
so this would be good to submit. However a few are positive yet benign, like 
this interesting one (simplified):

  void ParseString(char *s) {
char *next = s;
for (char *end = s; end; next = end + 1) { // ubsan error computing (nil + 
1), although it doesn't matter because the loop terminates when end == nil and 
next is not read after the loop
  // ...
  end = strchr(next, 'x'); // returns null if not found
  // ...
}
  }

If I had to guesstimate, I'd say 20-100 bugs in a couple billion lines of code, 
so a lot, but shouldn't be too disruptive to anyone that has these checks 
enabled globally.

I haven't noticed any timeouts -- which is not to say this isn't a slowdown, 
but at least it's not egregious.

BTW, here's a minimal + complete repro of the original issue:

  $ cat ub.cc
  #include 
  #include 
  
  static void Test(const char *x, int offset) {
printf("%p + %d => %s\n", x, offset, x + offset ? "true" : "false");
  }
  
  int main(int argc, char **argv) {
if (argc != 3) return 1;
  
const char *x = reinterpret_cast(atoi(argv[1]));
int offset = atoi(argv[2]);
  
Test(x, offset);
  
return 0;
  }
  $ previous-clang++ -O3 ub.cc && ./a.out 0 1
  (nil) + 1 => true
  $ next-clang++ -O3 ub.cc && ./a.out 0 1
  (nil) + 1 => false
  $ patch-D67122-clang++ -O3 -fsanitize=undefined ub.cc && ./a.out 0 1
  ubsan: ub.cc:5:42: runtime error: applying non-zero offset 1 to null pointer  

  
  (nil) + 1 => false


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67122



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

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

> But TLDR, either the fix in https://github.com/google/filament/pull/1566
>  is incorrect and the actually-bad code is elsewhere,
>  or you have some other unsanitized UB elsewhere. Could be both :S

My money is on "both" :p

I tested a random sample of a couple thousand tests internally and ~1% of them 
failed, but when I looked at them, they were all coming from two separate 
widely used libraries. I fixed those, and I'll do a broader test now to see how 
bad the long tail of issues are.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67122



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

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

> Still think this looks good. Have you tried running this on the llvm test 
> suite, or some other interesting corpus? Would be curious to see any pre/post 
> patch numbers.

I finally had time this morning to patch this in and give it a shot. (Sorry for 
the delay... it's been a real busy week :( )

First, starting off with the good news: I reverted all the fixes I made, and 
now all the tests fail when running w/ ubsan. Yay!

The error we see in each case is `UndefinedBehaviorSanitizer: 
nullptr-with-nonzero-offset` with the logs containing `runtime error: applying 
non-zero offset  to null pointer`. Which catches the two places where 
we were adding some non-zero offset to nullptr, but doesn't seem to catch the 
nullptr-after-nonzero-offset case in 
https://github.com/google/filament/pull/1566 -- instead, it fails later, when 
the pointer with a value of nullptr is incremented. (Or... maybe this is 
actually a separate bug. Hmm. Needs some more testing...)

At any rate, I have some more tests to run to get some idea of what % of code 
this would flag as being bad.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67122



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

[PATCH] D66490: [NewPM] Enable the New Pass Manager by Default in Clang

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

We already know that we don't want this enabled for tsan builds due to 
https://bugs.llvm.org/show_bug.cgi?id=42877, but I don't even know if anyone 
else will hit it (it's only when building one particular library).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66490



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

[PATCH] D63625: [CodeGen][test] Use -fno-discard-value-names for better test support

[Jordan Rupprecht via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG73986707bd57: [CodeGen][test] Use FileCheck variable 
matchers for better test support (authored by rupprecht).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63625

Files:
  clang/test/CodeGenCXX/debug-info-nrvo.cpp


Index: clang/test/CodeGenCXX/debug-info-nrvo.cpp
===
--- clang/test/CodeGenCXX/debug-info-nrvo.cpp
+++ clang/test/CodeGenCXX/debug-info-nrvo.cpp
@@ -1,5 +1,10 @@
-// RUN: %clangxx -target x86_64-unknown-unknown -g %s -emit-llvm -S -o - | 
FileCheck %s
-// RUN: %clangxx -target x86_64-unknown-unknown -g -fno-elide-constructors %s 
-emit-llvm -S -o - | FileCheck %s -check-prefix=NOELIDE
+// RUN: %clangxx -target x86_64-unknown-unknown -g \
+// RUN:   %s -emit-llvm -S -o - | FileCheck %s
+
+// RUN: %clangxx -target x86_64-unknown-unknown -g \
+// RUN:   -fno-elide-constructors %s -emit-llvm -S -o - | \
+// RUN:   FileCheck %s -check-prefix=NOELIDE
+
 struct Foo {
   Foo() = default;
   Foo(Foo &) { x = other.x; }
@@ -21,8 +26,10 @@
 // Check that NRVO variables are stored as a pointer with deref if they are
 // stored in the return register.
 
-// CHECK: %result.ptr = alloca i8*, align 8
-// CHECK: call void @llvm.dbg.declare(metadata i8** %result.ptr,
+// CHECK: %[[RESULT:.*]] = alloca i8*, align 8
+// CHECK: call void @llvm.dbg.declare(metadata i8** %[[RESULT]],
 // CHECK-SAME: metadata !DIExpression(DW_OP_deref)
-// NOELIDE: call void @llvm.dbg.declare(metadata %struct.Foo* %foo,
+
+// NOELIDE: %[[FOO:.*]] = alloca %struct.Foo, align 4
+// NOELIDE: call void @llvm.dbg.declare(metadata %struct.Foo* %[[FOO]],
 // NOELIDE-SAME:metadata !DIExpression()


Index: clang/test/CodeGenCXX/debug-info-nrvo.cpp
===
--- clang/test/CodeGenCXX/debug-info-nrvo.cpp
+++ clang/test/CodeGenCXX/debug-info-nrvo.cpp
@@ -1,5 +1,10 @@
-// RUN: %clangxx -target x86_64-unknown-unknown -g %s -emit-llvm -S -o - | FileCheck %s
-// RUN: %clangxx -target x86_64-unknown-unknown -g -fno-elide-constructors %s -emit-llvm -S -o - | FileCheck %s -check-prefix=NOELIDE
+// RUN: %clangxx -target x86_64-unknown-unknown -g \
+// RUN:   %s -emit-llvm -S -o - | FileCheck %s
+
+// RUN: %clangxx -target x86_64-unknown-unknown -g \
+// RUN:   -fno-elide-constructors %s -emit-llvm -S -o - | \
+// RUN:   FileCheck %s -check-prefix=NOELIDE
+
 struct Foo {
   Foo() = default;
   Foo(Foo &) { x = other.x; }
@@ -21,8 +26,10 @@
 // Check that NRVO variables are stored as a pointer with deref if they are
 // stored in the return register.
 
-// CHECK: %result.ptr = alloca i8*, align 8
-// CHECK: call void @llvm.dbg.declare(metadata i8** %result.ptr,
+// CHECK: %[[RESULT:.*]] = alloca i8*, align 8
+// CHECK: call void @llvm.dbg.declare(metadata i8** %[[RESULT]],
 // CHECK-SAME: metadata !DIExpression(DW_OP_deref)
-// NOELIDE: call void @llvm.dbg.declare(metadata %struct.Foo* %foo,
+
+// NOELIDE: %[[FOO:.*]] = alloca %struct.Foo, align 4
+// NOELIDE: call void @llvm.dbg.declare(metadata %struct.Foo* %[[FOO]],
 // NOELIDE-SAME:metadata !DIExpression()
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63625: [CodeGen][test] Use -fno-discard-value-names for better test support

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

Sounds good, changed to use variable matching instead. This passes w/ either 
`-fno-discard-value-names` or `-fdiscard-value-names` used.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63625



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

[PATCH] D63625: [CodeGen][test] Use -fno-discard-value-names for better test support

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht updated this revision to Diff 205912.
rupprecht added a comment.

- Use filecheck variable matching instead of an explicit 
-fno-discard-value-names option


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63625

Files:
  clang/test/CodeGenCXX/debug-info-nrvo.cpp


Index: clang/test/CodeGenCXX/debug-info-nrvo.cpp
===
--- clang/test/CodeGenCXX/debug-info-nrvo.cpp
+++ clang/test/CodeGenCXX/debug-info-nrvo.cpp
@@ -1,5 +1,10 @@
-// RUN: %clangxx -target x86_64-unknown-unknown -g %s -emit-llvm -S -o - | 
FileCheck %s
-// RUN: %clangxx -target x86_64-unknown-unknown -g -fno-elide-constructors %s 
-emit-llvm -S -o - | FileCheck %s -check-prefix=NOELIDE
+// RUN: %clangxx -target x86_64-unknown-unknown -g \
+// RUN:   %s -emit-llvm -S -o - | FileCheck %s
+
+// RUN: %clangxx -target x86_64-unknown-unknown -g \
+// RUN:   -fno-elide-constructors %s -emit-llvm -S -o - | \
+// RUN:   FileCheck %s -check-prefix=NOELIDE
+
 struct Foo {
   Foo() = default;
   Foo(Foo &) { x = other.x; }
@@ -21,8 +26,10 @@
 // Check that NRVO variables are stored as a pointer with deref if they are
 // stored in the return register.
 
-// CHECK: %result.ptr = alloca i8*, align 8
-// CHECK: call void @llvm.dbg.declare(metadata i8** %result.ptr,
+// CHECK: %[[RESULT:.*]] = alloca i8*, align 8
+// CHECK: call void @llvm.dbg.declare(metadata i8** %[[RESULT]],
 // CHECK-SAME: metadata !DIExpression(DW_OP_deref)
-// NOELIDE: call void @llvm.dbg.declare(metadata %struct.Foo* %foo,
+
+// NOELIDE: %[[FOO:.*]] = alloca %struct.Foo, align 4
+// NOELIDE: call void @llvm.dbg.declare(metadata %struct.Foo* %[[FOO]],
 // NOELIDE-SAME:metadata !DIExpression()


Index: clang/test/CodeGenCXX/debug-info-nrvo.cpp
===
--- clang/test/CodeGenCXX/debug-info-nrvo.cpp
+++ clang/test/CodeGenCXX/debug-info-nrvo.cpp
@@ -1,5 +1,10 @@
-// RUN: %clangxx -target x86_64-unknown-unknown -g %s -emit-llvm -S -o - | FileCheck %s
-// RUN: %clangxx -target x86_64-unknown-unknown -g -fno-elide-constructors %s -emit-llvm -S -o - | FileCheck %s -check-prefix=NOELIDE
+// RUN: %clangxx -target x86_64-unknown-unknown -g \
+// RUN:   %s -emit-llvm -S -o - | FileCheck %s
+
+// RUN: %clangxx -target x86_64-unknown-unknown -g \
+// RUN:   -fno-elide-constructors %s -emit-llvm -S -o - | \
+// RUN:   FileCheck %s -check-prefix=NOELIDE
+
 struct Foo {
   Foo() = default;
   Foo(Foo &) { x = other.x; }
@@ -21,8 +26,10 @@
 // Check that NRVO variables are stored as a pointer with deref if they are
 // stored in the return register.
 
-// CHECK: %result.ptr = alloca i8*, align 8
-// CHECK: call void @llvm.dbg.declare(metadata i8** %result.ptr,
+// CHECK: %[[RESULT:.*]] = alloca i8*, align 8
+// CHECK: call void @llvm.dbg.declare(metadata i8** %[[RESULT]],
 // CHECK-SAME: metadata !DIExpression(DW_OP_deref)
-// NOELIDE: call void @llvm.dbg.declare(metadata %struct.Foo* %foo,
+
+// NOELIDE: %[[FOO:.*]] = alloca %struct.Foo, align 4
+// NOELIDE: call void @llvm.dbg.declare(metadata %struct.Foo* %[[FOO]],
 // NOELIDE-SAME:metadata !DIExpression()
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63625: [CodeGen][test] Use -fno-discard-value-names for better test support

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht created this revision.
rupprecht added reviewers: rnk, akhuang, aprantl.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Depending on how clang is built, it may discard the IR names and use names like 
`%2` instead of `%result.ptr`, causing tests that rely on the IR name to fail. 
Using `fno-discard-value-names` ensures the actual name is present regardless 
of the build mode.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D63625

Files:
  clang/test/CodeGenCXX/debug-info-nrvo.cpp


Index: clang/test/CodeGenCXX/debug-info-nrvo.cpp
===
--- clang/test/CodeGenCXX/debug-info-nrvo.cpp
+++ clang/test/CodeGenCXX/debug-info-nrvo.cpp
@@ -1,5 +1,10 @@
-// RUN: %clangxx -target x86_64-unknown-unknown -g %s -emit-llvm -S -o - | 
FileCheck %s
-// RUN: %clangxx -target x86_64-unknown-unknown -g -fno-elide-constructors %s 
-emit-llvm -S -o - | FileCheck %s -check-prefix=NOELIDE
+// RUN: %clangxx -target x86_64-unknown-unknown -g -fno-discard-value-names \
+// RUN:   %s -emit-llvm -S -o - | FileCheck %s
+
+// RUN: %clangxx -target x86_64-unknown-unknown -g -fno-discard-value-names \
+// RUN:   -fno-elide-constructors %s -emit-llvm -S -o - | \
+// RUN:   FileCheck %s -check-prefix=NOELIDE
+
 struct Foo {
   Foo() = default;
   Foo(Foo &) { x = other.x; }


Index: clang/test/CodeGenCXX/debug-info-nrvo.cpp
===
--- clang/test/CodeGenCXX/debug-info-nrvo.cpp
+++ clang/test/CodeGenCXX/debug-info-nrvo.cpp
@@ -1,5 +1,10 @@
-// RUN: %clangxx -target x86_64-unknown-unknown -g %s -emit-llvm -S -o - | FileCheck %s
-// RUN: %clangxx -target x86_64-unknown-unknown -g -fno-elide-constructors %s -emit-llvm -S -o - | FileCheck %s -check-prefix=NOELIDE
+// RUN: %clangxx -target x86_64-unknown-unknown -g -fno-discard-value-names \
+// RUN:   %s -emit-llvm -S -o - | FileCheck %s
+
+// RUN: %clangxx -target x86_64-unknown-unknown -g -fno-discard-value-names \
+// RUN:   -fno-elide-constructors %s -emit-llvm -S -o - | \
+// RUN:   FileCheck %s -check-prefix=NOELIDE
+
 struct Foo {
   Foo() = default;
   Foo(Foo &) { x = other.x; }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60974: Clang IFSO driver action.

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a reviewer: rupprecht.
rupprecht added a comment.

Can you upload this patch with context? Either use arc or upload w/ -U9

I seem to have a lot of comments, but they're all nits -- my major concern 
being the `llvm_unreachable`s should be errors instead (i.e. should be 
triggered in release builds).

Since this is clearly labeled as experimental, I think you should feel free to 
commit if you can get another lgtm (@compnerd?)




Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3614-3616
+  Args.hasArg(options::OPT_iterface_stub_version_EQ)
+  ? Args.getLastArgValue(options::OPT_iterface_stub_version_EQ)
+  : "")

`Args.getLastArgValue(options::OPT_iterface_stub_version_EQ)` should already 
default to returning the empty string if unset (note the default param)



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1678-1680
+  Args.hasArg(OPT_iterface_stub_version_EQ)
+  ? Args.getLastArgValue(OPT_iterface_stub_version_EQ)
+  : "")

(same here)



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1688
+  if (!ProgramActionPair.second)
+llvm_unreachable("Must specify a valid interface stub format.");
+  Opts.ProgramAction = ProgramActionPair.first;

I think this is very much reachable if someone provides 
`--interface-stub-version=x`



Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:20-21
+  CompilerInstance 
+  StringRef InFile = "";
+  StringRef Format = "";
+  std::set ParsedTemplates;

nit: these default initializations are redundant



Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:26
+  struct MangledSymbol {
+std::string ParentName = "";
+uint8_t Type;

ditto



Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:39-41
+if (!(RDO & FromTU))
+  return true;
+if (Symbols.find(ND) != Symbols.end())

Can you add a comment why these two are excluded?



Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:41
+  return true;
+if (Symbols.find(ND) != Symbols.end())
+  return true;

llvm::is_contained(Symbols, ND)



Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:44-47
+if (isa(ND))
+  return true;
+if (isa(ND))
+  return true;

This would be terser (and more readable) combined; `if (isa(ND) || 
isa(ND))`



Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:95
+  index::CodegenNameGenerator CGNameGen(ND->getASTContext());
+  auto MangledNames = CGNameGen.getAllManglings(ND);
+  if (MangledNames.size() == 1)

auto -> std::vector, unless the return type is obvious to anyone 
that commits here (i.e. not me)



Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:107
+  index::CodegenNameGenerator CGNameGen(ND->getASTContext());
+  auto MangledName = CGNameGen.getName(ND);
+  auto MangledNames = CGNameGen.getAllManglings(ND);

auto -> std::string



Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:206-207
+// catch anything that's not a VarDecl or Template/FunctionDecl.
+llvm_unreachable("clang -emit-iterface-stubs: Expected a function or "
+ "function template decl.");
+return false;

This should be an error, not llvm_unreachable (which I think gets filtered out 
in release builds)



Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:242
+MangledSymbols Symbols;
+auto OS = Instance.createDefaultOutputFile(/*Binary=*/false, InFile, 
"ifo");
+if (!OS)

elsewhere it looks like this was changed to "ifs"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60974



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

[PATCH] D61689: Change -gz and -Wa,--compress-debug-sections to use gABI compression (SHF_COMPRESSED)

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

So, while I think this is an //entirely// reasonable assumption in most cases, 
it's also not one that provides any kind of workaround for the few cases where 
it's not universally true.

- As mentioned in the patch, this effectively changes the default from 
`-gz=zlib-gnu` to `-gz=zlib`. Anyone with an older toolchain that wants the old 
behavior can set `-gz=zlib-gnu`. Seems OK so far.
- However, passing the `-gz` flag //also// implies sending 
`--compress-debug-sections=XXX` to the assembler, which -- if someone is using 
`-no-integrated-as` has an older toolchain -- is not a supported option (at 
least the variant that takes a value).

In other works, for the few users that have an older toolchain, this provides 
literally no workaround. Not setting the flag causes failures with an older 
linker, and setting the flag causes failures with the assembler.

(echristo reverted this in rL360703 , I 
wanted to add some context)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61689



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

[PATCH] D60974: Clang IFSO driver action.

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

I didn't follow the technical details, but I don't see anything wrong with 
moving forward on this patch. I think this seems like an interesting idea worth 
experimenting with.

In D60974#1488563 , @jakehehrlich 
wrote:

> > Jake, I am still not sure what you would prefer for moving forward. The 
> > current patch can output the yaml format I originally proposed or the one 
> > that looks similar to the one you proposed. What I need to know is:
> > 
> > Do you want the merging if the "ifo" files to happen inside of llvm-elfabi?
> >  Do you care if we upstream the yaml we proposed as a first step (which is 
> > really a distilled version of that yaml2obj can consume anyways. this right 
> > now functions actually) ???
> >  Or, would you rather the ifo files be merged by a different separate tool 
> > (something maybe called llvm-ifsogen)???
>
> This is my proposal:
>
> 1. We should plan on adding the code ofr merging these files inside of 
> `llvm/tools/llvm-elfabi` but will it be a separate tool from `llvm-elfabi`. 
> You can create "separate" tools in the same directory using the symlink 
> trick. See llvm-ar and friends or llvm-objcopy and llvm-strip. The tool name 
> can be discussed in review.


The symlink trick is usually used when the new frontend tool is just a thin 
layer over the underlying tool. Is that going to be the case here?

For example,

- `llvm-ranlib` is equivalent to `llvm-ar s`
- `llvm-readelf` is equivalent to `llvm-readobj --elf-output-style=GNU`
- `llvm-addr2line` is equivalent to `llvm-symbolizer --output-style=GNU`
- `llvm-strip` is equivalent to `llvm-objcopy --strip-all` (maybe not exactly)

In other words, is `llvm-mergeifo` (placeholder name) going to be roughly 
equivalent to `llvm-elfabi --merge-ifo` (again, placeholder flag name)? And if 
so, it doesn't seem like a symlink is strictly necessary -- users can just call 
`llvm-elfabi --merge-ifo`. Am I missing something?

> 2. The yaml proposed thus far is not acceptable IMO for reasons already 
> outlined but this can be discussed in code review. Before adding sections a 
> clear reason for needing them is required which hasn't been given yet. Things 
> can evolve and change later as needed but we should start with the minimum 
> and add as needed later; not start with extra and remove later. Support for 
> the new format should be added into TextAPI and in the review for that 
> process we should discuss the format. After we add support for this new 
> format into TextAPI we can add support for emitting this format into clang 
> (well actually you can write the code whenever but I'm using "after" in a 
> different sense here).

What if we named it experimental for now? i.e. `--experimental-emit-ifo` for 
the clang flag name and `!experimental-ifo-elf-v1` for the yaml id? That would 
allow those working on this patch to play around with more features, but still 
give sufficient warning to anyone that fields they depend on may be removed.

In fact, if we label it experimental (or maybe even if we don't), I don't see 
any reason this couldn't land now, even without a consumer of it. So what if a 
tool produces a yaml file that we don't haven't finished the yaml parser for? 
Does anything break?




Comment at: clang/lib/Driver/Driver.cpp:3449-3450
   return C.MakeAction(Input, types::TY_Nothing);
+if (Args.hasArg(options::OPT_emit_ifso))
+  return C.MakeAction(Input, types::TY_IFO);
 return C.MakeAction(Input, types::TY_LLVM_BC);

bikeshed: I don't think we should have both ifo and ifso. Either name sounds 
fine to me, but if running "-emit-ifso" gives me an ifo and not an ifso, that's 
very surprising, and is going to be a major headache trying to remember when to 
call it ifo and when to call it ifso.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60974



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

[PATCH] D59609: [clang][OpenMP] Fix build when using libgomp

[Jordan Rupprecht via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC356614: [clang][OpenMP] Fix build when using libgomp 
(authored by rupprecht, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D59609?vs=191567=191577#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D59609

Files:
  unittests/AST/OMPStructuredBlockTest.cpp


Index: unittests/AST/OMPStructuredBlockTest.cpp
===
--- unittests/AST/OMPStructuredBlockTest.cpp
+++ unittests/AST/OMPStructuredBlockTest.cpp
@@ -55,7 +55,7 @@
   StringRef ExpectedPrinted,
   PolicyAdjusterType PolicyAdjuster = None) {
   std::vector Args = {
-  "-fopenmp",
+  "-fopenmp=libomp",
   };
   return PrintedStmtMatches(Code, Args, NodeMatch, ExpectedPrinted,
 PolicyAdjuster);


Index: unittests/AST/OMPStructuredBlockTest.cpp
===
--- unittests/AST/OMPStructuredBlockTest.cpp
+++ unittests/AST/OMPStructuredBlockTest.cpp
@@ -55,7 +55,7 @@
   StringRef ExpectedPrinted,
   PolicyAdjusterType PolicyAdjuster = None) {
   std::vector Args = {
-  "-fopenmp",
+  "-fopenmp=libomp",
   };
   return PrintedStmtMatches(Code, Args, NodeMatch, ExpectedPrinted,
 PolicyAdjuster);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59609: [clang][OpenMP] Fix build when using libgomp

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D59609#1436975 , @lebedev.ri wrote:

> Interesting.
>  What happens if libomp is not being built?
>  What happens if libomp is not present?


I'm far from an omp expert (only running across this because some tests 
failed), but I would guess both those cases to break.

But this is broken anyway when libomp isn't being used. So the set of broken 
cases is going from (people that don't use libomp) to (people that don't have 
libomp) which I think is a much smaller subset.

> Or more generally, why does it even matter whether there is runtime or not, 
> this only does paring+sema, no codegen/execution.

I'm very surprised myself. Hence my question "has something gone wrong" on 
D59214 . Hopefully this fix/hack can be 
reverted once that's figured out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59609



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