[PATCH] D155610: [Clang][Sema] Fix display of characters on static assertion failure

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/SemaCXX/static-assert.cpp:287
+  static_assert((char16_t)L'ゆ' == L"C̵̭̯̠̎͌ͅť̺"[1], ""); // expected-error 
{{failed}} \
+  // expected-note {{evaluates 
to ''ゆ' (0x3086) == '̵' (0x335)'}}
+  static_assert(L"＼／"[1] == u'\xFFFD', ""); // expected-error {{failed}} \

cor3ntin wrote:
> hubert.reinterpretcast wrote:
> > tahonermann wrote:
> > > cor3ntin wrote:
> > > > hubert.reinterpretcast wrote:
> > > > > hubert.reinterpretcast wrote:
> > > > > > cor3ntin wrote:
> > > > > > > hubert.reinterpretcast wrote:
> > > > > > > > The C++23 escaped string formatting facility would not generate 
> > > > > > > > a trailing combining character like this. I recommend following 
> > > > > > > > suit.
> > > > > > > > 
> > > > > > > > Info on U+0335: 
> > > > > > > > https://util.unicode.org/UnicodeJsps/character.jsp?a=0335
> > > > > > > > 
> > > > > > > This is way outside the scope of the patch. The diagnostic output 
> > > > > > > facility has no understanding of combining characters or 
> > > > > > > graphemes and do not attempt to match std::print. It probably 
> > > > > > > would be an improvement but this patch is not trying to modify 
> > > > > > > how all diagnostics are printed. (all of that logic is in 
> > > > > > > Diagnostic.cpp)
> > > > > > This patch is pushing the envelope of what appears in diagnostics. 
> > > > > > One can also argue that someone writing
> > > > > > ```
> > > > > > static_assert(false, "\u0301");
> > > > > > ```
> > > > > > gets what they deserve, but that case does not have a big problem 
> > > > > > anyway (because the provided message text appears after `: `).
> > > > > > 
> > > > > > This patch increases the exposure of the diagnostic output facility 
> > > > > > to input that it does not handle well. I disagree that it is 
> > > > > > outside the scope of this patch to insist that it does not generate 
> > > > > > such inputs to the diagnostic output facility (even if a possible 
> > > > > > solution is to modify the diagnostic output facility first).
> > > > > @cor3ntin, do you have status quo examples for how grapheme-extending 
> > > > > characters that are not already "problematic" in their original 
> > > > > context are emitted in diagnostics in contexts where they are?
> > > > Are you looking for that sort of examples? 
> > > > https://godbolt.org/z/c79xWr7Me
> > > > That shows that clang has no understanding of graphemes
> > > gcc and MSVC get that case "right" (probably by accident). 
> > > https://godbolt.org/z/Tjd6xnEon
> > I was more looking for cases where the output grapheme includes elements 
> > that were part of the fixed message text (gobbling quotes, etc.). Also, 
> > this patch is in the wrong shape for handling this concern in the 
> > diagnostic printer because the delimiting of the replacement text happens 
> > in this patch.
> Could you craft a message that becomes a graphene after substitution by the 
> engine? Maybe? You would have to try very hard and `static_assert` 
> diagnostics are not of the right shape.
> 
> > This patch increases the exposure of the diagnostic output facility to 
> > input that it does not handle well. I disagree that it is outside the scope 
> > of this patch to insist that it does not generate such inputs to the 
> > diagnostic output facility (even if a possible solution is to modify the 
> > diagnostic output facility first).
> 
> I still don't see it. None of the output produce in this patch are even close 
> to what i could be problematic. ie this patch is only ever producing ASCII or 
> single codepoints that gets escaped when they are not printable
> Could you craft a message that becomes a graphene after substitution by the 
> engine? Maybe? You would have to try very hard and `static_assert` 
> diagnostics are not of the right shape.

That is what I meant by this patch introducing new situations.

> > This patch increases the exposure of the diagnostic output facility to 
> > input that it does not handle well. I disagree that it is outside the scope 
> > of this patch to insist that it does not generate such inputs to the 
> > diagnostic output facility (even if a possible solution is to modify the 
> > diagnostic output facility first).
> 
> I still don't see it. None of the output produce in this patch are even close 
> to what i could be problematic. ie this patch is only ever producing ASCII or 
> single codepoints that gets escaped when they are not printable

This patch produces single codepoints that are not escaped even when they may 
combine with a `'` delimiter. This patch also (currently) forms the string with 
the `'` and the potentially-combining character directly adjacent to each 
other. The least this patch should do is to emit the potentially-combining 
character to the diagnostic facility as a separate substitution (that is, if we 
can agree that the 

[PATCH] D155610: [Clang][Sema] Fix display of characters on static assertion failure

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16970
+  OS << '\'';
+  WriteCharValueForDiagnostic(CodeUnit, BTy, TyWidth, OS);
+  OS << "' (0x"

To have the diagnostic printer handle separating any potential grapheme (if it 
is capable of doing so--potentially in the future), we need to isolate the 
result of `WriteCharValueForDiagnostic` in a separate message substitution.



Comment at: clang/test/SemaCXX/static-assert.cpp:287
+  static_assert((char16_t)L'ゆ' == L"C̵̭̯̠̎͌ͅť̺"[1], ""); // expected-error 
{{failed}} \
+  // expected-note {{evaluates 
to ''ゆ' (0x3086) == '̵' (0x335)'}}
+  static_assert(L"＼／"[1] == u'\xFFFD', ""); // expected-error {{failed}} \

tahonermann wrote:
> cor3ntin wrote:
> > hubert.reinterpretcast wrote:
> > > hubert.reinterpretcast wrote:
> > > > cor3ntin wrote:
> > > > > hubert.reinterpretcast wrote:
> > > > > > The C++23 escaped string formatting facility would not generate a 
> > > > > > trailing combining character like this. I recommend following suit.
> > > > > > 
> > > > > > Info on U+0335: 
> > > > > > https://util.unicode.org/UnicodeJsps/character.jsp?a=0335
> > > > > > 
> > > > > This is way outside the scope of the patch. The diagnostic output 
> > > > > facility has no understanding of combining characters or graphemes 
> > > > > and do not attempt to match std::print. It probably would be an 
> > > > > improvement but this patch is not trying to modify how all 
> > > > > diagnostics are printed. (all of that logic is in Diagnostic.cpp)
> > > > This patch is pushing the envelope of what appears in diagnostics. One 
> > > > can also argue that someone writing
> > > > ```
> > > > static_assert(false, "\u0301");
> > > > ```
> > > > gets what they deserve, but that case does not have a big problem 
> > > > anyway (because the provided message text appears after `: `).
> > > > 
> > > > This patch increases the exposure of the diagnostic output facility to 
> > > > input that it does not handle well. I disagree that it is outside the 
> > > > scope of this patch to insist that it does not generate such inputs to 
> > > > the diagnostic output facility (even if a possible solution is to 
> > > > modify the diagnostic output facility first).
> > > @cor3ntin, do you have status quo examples for how grapheme-extending 
> > > characters that are not already "problematic" in their original context 
> > > are emitted in diagnostics in contexts where they are?
> > Are you looking for that sort of examples? https://godbolt.org/z/c79xWr7Me
> > That shows that clang has no understanding of graphemes
> gcc and MSVC get that case "right" (probably by accident). 
> https://godbolt.org/z/Tjd6xnEon
I was more looking for cases where the output grapheme includes elements that 
were part of the fixed message text (gobbling quotes, etc.). Also, this patch 
is in the wrong shape for handling this concern in the diagnostic printer 
because the delimiting of the replacement text happens in this patch.


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

https://reviews.llvm.org/D155610

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

[PATCH] D153907: [AIX] [TOC] Add -mtocdata/-mno-tocdata options on AIX

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/docs/UsersManual.rst:3161
+static storage duration, including static data members of classes and
+block-scope static variables will be marked with the toc-data attribute.
+Alternatively, the user can specify a comma separated list of external linkage

The "toc-data attribute" is an implementation detail. The user-facing 
documentation should not talk about it.



Comment at: clang/docs/UsersManual.rst:3173
+which variables will be an exception to -mno-tocdata. Alternatively, a user
+may specify -mtocdata to mark all suitable variables with toc-data and specify
+-mno-tocdata= to add which variables will be an exception to -mtocdata.

Please document the conditions for a variable to be "suitable".



Comment at: clang/docs/UsersManual.rst:3178
+
+  Mark all external linkage variables and variables with static storage 
duration
+  that are not explicitly specified with -mno-tocdata= as toc-data.

The use of "all" is incorrect. Please qualify with "suitable" (and address this 
issue elsewhere in the text where applicable).



Comment at: clang/docs/UsersManual.rst:3159
 
+Mark TOC Data / Not TOC Data (AIX-specific)
+---

amyk wrote:
> 
@syzaara, I see the comment has been addressed but it is not marked as "done". 
The author is responsible for marking comments as "done" in Phabricator. Please 
mark all comments that you believe have been addressed as "done" so that other 
parties know if the comment is still pending action.


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

https://reviews.llvm.org/D153907

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

[PATCH] D158739: AIX: Issue an error when specifying an alias for a common symbol

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

Aside from the comments Digger has made, I have no additional concerns about 
this patch. It is an improvement (although there are adjacent cases that need 
further handling).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158739

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

[PATCH] D155610: [Clang][Sema] Fix display of characters on static assertion failure

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/SemaCXX/static-assert.cpp:287
+  static_assert((char16_t)L'ゆ' == L"C̵̭̯̠̎͌ͅť̺"[1], ""); // expected-error 
{{failed}} \
+  // expected-note {{evaluates 
to ''ゆ' (0x3086) == '̵' (0x335)'}}
+  static_assert(L"＼／"[1] == u'\xFFFD', ""); // expected-error {{failed}} \

hubert.reinterpretcast wrote:
> cor3ntin wrote:
> > hubert.reinterpretcast wrote:
> > > The C++23 escaped string formatting facility would not generate a 
> > > trailing combining character like this. I recommend following suit.
> > > 
> > > Info on U+0335: https://util.unicode.org/UnicodeJsps/character.jsp?a=0335
> > > 
> > This is way outside the scope of the patch. The diagnostic output facility 
> > has no understanding of combining characters or graphemes and do not 
> > attempt to match std::print. It probably would be an improvement but this 
> > patch is not trying to modify how all diagnostics are printed. (all of that 
> > logic is in Diagnostic.cpp)
> This patch is pushing the envelope of what appears in diagnostics. One can 
> also argue that someone writing
> ```
> static_assert(false, "\u0301");
> ```
> gets what they deserve, but that case does not have a big problem anyway 
> (because the provided message text appears after `: `).
> 
> This patch increases the exposure of the diagnostic output facility to input 
> that it does not handle well. I disagree that it is outside the scope of this 
> patch to insist that it does not generate such inputs to the diagnostic 
> output facility (even if a possible solution is to modify the diagnostic 
> output facility first).
@cor3ntin, do you have status quo examples for how grapheme-extending 
characters that are not already "problematic" in their original context are 
emitted in diagnostics in contexts where they are?


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

https://reviews.llvm.org/D155610

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

[PATCH] D158158: [clang] Set FP options in Sema when instantiating CompoundStmt

[Hubert Tong via Phabricator via cfe-commits]

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

LGTM; thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158158

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

[PATCH] D155610: [Clang][Sema] Fix display of characters on static assertion failure

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16920-16926
+if (llvm::ConvertCodePointToUTF8(Value, Ptr)) {
+  OS << StringRef(Arr, Ptr - Arr);
+} else {
+  OS << "\\x"
+ << llvm::format_hex_no_prefix(Value, TyWidth / 4,
+   /*Upper=*/true);
+}

Minor nit: Braces no longer needed.


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

https://reviews.llvm.org/D155610

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

[PATCH] D155610: [Clang][Sema] Fix display of characters on static assertion failure

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16867-16868
 
+/// Convert character's code unit value to a string.
+/// The code point needs to be zero-extended to 32-bits.
+static void WriteCharValueForDiagnostic(uint32_t Value, const BuiltinType *BTy,

Suggest wording tweaks.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16894-16895
+if (llvm::ConvertCodePointToUTF8(Value, Ptr)) {
+  for (char *I = Arr; I != Ptr; ++I)
+OS << *I;
+} else {

Try using `StringRef`.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16897
+} else {
+  // FIXME: This assumes Unicode literal encodings
+  OS << "\\x"

Since the function interface has been clarified, this part actually doesn't 
need a FIXME. The FIXME should instead be added to the comment above the 
function declaration.


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

https://reviews.llvm.org/D155610

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

[PATCH] D156237: Complete the implementation of P2361 Unevaluated string literals

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

Thanks, @cor3ntin, for addressing my feedback. I am not familiar enough with 
various aspects of it to approve it, but I see Aaron has already approved it 
and I think all comments have been addressed.
I also appreciate that the patch helps towards making the internally-used 
encoding less exposed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156237

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

[PATCH] D155610: [Clang][Sema] Fix display of characters on static assertion failure

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16876
+  // other types.
+  if (CodePoint <= UCHAR_MAX) {
+StringRef Escaped = escapeCStyle(CodePoint);

cor3ntin wrote:
> hubert.reinterpretcast wrote:
> > For types other than `Char_S`, `Char_U`, and `Char8`, this fails to treat 
> > the C1 Controls and Latin-1 Supplement characters as Unicode code points. 
> > It looks like test coverage for these cases are missing.
> `escapeCStyle` is one of the things that assume ASCII / UTF, but yes, we 
> might as well reduce to 0x7F just to avoid unnecessary work
> `escapeCStyle` is one of the things that assume ASCII / UTF, but yes, we 
> might as well reduce to 0x7F just to avoid unnecessary work

I meant (with a `signed char` type to trigger the assertion):
```
:1:28: note: expression evaluates to ''' (0xA2) == '' (0xA2)'
1 | static_assert(u"\u00a2"[0] == '');
  |   ~^
```

should be:
```
:1:28: note: expression evaluates to ''¢' (0xA2) == '' (0xA2)'
1 | static_assert(u"\u00a2"[0] == '');
  |   ~^
```



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

https://reviews.llvm.org/D155610

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

[PATCH] D155610: [Clang][Sema] Fix display of characters on static assertion failure

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D155610#4586213 , 
@abhina.sreeskantharajan wrote:

> I've discussed offline with @hubert.reinterpretcast and agree with him that 
> with the addition of fexec-charset support, the set of characters deemed 
> printable will not be accurate when other encodings are used. This will be 
> similar to the printf/scanf format string validation issue I mentioned in my 
> RFC and would require us to reverse the conversion or keep the original 
> string around to check if the character is printable. I don't think we have 
> finalized a solution on how to handle these issues yet.

Furthermore, it is reasonable for the scope of the current patch to focus on 
producing UTF-8 (or UTF-16 for Windows) output to the "terminal".


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

https://reviews.llvm.org/D155610

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

[PATCH] D156237: Complete the implementation of P2361 Unevaluated string literals

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/Parse/ParseDecl.cpp:431-436
+if (Expr.isInvalid()) {
+  SawError = true;
+  break;
+} else {
+  Exprs.push_back(Expr.get());
+}

cor3ntin wrote:
> aaron.ballman wrote:
> > 
> Oups, sorry @Fznamznon, I'll fix that!
It seems Aaron's request to move the code out of the `else` was not acted on.



Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:2337
+// Emits the list of arguments that should be parsed as unevaluated string
+// literals for each attributes
+static void emitClangAttrUnevaluatedStringLiteralList(RecordKeeper ,

Minor nit: Grammar



Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:2345
+uint32_t Bits = 0;
+for (uint32_t N = 0; N < Args.size(); ++N) {
+  Bits |= (isStringLiteralArgument(Args[N]) << N);

Do we know that `Args.size()` is less than 32?



Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:2349-2350
+  if (isVariadicStringLiteralArgument(Args[N])) {
+for (; N < sizeof(uint32_t) * CHAR_BIT; ++N)
+  Bits |= (1 << N);
+break;

```
#include 
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156237

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

[PATCH] D155610: [Clang][Sema] Fix display of characters on static assertion failure

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D155610#4575579 , @cor3ntin wrote:

> @hubert.reinterpretcast It does not, Unicode characters are only escaped in 
> Diagnostics.cpp, and I think this is what we want.

Thanks @cor3ntin for the insight. I agree that this is a separate concern that 
also applies to static assert messages.


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

https://reviews.llvm.org/D155610

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

[PATCH] D155610: [Clang][Sema] Fix display of characters on static assertion failure

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16899
+  uint32_t CodePoint = 
static_cast(V.getInt().getZExtValue());
+  PrintCharLiteralPrefix(BTy->getKind(), OS);
+  OS << '\'';

cor3ntin wrote:
> hubert.reinterpretcast wrote:
> > hubert.reinterpretcast wrote:
> > > cor3ntin wrote:
> > > > Looking at the diagnostics, I don't think it makes sense to print a 
> > > > prefix here. You could just leave that part out.
> > > Why is removing the prefix better? The types can matter (characters 
> > > outside the basic character set are allowed to have negative `char` 
> > > values). Also, moving forward, the value of a character need not be the 
> > > same in the various encodings.
> > Some fun with signedness (imagine a more realistic example with 
> > `ISO-8859-1` ordinary character encoding with a signed `char` type):
> > ```
> > $ clang -Xclang -fwchar-type=short -xc++ -<<<$'static_assert(L"\\uFF10"[0] 
> > == U\'\\uFF10\');'
> > :1:15: error: static assertion failed due to requirement 
> > 'L"\xFF10"[0] == U'\uff10''
> > 1 | static_assert(L"\uFF10"[0] == U'\uFF10');
> >   |   ^
> > :1:28: note: expression evaluates to ''０' (0xFF10) == '０' (0xFF10)'
> > 1 | static_assert(L"\uFF10"[0] == U'\uFF10');
> >   |   ~^~~~
> > 1 error generated.
> > Return:  0x01:1   Fri Aug 11 23:49:02 2023 EDT
> > ```
> Either we care about the actual character - ie `'a'`, or it's value (ie 
> `42`). The motivation for the current patch is to add the value to the 
> diagnostic message.
> I'm also concerned about mixing things that are are are not lexical elements 
> in the diagnostics
Maybe the //motivation// for the current patch is to add the value, but what it 
does (for wide characters as defined in C) is to add the character (and 
obfuscate the value).

Observe the status quo (https://godbolt.org/z/Wc6nKvTMn):
```
note: expression evaluates to '-240 == 65296'
```

From the output higher up (with this patch), we see two "identical" characters 
and values (due to lack of decimal value output). With decimal value output 
added, it will still be potentially confusing why the two identical characters 
have different values (without some sort of type annotation).

I admit that the confusion arises in the status quo treatment of `signed char` 
and `unsigned char`. I hope I am using the word correctly when I say that it is 
ironic that the patch breaks in one context what it seeks to fix in another.




Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16869
+/// The code point needs to be zero-extended to 32-bits.
+void ConvertCharToString(uint32_t CodePoint, const BuiltinType *BTy,
+ unsigned TyWidth, llvm::raw_ostream ) {

It does not seem that the first parameter expects a `CodePoint` argument in all 
cases. For `Char_S`, `Char_U`, and `Char8`, it seems the function wants to 
treat the input as a UTF-8 code unit.

I suggest changing the argument to be clearly a code unit (and potentially 
treat it as a code point value as appropriate later in the function).

Also: The function should probably be declared as having static linkage.
Additionally: The function does not "convert" in the language semantic sense. 
`WriteCharacterValueDescriptionForDisplay` might be a better name.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16876
+  // other types.
+  if (CodePoint <= UCHAR_MAX) {
+StringRef Escaped = escapeCStyle(CodePoint);

For types other than `Char_S`, `Char_U`, and `Char8`, this fails to treat the 
C1 Controls and Latin-1 Supplement characters as Unicode code points. It looks 
like test coverage for these cases are missing.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16930-16936
+case BuiltinType::Char_S:
+case BuiltinType::Char_U:
+case BuiltinType::Char8:
+case BuiltinType::Char16:
+case BuiltinType::Char32:
+case BuiltinType::WChar_S:
+case BuiltinType::WChar_U: {

cor3ntin wrote:
> hubert.reinterpretcast wrote:
> > Add FIXME for `char` and `wchar_t` cases that this assumes Unicode literal 
> > encodings.
> If we wanted such fixme, it should be L1689.
The `ConvertCharToString` has a first parameter called `CodePoint`. With that 
interface[^1], it is sensible to insert conversion from the applicable literal 
encoding to a Unicode code point value here (thus my request for a FIXME here).

You are probably right that the FIXME belongs elsewhere. If you were thinking 
what I am thinking, then I am guessing you meant L16894? That is where the 
`ConvertCharToString` function seems to assume that a `wchar_t` value is 
directly a "code point value". To generate hex escapes, the function needs to 
be passed the original value (including for `char`s, e.g., to handle stray code 

[PATCH] D155610: [Clang][Sema] Fix display of characters on static assertion failure

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16899
+  uint32_t CodePoint = 
static_cast(V.getInt().getZExtValue());
+  PrintCharLiteralPrefix(BTy->getKind(), OS);
+  OS << '\'';

hubert.reinterpretcast wrote:
> cor3ntin wrote:
> > Looking at the diagnostics, I don't think it makes sense to print a prefix 
> > here. You could just leave that part out.
> Why is removing the prefix better? The types can matter (characters outside 
> the basic character set are allowed to have negative `char` values). Also, 
> moving forward, the value of a character need not be the same in the various 
> encodings.
Some fun with signedness (imagine a more realistic example with `ISO-8859-1` 
ordinary character encoding with a signed `char` type):
```
$ clang -Xclang -fwchar-type=short -xc++ -<<<$'static_assert(L"\\uFF10"[0] == 
U\'\\uFF10\');'
:1:15: error: static assertion failed due to requirement 'L"\xFF10"[0] 
== U'\uff10''
1 | static_assert(L"\uFF10"[0] == U'\uFF10');
  |   ^
:1:28: note: expression evaluates to ''０' (0xFF10) == '０' (0xFF10)'
1 | static_assert(L"\uFF10"[0] == U'\uFF10');
  |   ~^~~~
1 error generated.
Return:  0x01:1   Fri Aug 11 23:49:02 2023 EDT
```


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

https://reviews.llvm.org/D155610

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

[PATCH] D156237: Complete the implementation of P2361 Unevaluated string literals

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/Parser/c2x-attributes.c:29
+[[deprecated(L"abc")]] void unevaluated_string(void);
+// expected-warning@-1 {{encoding prefix 'L' on an unevaluated string literal 
has no effec}}
+

"Typo" fix.



Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:2336
 
+// Emits the list of arguments that shoulkd be parsed as unevaluated string
+// literals for each attributes

Typo fix



Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:2349
+  if (isVariadicStringLiteralArgument(Args[N])) {
+for (; N < sizeof(uint32_t); N++)
+  Bits |= (1 << N);

barannikov88 wrote:
> maskTrailingZeros might also be useful
@cor3ntin, this comment appears unaddressed.

Additionally, (just for future reference) `++N` is suggested for the loop: 
https://llvm.org/docs/CodingStandards.html#prefer-preincrement.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156237

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

[PATCH] D155610: [Clang][Sema] Fix display of characters on static assertion failure

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16930-16936
+case BuiltinType::Char_S:
+case BuiltinType::Char_U:
+case BuiltinType::Char8:
+case BuiltinType::Char16:
+case BuiltinType::Char32:
+case BuiltinType::WChar_S:
+case BuiltinType::WChar_U: {

Add FIXME for `char` and `wchar_t` cases that this assumes Unicode literal 
encodings.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16942-16945
+  OS << "' (0x"
+ << llvm::format_hex_no_prefix(CodePoint, /*Width=*/2,
+   /*Upper=*/true)
+ << ')';

@aaron.ballman, hex output hides signedness. I think we want hex //and// 
decimal.



Comment at: clang/test/SemaCXX/static-assert.cpp:287
+  static_assert((char16_t)L'ゆ' == L"C̵̭̯̠̎͌ͅť̺"[1], ""); // expected-error 
{{failed}} \
+  // expected-note {{evaluates 
to ''ゆ' (0x3086) == '̵' (0x335)'}}
+  static_assert(L"＼／"[1] == u'\xFFFD', ""); // expected-error {{failed}} \

The C++23 escaped string formatting facility would not generate a trailing 
combining character like this. I recommend following suit.

Info on U+0335: https://util.unicode.org/UnicodeJsps/character.jsp?a=0335



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

https://reviews.llvm.org/D155610

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

[PATCH] D155610: [Clang][Sema] Fix display of characters on static assertion failure

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16899
+  uint32_t CodePoint = 
static_cast(V.getInt().getZExtValue());
+  PrintCharLiteralPrefix(BTy->getKind(), OS);
+  OS << '\'';

cor3ntin wrote:
> Looking at the diagnostics, I don't think it makes sense to print a prefix 
> here. You could just leave that part out.
Why is removing the prefix better? The types can matter (characters outside the 
basic character set are allowed to have negative `char` values). Also, moving 
forward, the value of a character need not be the same in the various encodings.


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

https://reviews.llvm.org/D155610

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

[PATCH] D154359: [clang] Reset FP options before template instantiation

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

This patch seems to be direct cause of a regression: 
https://github.com/llvm/llvm-project/issues/64605.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154359

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

[PATCH] D155610: [Clang][Sema] Fix display of characters on static assertion failure

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D155610#4575579 , @cor3ntin wrote:

> @hubert.reinterpretcast It does not, Unicode characters are only escaped in 
> Diagnostics.cpp, and I think this is what we want.
> Currently, llvm assume UTF-8 terminal, except on Windows where we convert to 
> UTF-16 and use the wide windows APIs (`raw_fd_ostream::write_impl`).

I am skeptical of the extent to which that assumption is exercised in a 
problematic manner today. The characters being emitted (aside from the [U+0020, 
U+007E] fixed message text itself) generally come from the text of the source 
file, which is generally written using characters that the user can display 
(even if they are not "basic Latin" characters).


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

https://reviews.llvm.org/D155610

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

[PATCH] D157201: [Clang] Support qualified name as member designator in offsetof

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/SemaCXX/offsetof.cpp:106
+int x3[__builtin_offsetof(struct X2, X2::static_a) == 0 ? 1 : -1]; // 
expected-error{{no member named 'static_a'}}
+int x4[__builtin_offsetof(struct X2, X2::X2) == 0 ? 1 : -1]; // 
expected-error{{no member named 'X2'}}
+

aaron.ballman wrote:
> yichi170 wrote:
> > aaron.ballman wrote:
> > > There's one more test I'd like to see:
> > > ```
> > > struct S {
> > >   int Foo;
> > > };
> > > 
> > > template 
> > > void func() {
> > >   static_assert(__builtin_offsetof(Ty, Ty::Foo) == 0, "");
> > > }
> > > 
> > > void inst() {
> > >   func();
> > > }
> > > ```
> > It would get the compile error in the current patch, but I think it should 
> > be compiled without any error, right?
> Correct, that should be accepted: https://godbolt.org/z/1f6a9Yaxa
Should expect this to pass too:
```
template 
struct Z {
  static_assert(!__builtin_offsetof(T, template Q::x));
};

struct A {
  template  using Q = T;
  int x;
};

Z za;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157201

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

[PATCH] D153701: [WIP][Clang] Implement P2718R0 "Lifetime extension in range-based for loops"

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D153701#4563919 , @yronglin wrote:

> The gap between these two numbers is very large. So I'think we can create 
> additional materializations only within for-range initializers. I'm not sure 
> if I can find a way to only create materializes for temporaries that need to 
> have an extended lifetime, WDYT?

@rsmith asked for something slightly different (but I am not sure how to 
collect that info): He wanted the AST size delta.

For what we have, I think the detailed results table would be more useful if we 
add some size metric for the files and the increase expressed as a percentage. 
Perhaps sorting by file size would help contextualize the significance of the 
percentage change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153701

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

[PATCH] D156596: [Clang] Produce a warning instead of an error in unevaluated strings before C++26

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast accepted this revision.
hubert.reinterpretcast added a comment.

In D156596#4573354 , @aaron.ballman 
wrote:

> LGTM but please wait for @hubert.reinterpretcast to confirm the new tests 
> cover what he was looking for.

LGTM; thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156596

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

[PATCH] D157201: [Clang] Support qualified name as member designator in offsetof

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/SemaCXX/offsetof.cpp:104
+struct X2 { int a; static int static_a; };
+int x2[__builtin_offsetof(struct X2, X2::a) == 0 ? 1 : -1];
+int x3[__builtin_offsetof(struct X2, X2::static_a) == 0 ? 1 : -1]; // 
expected-error{{no member named 'static_a'}}

Tests should include cases where the member is a direct member of a base class 
or an anonymous union therein (and named variously by the qualification with 
the base class name, the complete class name, an intermediate base class name, 
the base class name qualified with an intermediate base class, the base class 
name denoted using `::identity_t`, etc.).

Additionally, there should be a diagnostic test case where the qualifier is for 
an unrelated identically-defined class in another namespace.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157201

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

[PATCH] D155610: [Clang][Sema] Fix display of characters on static assertion failure

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added subscribers: abhina.sreeskantharajan, 
hubert.reinterpretcast.
hubert.reinterpretcast added a comment.

@abhina.sreeskantharajan, does this patch assume too much about the characters 
displayable for diagnostic output?


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

https://reviews.llvm.org/D155610

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

[PATCH] D156237: Complete the implementation of P2361 Unevaluated string literals

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/Parser/cxx0x-attributes.cpp:450-451
+namespace P2361 {
+[[deprecated(L"abc")]] void a(); // expected-error{{an unevaluated string 
literal cannot have an encoding prefix}}
+[[nodiscard("\123")]] int b(); // expected-error{{invalid escape sequence 
'\123' in an unevaluated string literal}}
+}

There should be corresponding C tests if, as the release notes text implies, 
the C behaviour changes (although, again, this direction has not been adopted 
by the C committee).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156237

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

[PATCH] D151567: [LLVM][Support] Report EISDIR when opening a directory on AIX

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

I believe the unit tests should be updated to ensure that we get `EISDIR` when 
opening directories for reading and for writing: 
`llvm/unittests/Support/Path.cpp`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151567

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

[PATCH] D151567: [LLVM][Support] Report EISDIR when opening a directory on AIX

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: llvm/lib/Support/Unix/Path.inc:1020
+struct stat Status;
+if (stat(P.begin(), ) == -1) 
+  return std::error_code(errno, std::generic_category());

Please try using `fstat` on the result of `open` to address the comments from 
https://reviews.llvm.org/D156798.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151567

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

[PATCH] D156596: [Clang] Produce a warning instead of an error in unevaluated strings before C++26

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D156596#4558097 , @cor3ntin wrote:

> Note: we are waiting for feedback from @hubert.reinterpretcast's people 
> before progressing this.

The "full" build hit some (unrelated) snags, but the initial results look good. 
The patch is awaiting C test cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156596

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

[PATCH] D156596: [Clang] Produce a warning instead of an error in unevaluated strings before C++26

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:290
+  "encoding prefix '%0' on an unevaluated string literal has no effect"
+  "%select{| and is incompatible with c++2c}1">,
+  InGroup>;

aaron.ballman wrote:
> hubert.reinterpretcast wrote:
> > I am not seeing any tests covering C mode.
> > 
> > @aaron.ballman, can you confirm that you are okay with the warning when 
> > processing C code (and, moreover, the newly-added errors for numeric escape 
> > sequences)?
> > 
> Good call on C test coverage, thank you for bringing that up!
> 
> I think the behavior in C is reasonable, even if it's not mandated by the 
> standard. We're going from issuing an error in C to issuing a warning and the 
> warning text helpfully omits the C++2c part of the diagnostic, so it seems 
> like we're more relaxed in C than we previously were.
> 
> Do you have concerns about the behavior in C? (Or are you saying you'd prefer 
> this to be an error in C as it was before?)
My question was with respect to the change from the Clang 16 status quo for 
numeric escapes introduced by the prior change and carried forward with this 
one: https://godbolt.org/z/aneGr1Yfb

```
_Static_assert(1, "\x30"); // error in Clang 17
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156596

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

[PATCH] D156596: [Clang] Produce a warning instead of an error in unevaluated strings before C++26

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:290
+  "encoding prefix '%0' on an unevaluated string literal has no effect"
+  "%select{| and is incompatible with c++2c}1">,
+  InGroup>;

I am not seeing any tests covering C mode.

@aaron.ballman, can you confirm that you are okay with the warning when 
processing C code (and, moreover, the newly-added errors for numeric escape 
sequences)?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156596

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

[PATCH] D156596: [Clang] Produce a warning instead of an error in unevaluated strings before C++26

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D156596#4546742 , @aaron.ballman 
wrote:

> If so, I'd like @hubert.reinterpretcast to mention if this works for his 
> needs.

We are putting together a build for the (known) affected groups. I will report 
back when we have results.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156596

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

[PATCH] D156596: [Clang] Produce a warning instead of an error in unevaluated strings before C++26

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/SemaCXX/static-assert.cpp:44-45
+static_assert(false, L"\x1ff"// expected-warning {{encoding prefix 'L' on 
an unevaluated string literal has no effect and is incompatible with c++2c}} \
+ // expected-error {{hex escape sequence out 
of range}} \
  // expected-error {{invalid escape sequence 
'\x1ff' in an unevaluated string literal}}
  "0\x123"// expected-error {{invalid escape sequence 
'\x123' in an unevaluated string literal}}

I doubt that it is a problem, but Clang 16 accepted such hex escapes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156596

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

[PATCH] D105759: Implement P2361 Unevaluated string literals

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D105759#4543246 , @aaron.ballman 
wrote:

> I'd recommend we change the diagnostic to be a warning that defaults to an 
> error so that users who are caught by the changes can still disable the 
> diagnostic rather than be stuck; for Clang 18, we can explore other solutions 
> to the issue. Would this work for you @hubert.reinterpretcast?

I think there are questions about whether an error (or even warning) by default 
is appropriate. This seems to be a change for C++2c that does not have "DR" 
treatment from the committee. Considering this a warning controlled by 
`c++2c-compat` is a potential direction. Indeed, if we are going to accept the 
code, we might as well allow it as an extension in C++2c modes. With this line 
of logic, I don't see why we would want user-side churn of making a migration 
effort.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105759

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

[PATCH] D105759: Implement P2361 Unevaluated string literals

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D105759#4541813 , @cor3ntin wrote:

> In D105759#4540716 , 
> @hubert.reinterpretcast wrote:
>
>>> I hope this patch may allow to gather some data on that.
>>
>> @cor3ntin, I have reports that applications having encoding prefixes in 
>> `static_assert` are failing to build. The committee did not adopt the 
>> subject paper as a "DR resolution". Is it possible to downgrade to a warning?
>
> You know how frequent it is?

No, but I am concerned that this came up even before we deployed an LLVM 
17-based solution (in pre-release testing). I believe that reverting for LLVM 
17 is the prudent course of action.

> Previously with a prefix, it was parsed as a wide (for example) string, and 
> we relied on the fact that L was UTF-16/32 to sometimes print a reasonable 
> diagnostics - and sometimes not https://godbolt.org/z/f3Pj4T5aj
> This is going to be worse when we add -fexec-charset:
>
> - `static_assert(true, L"やあ")` is going to be ill-formed when coded as, eg, 
> EBCDIC because it's not representable, and if it is representable we need to 
> either output mojibake or convert the string back to UTF-8 which we are 
> currently not doing.

This may be the motivation for the prefixes in the applications in the first 
place in the context of other compilers: They may have needed the prefix to 
avoid unrepresentable character issues (e.g., if the other compiler rejects the 
unprefixed string, but manages to emit the error to the terminal because both 
the terminal and the compiler use the source encoding).

> Another solution maybe to lexically ignore prefixes by replacing the string 
> literal token on the fly such that they are still parsed as unevaluated 
> strings and not encoded, i could look into that.

This sounds good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105759

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

[PATCH] D105759: Implement P2361 Unevaluated string literals

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

> I hope this patch may allow to gather some data on that.

@cor3ntin, I have reports that applications having encoding prefixes in 
`static_assert` are failing to build. The committee did not adopt the subject 
paper as a "DR resolution". Is it possible to downgrade to a warning?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105759

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

[PATCH] D155544: [AIX][TLS][clang] Add -maix-small-local-exec-tls clang option.

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast accepted this revision.
hubert.reinterpretcast added a comment.

LGTM for landing after D156203  and D155600 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155544

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

[PATCH] D155501: Add new option -fkeep-persistent-storage-variables to Clang release notes

[Hubert Tong via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGcead1497ae0c: Add new option 
-fkeep-persistent-storage-variables to Clang release notes (authored by 
qianzhen, committed by hubert.reinterpretcast).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155501

Files:
  clang/docs/ReleaseNotes.rst


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -274,6 +274,11 @@
 - ``-fcaret-diagnostics-max-lines=`` has been added as a driver options, which
   lets users control the maximum number of source lines printed for a
   caret diagnostic.
+- ``-fkeep-persistent-storage-variables`` has been implemented to keep all
+  variables that have a persistent storage duration—including global, static
+  and thread-local variables—to guarantee that they can be directly addressed.
+  Since this inhibits the merging of the affected variables, the number of
+  individual relocations in the program will generally increase.
 
 Deprecated Compiler Flags
 -


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -274,6 +274,11 @@
 - ``-fcaret-diagnostics-max-lines=`` has been added as a driver options, which
   lets users control the maximum number of source lines printed for a
   caret diagnostic.
+- ``-fkeep-persistent-storage-variables`` has been implemented to keep all
+  variables that have a persistent storage duration—including global, static
+  and thread-local variables—to guarantee that they can be directly addressed.
+  Since this inhibits the merging of the affected variables, the number of
+  individual relocations in the program will generally increase.
 
 Deprecated Compiler Flags
 -
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155544: [AIX][TLS] Add -maix-small-local-exec-tls option.

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D155544#4523857 , @amyk wrote:

> Wouldn't this patch need to land before the back-end patch, because I 
> introduce the option here, and then I use it in the backend patch?

Please move the `llvm/` changes into the back-end patch or land them separately 
first.

> In terms of checking for a 32-bit diagnostic within 
> `check-aix-small-local-exec-tls-opt.ll`, the RUN lines in that test currently 
> are 64-bit RUN lines. Perhaps in the backend patch, I should just add the 
> 32-bit lines to show the diagnostic?

This seems to be the test designed to cover the non-codegen aspects of the 
attribute. In that light, the 32-bit run line belongs here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155544

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

[PATCH] D155544: [AIX][TLS] Add -maix-small-local-exec-tls option.

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/include/clang/Driver/Options.td:4095
+  HelpText<"Produce a faster access sequence for local-exec TLS variables "
+   "where the offset from the thread pointer value is encoded as an "
+   "immediate operand (AIX 64-bit only). "

Same comment: use "TLS base".



Comment at: clang/test/Driver/aix-small-local-exec-tls.c:2
+// RUN: %clang -target powerpc64-unknown-aix -S -emit-llvm %s -o - | FileCheck 
%s
+// RUN: %clang -target powerpc-unknown-aix -S -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang -target powerpc64le-unknown-linux-gnu -S -emit-llvm %s -o - | 
FileCheck %s

It was agreed in offline discussion that 32-bit AIX should generate a message 
about the option because the general semantics are meaningful in that context 
but there is no implementation.



Comment at: llvm/test/CodeGen/PowerPC/check-aix-small-local-exec-tls-opt.ll:6
+; RUN:   FileCheck %s --check-prefix=CHECK-NOT-SUPPORTED
+
+define dso_local signext i32 @f() {

Same comment re: 32-bit AIX.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155544

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

[PATCH] D155544: [AIX][TLS] Add -maix-small-local-exec-tls option.

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

Patch should not land before back-end patch. I also suggest having the patch 
incorporate the new option into the Clang release notes before it lands.




Comment at: llvm/lib/Target/PowerPC/PPC.td:324
+   "Produce a faster access sequence for local-exec TLS "
+   "variables where the offset from the thread pointer value "
+   "is encoded as an immediate operand (AIX 64-bit only). "

As agreed offline:
thread pointer value -> TLS base




Comment at: llvm/lib/Target/PowerPC/PPCSubtarget.cpp:127
+
+  if ((!TargetTriple.isOSAIX()) && HasAIXSmallLocalExecTLS)
+report_fatal_error(

There should be no confusion over how tightly `!` binds over `&&`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155544

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

[PATCH] D155501: Add new option -fkeep-persistent-storage-variables to Clang release notes

[Hubert Tong via Phabricator via cfe-commits]

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

LGTM; thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155501

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

[PATCH] D153536: [Clang] Implement P2169 A nice placeholder with no name

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D153536#4512897 , @dblaikie wrote:

> but at least at a first blush I can't reproduce the failures shown...

@dblaikie, you //did// reproduce the issue with the members. Both entries have 
DW_AT_decl_line 2 and DW_AT_data_member_location 0 (the second entry should 
indicate DW_AT_decl_line 3 and DW_AT_data_member_location 4):

>   0x002e:   DW_TAG_structure_type
>   DW_AT_calling_convention(DW_CC_pass_by_value)
>   DW_AT_name  ("t1")
>   DW_AT_byte_size (0x08)
>   DW_AT_decl_file 
> ("/usr/local/google/home/blaikie/dev/scratch/unused_member.cpp")
>   DW_AT_decl_line (1)
>   
>   0x0034: DW_TAG_member
> DW_AT_name("_")
> DW_AT_type(0x0047 "int")
> DW_AT_decl_file   
> ("/usr/local/google/home/blaikie/dev/scratch/unused_member.cpp")
> DW_AT_decl_line   (2)
> DW_AT_data_member_location(0x00)
>   
>   0x003d: DW_TAG_member
> DW_AT_name("_")
> DW_AT_type(0x0047 "int")
> DW_AT_decl_file   
> ("/usr/local/google/home/blaikie/dev/scratch/unused_member.cpp")
> DW_AT_decl_line   (2)
> DW_AT_data_member_location(0x00)
>   
>   0x0046: NULL

As for the block-scope case, I am still able to reproduce the issue (and also 
your result that does not exhibit the issue). The key seems to be having the 
`_`s on the same line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153536

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

[PATCH] D139586: [Clang][C++23] Lifetime extension in range-based for loops

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D139586#4363725 , @cor3ntin wrote:

> @hubert.reinterpretcast I'll try to look at that but unless I'm mistaken,  
> the wording excludes function parameters
>
>> The fourth context is when a temporary object **other than a function 
>> parameter** object is created in the for-range-initializer of a range-based 
>> for statement. If such a temporary object would otherwise be destroyed at 
>> the end of the for-range-initializer full-expression, the object persists 
>> for the lifetime of the reference initialized by the for-range-initializer.
>
> I think that invalidates some of your examples?

@cor3ntin, the examples I gave were for temporary objects created for binding 
to a reference (that, in turn, happens to be a function parameter). Those 
temporary objects are not function parameters.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139586

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

[PATCH] D155501: Add new option -fkeep-persistent-storage-variables to Clang release notes

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:259-260
+- ``-fkeep-persistent-storage-variables`` has been implemented to keep all
+  variables that have a persistent storage duration, including global, static
+  and thread-local variables, to guarantee that they can be directly addressed.
 

Suggestion: Use em dashes to avoid overuse of commas.

Additionally, I am wondering if the release notes is a reasonable place to add 
something like: Since this inhibits the merging of the affected variables, the 
number of individual relocations in the program will generally increase.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155501

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

[PATCH] D150221: Add option -fkeep-persistent-storage-variables to emit all variables that have a persistent storage duration

[Hubert Tong via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGbb6ab91b1dcd: Add option -fkeep-persistent-storage-variables 
to emit all variables that have… (authored by qianzhen, committed by 
hubert.reinterpretcast).

Changed prior to commit:
  https://reviews.llvm.org/D150221?vs=540060=540732#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150221

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CodeGen/keep-persistent-storage-variables.cpp
  clang/test/Driver/fkeep-persistent-storage-variables.c

Index: clang/test/Driver/fkeep-persistent-storage-variables.c
===
--- /dev/null
+++ clang/test/Driver/fkeep-persistent-storage-variables.c
@@ -0,0 +1,5 @@
+// RUN: %clang -fkeep-persistent-storage-variables -c %s -### 2>&1 | FileCheck %s
+// RUN: %clang -fkeep-persistent-storage-variables -fno-keep-persistent-storage-variables -c %s -### 2>&1 | FileCheck %s -check-prefix=CHECK-NOKEEP
+
+// CHECK: "-fkeep-persistent-storage-variables"
+// CHECK-NOKEEP-NOT: "-fkeep-persistent-storage-variables"
Index: clang/test/CodeGen/keep-persistent-storage-variables.cpp
===
--- /dev/null
+++ clang/test/CodeGen/keep-persistent-storage-variables.cpp
@@ -0,0 +1,53 @@
+// RUN: %clang_cc1 -fkeep-persistent-storage-variables -emit-llvm %s -o - -triple=x86_64-unknown-linux-gnu | FileCheck %s
+// RUN: %clang_cc1 -fkeep-persistent-storage-variables -emit-llvm %s -o - -triple=powerpc64-ibm-aix-xcoff | FileCheck %s
+
+// CHECK: @_ZL2g1 = internal global i32 0, align 4
+// CHECK: @_ZL2g2 = internal global i32 1, align 4
+// CHECK: @tl1 = thread_local global i32 0, align 4
+// CHECK: @tl2 = thread_local global i32 3, align 4
+// CHECK: @_ZL3tl3 = internal thread_local global i32 0, align 4
+// CHECK: @_ZL3tl4 = internal thread_local global i32 4, align 4
+// CHECK: @g5 = global i32 0, align 4
+// CHECK: @g6 = global i32 6, align 4
+// CHECK: @_ZZ5test3vE2s3 = internal global i32 0, align 4
+// CHECK: @_ZN12_GLOBAL__N_12s4E = internal global i32 42, align 4
+// CHECK: @_ZZ5test5vE3tl5 = internal thread_local global i32 1, align 4
+// CHECK: @_ZN2ST2s6E = global i32 7, align 4
+// CHECK: @_Z2v7 = internal global %union.anon zeroinitializer, align 4
+// CHECK: @_ZDC2v8E = global %struct.ST8 zeroinitializer, align 4
+// CHECK: @llvm{{(\.compiler)?}}.used = appending global [14 x ptr] [ptr @_ZL2g1, ptr @_ZL2g2, ptr @tl1, ptr @tl2, ptr @_ZL3tl3, ptr @_ZL3tl4, ptr @g5, ptr @g6, ptr @_ZZ5test3vE2s3, ptr @_ZN12_GLOBAL__N_12s4E, ptr @_ZZ5test5vE3tl5, ptr @_ZN2ST2s6E, ptr @_Z2v7, ptr @_ZDC2v8E], section "llvm.metadata"
+
+static int g1;
+static int g2 = 1;
+__thread int tl1;
+__thread int tl2 = 3;
+static __thread int tl3;
+static __thread int tl4 = 4;
+int g5;
+int g6 = 6;
+
+int test3() {
+  static int s3 = 0;
+  ++s3;
+  return s3;
+}
+
+namespace {
+  int s4 = 42;
+}
+
+int test5() {
+  static __thread int tl5 = 1;
+  ++tl5;
+  return tl5;
+}
+
+struct ST {
+  static int s6;
+};
+int ST::s6 = 7;
+
+static union { int v7; };
+
+struct ST8 { int v8; };
+auto [v8] = ST8{0};
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -7404,6 +7404,8 @@
 
   Args.addOptInFlag(CmdArgs, options::OPT_fkeep_static_consts,
 options::OPT_fno_keep_static_consts);
+  Args.addOptInFlag(CmdArgs, options::OPT_fkeep_persistent_storage_variables,
+options::OPT_fno_keep_persistent_storage_variables);
   Args.addOptInFlag(CmdArgs, options::OPT_fcomplete_member_pointers,
 options::OPT_fno_complete_member_pointers);
   Args.addOptOutFlag(CmdArgs, options::OPT_fcxx_static_destructors,
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -2426,12 +2426,14 @@
   if (D && D->hasAttr())
 addUsedOrCompilerUsedGlobal(GV);
 
-  if (CodeGenOpts.KeepStaticConsts && D && isa(D)) {
-const auto *VD = cast(D);
-if (VD->getType().isConstQualified() &&
-VD->getStorageDuration() == SD_Static)
-  addUsedOrCompilerUsedGlobal(GV);
-  }
+  if (const auto *VD = dyn_cast_if_present(D);
+  VD &&
+  ((CodeGenOpts.KeepPersistentStorageVariables &&
+(VD->getStorageDuration() == SD_Static ||
+ VD->getStorageDuration() == SD_Thread)) ||
+   (CodeGenOpts.KeepStaticConsts && VD->getStorageDuration() == SD_Static &&
+VD->getType().isConstQualified(
+addUsedOrCompilerUsedGlobal(GV);
 }
 
 

[PATCH] D153536: [Clang] Implement P2169 A nice placeholder with no name

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:136-139
+- Implemented `P2169R4: A nice placeholder with no name 
`_. This allows using `_`
+  as a variable name multiple times in the same scope and is supported in all 
C++ language modes as an extension.
+  An extension warning is produced when multiple variables are introduced by 
`_` in the same scope.
+  Unused warnings are no longer produced for variables named `_`.

Use double backticks to start/end inline code in RST.



Comment at: clang/lib/Sema/SemaInit.cpp:2607
+  ValueDecl *VD =
+  SemaRef.tryLookupUnambiguousFieldDecl(RT->getDecl(), FieldName);
+  if (auto *FD = dyn_cast_if_present(VD)) {

Needs rebase on top of 632dd6a4ca00.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153536

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

[PATCH] D153701: [WIP][Clang] Implement P2718R0 "Lifetime extension in range-based for loops"

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/Sema/SemaExprCXX.cpp:8901-8914
+  // [P2718R0] Lifetime extension in range-based for loops.
+  //
+  // 6.7.7 [class.temporary] p5:
+  // There are four contexts in which temporaries are destroyed at a different
+  // point than the end of the full-expression.
+  //
+  // 6.7.7 [class.temporary] p6:

yronglin wrote:
> hubert.reinterpretcast wrote:
> > yronglin wrote:
> > > hubert.reinterpretcast wrote:
> > > > yronglin wrote:
> > > > > yronglin wrote:
> > > > > > hubert.reinterpretcast wrote:
> > > > > > > yronglin wrote:
> > > > > > > > rsmith wrote:
> > > > > > > > > This isn't the right way to model the behavior here -- the 
> > > > > > > > > presence or absence of an `ExprWithCleanups` is just a 
> > > > > > > > > convenience to tell consumers of the AST whether they should 
> > > > > > > > > expect to see cleanups later or not, and doesn't carry an 
> > > > > > > > > implication of affecting the actual temporary lifetimes and 
> > > > > > > > > storage durations.
> > > > > > > > > 
> > > > > > > > > The outcome that we should be aiming to reach is that all 
> > > > > > > > > `MaterializeTemporaryExpr`s created as part of processing the 
> > > > > > > > > for-range-initializer are marked as being lifetime-extended 
> > > > > > > > > by the for-range variable. Probably the simplest way to 
> > > > > > > > > handle that would be to track the current enclosing 
> > > > > > > > > for-range-initializer variable in the 
> > > > > > > > > `ExpressionEvaluationContextRecord`, and whenever a 
> > > > > > > > > `MaterializeTemporaryExpr` is created, if there is a current 
> > > > > > > > > enclosing for-range-initializer, mark that 
> > > > > > > > > `MaterializeTemporaryExpr` as being lifetime-extended by it.
> > > > > > > > Awesome! Thanks a lot for your advice, this is very helpful! I 
> > > > > > > > want to take a longer look at it.
> > > > > > > As mentioned in D139586, `CXXDefaultArgExpr`s may need additional 
> > > > > > > handling. Similarly for `CXXDefaultInitExpr`s.
> > > > > > Thanks for your tips! I have a question that what's the correct way 
> > > > > > to extent the lifetime of `CXXBindTemporaryExpr`? Can I just 
> > > > > > `materialize` the temporary? It may replaced by 
> > > > > > `MaterializeTemporaryExpr`, and then I can mark it as being 
> > > > > > lifetime-extended by the for-range variable.
> > > > > Eg.
> > > > > ```
> > > > > void f() {
> > > > >   int v[] = {42, 17, 13};
> > > > >   Mutex m;
> > > > >   for (int x : static_cast(LockGuard(m)), v) // lock released 
> > > > > in C++ 2020
> > > > >   {
> > > > > LockGuard guard(m); // OK in C++ 2020, now deadlocks
> > > > >   }
> > > > > }
> > > > > ```
> > > > > ```
> > > > > BinaryOperator 0x135036220 'int[3]' lvalue ','
> > > > > |-CXXStaticCastExpr 0x1350361d0 'void' static_cast 
> > > > > | `-CXXFunctionalCastExpr 0x135036198 'LockGuard':'struct LockGuard' 
> > > > > functional cast to LockGuard 
> > > > > |   `-CXXBindTemporaryExpr 0x135036178 'LockGuard':'struct LockGuard' 
> > > > > (CXXTemporary 0x135036178)
> > > > > | `-CXXConstructExpr 0x135036140 'LockGuard':'struct LockGuard' 
> > > > > 'void (Mutex &)'
> > > > > |   `-DeclRefExpr 0x135035e18 'Mutex':'class Mutex' lvalue Var 
> > > > > 0x135035b40 'm' 'Mutex':'class Mutex'
> > > > > `-DeclRefExpr 0x135036200 'int[3]' lvalue Var 0x135035928 'v' 'int[3]'
> > > > > ```
> > > > If `MaterializeTemporaryExpr` represents a "temporary materialization 
> > > > conversion", then the above should already have one just under the 
> > > > `static_cast` to `void` (since the cast operand would be a 
> > > > discarded-value expression).
> > > > 
> > > > There may be unfortunate effects from materializing temporaries for 
> > > > discarded-value expressions though: Technically, temporaries are also 
> > > > created for objects having scalar type.
> > > > 
> > > > Currently, `MaterializeTemporaryExpr` is documented as being tied to 
> > > > reference binding, but that is not correct: for example, 
> > > > `MaterializeTemporaryExpr` also appears when a member access is made on 
> > > > a temporary of class type.
> > > http://eel.is/c++draft/class.temporary says:
> > > ```
> > > [Note 3: Temporary objects are materialized:
> > > ...
> > > (2.6)
> > > when a prvalue that has type other than cv void appears as a 
> > > discarded-value expression ([expr.context]).
> > > — end note]
> > > ```
> > > Seems we should materialized the discard-value expression in this case, 
> > > WDYT?
> > I think we should, but what is the codegen fallout? Would no-opt builds 
> > start writing `42` into allocated memory for `static_cast(42)`?
> Thanks for your confirm @hubert.reinterpretcast ! 
> 
> I have tried locally, the generated  IR of `void f()` is:
> ```
> define void @f()() {
> entry:
>   %v = alloca [3 x i32], align 4
>   %m = alloca %class.Mutex, align 8
>   %__range1 = alloca ptr, align 

[PATCH] D150221: Add option -fkeep-persistent-storage-variables to emit all variables that have a persistent storage duration

[Hubert Tong via Phabricator via cfe-commits]

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

> Furthermore, there is some discussion over covering more than just variables, 
> but also artifacts with reasonable mangled names such as the symbols for 
> temporaries bound to references with "persistent storage" and guard variables.

We can follow up with a separate patch to add the extra functionality. This 
patch LGTM for the scope it covers.




Comment at: clang/include/clang/Driver/Options.td:1703
+  PosFlag, NegFlag,
+  BothFlags<[NoXarchOption], " keeping all variables that have a persistent 
storage duration, including global, static and thread local variables, to 
guarantee that they can be directly addressed">>;
 defm fixed_point : BoolFOption<"fixed-point",

Minor nit: Use hyphen in "thread-local".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150221

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

[PATCH] D153701: [WIP][Clang] Implement P2718R0 "Lifetime extension in range-based for loops"

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/Sema/SemaExprCXX.cpp:8901-8914
+  // [P2718R0] Lifetime extension in range-based for loops.
+  //
+  // 6.7.7 [class.temporary] p5:
+  // There are four contexts in which temporaries are destroyed at a different
+  // point than the end of the full-expression.
+  //
+  // 6.7.7 [class.temporary] p6:

yronglin wrote:
> hubert.reinterpretcast wrote:
> > yronglin wrote:
> > > yronglin wrote:
> > > > hubert.reinterpretcast wrote:
> > > > > yronglin wrote:
> > > > > > rsmith wrote:
> > > > > > > This isn't the right way to model the behavior here -- the 
> > > > > > > presence or absence of an `ExprWithCleanups` is just a 
> > > > > > > convenience to tell consumers of the AST whether they should 
> > > > > > > expect to see cleanups later or not, and doesn't carry an 
> > > > > > > implication of affecting the actual temporary lifetimes and 
> > > > > > > storage durations.
> > > > > > > 
> > > > > > > The outcome that we should be aiming to reach is that all 
> > > > > > > `MaterializeTemporaryExpr`s created as part of processing the 
> > > > > > > for-range-initializer are marked as being lifetime-extended by 
> > > > > > > the for-range variable. Probably the simplest way to handle that 
> > > > > > > would be to track the current enclosing for-range-initializer 
> > > > > > > variable in the `ExpressionEvaluationContextRecord`, and whenever 
> > > > > > > a `MaterializeTemporaryExpr` is created, if there is a current 
> > > > > > > enclosing for-range-initializer, mark that 
> > > > > > > `MaterializeTemporaryExpr` as being lifetime-extended by it.
> > > > > > Awesome! Thanks a lot for your advice, this is very helpful! I want 
> > > > > > to take a longer look at it.
> > > > > As mentioned in D139586, `CXXDefaultArgExpr`s may need additional 
> > > > > handling. Similarly for `CXXDefaultInitExpr`s.
> > > > Thanks for your tips! I have a question that what's the correct way to 
> > > > extent the lifetime of `CXXBindTemporaryExpr`? Can I just `materialize` 
> > > > the temporary? It may replaced by `MaterializeTemporaryExpr`, and then 
> > > > I can mark it as being lifetime-extended by the for-range variable.
> > > Eg.
> > > ```
> > > void f() {
> > >   int v[] = {42, 17, 13};
> > >   Mutex m;
> > >   for (int x : static_cast(LockGuard(m)), v) // lock released in 
> > > C++ 2020
> > >   {
> > > LockGuard guard(m); // OK in C++ 2020, now deadlocks
> > >   }
> > > }
> > > ```
> > > ```
> > > BinaryOperator 0x135036220 'int[3]' lvalue ','
> > > |-CXXStaticCastExpr 0x1350361d0 'void' static_cast 
> > > | `-CXXFunctionalCastExpr 0x135036198 'LockGuard':'struct LockGuard' 
> > > functional cast to LockGuard 
> > > |   `-CXXBindTemporaryExpr 0x135036178 'LockGuard':'struct LockGuard' 
> > > (CXXTemporary 0x135036178)
> > > | `-CXXConstructExpr 0x135036140 'LockGuard':'struct LockGuard' 'void 
> > > (Mutex &)'
> > > |   `-DeclRefExpr 0x135035e18 'Mutex':'class Mutex' lvalue Var 
> > > 0x135035b40 'm' 'Mutex':'class Mutex'
> > > `-DeclRefExpr 0x135036200 'int[3]' lvalue Var 0x135035928 'v' 'int[3]'
> > > ```
> > If `MaterializeTemporaryExpr` represents a "temporary materialization 
> > conversion", then the above should already have one just under the 
> > `static_cast` to `void` (since the cast operand would be a discarded-value 
> > expression).
> > 
> > There may be unfortunate effects from materializing temporaries for 
> > discarded-value expressions though: Technically, temporaries are also 
> > created for objects having scalar type.
> > 
> > Currently, `MaterializeTemporaryExpr` is documented as being tied to 
> > reference binding, but that is not correct: for example, 
> > `MaterializeTemporaryExpr` also appears when a member access is made on a 
> > temporary of class type.
> http://eel.is/c++draft/class.temporary says:
> ```
> [Note 3: Temporary objects are materialized:
> ...
> (2.6)
> when a prvalue that has type other than cv void appears as a discarded-value 
> expression ([expr.context]).
> — end note]
> ```
> Seems we should materialized the discard-value expression in this case, WDYT?
I think we should, but what is the codegen fallout? Would no-opt builds start 
writing `42` into allocated memory for `static_cast(42)`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153701

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

[PATCH] D153701: [WIP][Clang] Implement P2718R0 "Lifetime extension in range-based for loops"

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/Sema/SemaExprCXX.cpp:8901-8914
+  // [P2718R0] Lifetime extension in range-based for loops.
+  //
+  // 6.7.7 [class.temporary] p5:
+  // There are four contexts in which temporaries are destroyed at a different
+  // point than the end of the full-expression.
+  //
+  // 6.7.7 [class.temporary] p6:

yronglin wrote:
> yronglin wrote:
> > hubert.reinterpretcast wrote:
> > > yronglin wrote:
> > > > rsmith wrote:
> > > > > This isn't the right way to model the behavior here -- the presence 
> > > > > or absence of an `ExprWithCleanups` is just a convenience to tell 
> > > > > consumers of the AST whether they should expect to see cleanups later 
> > > > > or not, and doesn't carry an implication of affecting the actual 
> > > > > temporary lifetimes and storage durations.
> > > > > 
> > > > > The outcome that we should be aiming to reach is that all 
> > > > > `MaterializeTemporaryExpr`s created as part of processing the 
> > > > > for-range-initializer are marked as being lifetime-extended by the 
> > > > > for-range variable. Probably the simplest way to handle that would be 
> > > > > to track the current enclosing for-range-initializer variable in the 
> > > > > `ExpressionEvaluationContextRecord`, and whenever a 
> > > > > `MaterializeTemporaryExpr` is created, if there is a current 
> > > > > enclosing for-range-initializer, mark that `MaterializeTemporaryExpr` 
> > > > > as being lifetime-extended by it.
> > > > Awesome! Thanks a lot for your advice, this is very helpful! I want to 
> > > > take a longer look at it.
> > > As mentioned in D139586, `CXXDefaultArgExpr`s may need additional 
> > > handling. Similarly for `CXXDefaultInitExpr`s.
> > Thanks for your tips! I have a question that what's the correct way to 
> > extent the lifetime of `CXXBindTemporaryExpr`? Can I just `materialize` the 
> > temporary? It may replaced by `MaterializeTemporaryExpr`, and then I can 
> > mark it as being lifetime-extended by the for-range variable.
> Eg.
> ```
> void f() {
>   int v[] = {42, 17, 13};
>   Mutex m;
>   for (int x : static_cast(LockGuard(m)), v) // lock released in C++ 
> 2020
>   {
> LockGuard guard(m); // OK in C++ 2020, now deadlocks
>   }
> }
> ```
> ```
> BinaryOperator 0x135036220 'int[3]' lvalue ','
> |-CXXStaticCastExpr 0x1350361d0 'void' static_cast 
> | `-CXXFunctionalCastExpr 0x135036198 'LockGuard':'struct LockGuard' 
> functional cast to LockGuard 
> |   `-CXXBindTemporaryExpr 0x135036178 'LockGuard':'struct LockGuard' 
> (CXXTemporary 0x135036178)
> | `-CXXConstructExpr 0x135036140 'LockGuard':'struct LockGuard' 'void 
> (Mutex &)'
> |   `-DeclRefExpr 0x135035e18 'Mutex':'class Mutex' lvalue Var 
> 0x135035b40 'm' 'Mutex':'class Mutex'
> `-DeclRefExpr 0x135036200 'int[3]' lvalue Var 0x135035928 'v' 'int[3]'
> ```
If `MaterializeTemporaryExpr` represents a "temporary materialization 
conversion", then the above should already have one just under the 
`static_cast` to `void` (since the cast operand would be a discarded-value 
expression).

There may be unfortunate effects from materializing temporaries for 
discarded-value expressions though: Technically, temporaries are also created 
for objects having scalar type.

Currently, `MaterializeTemporaryExpr` is documented as being tied to reference 
binding, but that is not correct: for example, `MaterializeTemporaryExpr` also 
appears when a member access is made on a temporary of class type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153701

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

[PATCH] D153701: [WIP][Clang] Implement P2718R0 "Lifetime extension in range-based for loops"

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/Sema/SemaExprCXX.cpp:8901-8914
+  // [P2718R0] Lifetime extension in range-based for loops.
+  //
+  // 6.7.7 [class.temporary] p5:
+  // There are four contexts in which temporaries are destroyed at a different
+  // point than the end of the full-expression.
+  //
+  // 6.7.7 [class.temporary] p6:

yronglin wrote:
> rsmith wrote:
> > This isn't the right way to model the behavior here -- the presence or 
> > absence of an `ExprWithCleanups` is just a convenience to tell consumers of 
> > the AST whether they should expect to see cleanups later or not, and 
> > doesn't carry an implication of affecting the actual temporary lifetimes 
> > and storage durations.
> > 
> > The outcome that we should be aiming to reach is that all 
> > `MaterializeTemporaryExpr`s created as part of processing the 
> > for-range-initializer are marked as being lifetime-extended by the 
> > for-range variable. Probably the simplest way to handle that would be to 
> > track the current enclosing for-range-initializer variable in the 
> > `ExpressionEvaluationContextRecord`, and whenever a 
> > `MaterializeTemporaryExpr` is created, if there is a current enclosing 
> > for-range-initializer, mark that `MaterializeTemporaryExpr` as being 
> > lifetime-extended by it.
> Awesome! Thanks a lot for your advice, this is very helpful! I want to take a 
> longer look at it.
As mentioned in D139586, `CXXDefaultArgExpr`s may need additional handling. 
Similarly for `CXXDefaultInitExpr`s.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153701

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

[PATCH] D153536: [Clang] Implement P2169 A nice placeholder with no name

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D153536#4483191 , 
@hubert.reinterpretcast wrote:

> Similarly, only one local variable out of two in the same line reported:

I can confirm that adding a lexical block scope causes both variables to be 
recorded into the debug info (thus this is a debug info generation issue unique 
to the new feature).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153536

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

[PATCH] D153536: [Clang] Implement P2169 A nice placeholder with no name

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D153536#4483182 , 
@hubert.reinterpretcast wrote:

> If `llvm-dwarfdump` is to be believed, it looks like a compiler bug (two 
> members reported at the same offset).

Similarly, only one local variable out of two in the same line reported:

  $ cat placeholderDbg2.cc
  extern "C" int printf(const char *, ...);
  struct A {
A(int x) : x(x) { }
~A() { printf("%d\n", x); }
int x;
  };
  int main() {
A _ = 0, _ = 1;
return 0;
  }
  Return:  0x00:0   Sat Jul  8 23:52:46 2023 EDT



  (lldb) target create "a.out"
  Current executable set to '/terrannew/hstong/.Lpcoral03/llvmbld/a.out' 
(powerpc64le).
  (lldb) b 9
  Breakpoint 1: where = a.out`main + 68 at placeholderDbg2.cc:9:3, address = 
0x00010ab4
  (lldb) r
  Process 2951717 launched: '/terrannew/hstong/.Lpcoral03/llvmbld/a.out' 
(powerpc64le)
  Process 2951717 stopped
  * thread #1, name = 'a.out', stop reason = breakpoint 1.1
  frame #0: 0x000100010ab4 a.out`main at placeholderDbg2.cc:9:3
 6};
 7int main() {
 8  A _ = 0, _ = 1;
  -> 9  return 0;
 10   }
  (lldb) var
  (A) _ = (x = 0)
  (lldb) c
  Process 2951717 resuming
  1
  0
  Process 2951717 exited with status = 0 (0x)
  (lldb) q



  0x009b:   DW_TAG_subprogram [14] * (0x000b)
  DW_AT_low_pc [DW_FORM_addr] (0x00010a70)
  DW_AT_high_pc [DW_FORM_data4]   (0x00b4)
  DW_AT_frame_base [DW_FORM_exprloc]  (DW_OP_reg31 X31)
  DW_AT_name [DW_FORM_strp]   ( .debug_str[0x0086] = 
"main")
  DW_AT_decl_file [DW_FORM_data1] 
("/terrannew/hstong/.Lpcoral03/llvmbld/placeholderDbg2.cc")
  DW_AT_decl_line [DW_FORM_data1] (7)
  DW_AT_type [DW_FORM_ref4]   (cu + 0x008f => {0x008f} 
"int")
  DW_AT_external [DW_FORM_flag_present]   (true)
  
  0x00b4: DW_TAG_variable [15]   (0x009b)
DW_AT_location [DW_FORM_exprloc]  (DW_OP_fbreg +128)
DW_AT_name [DW_FORM_strp] ( .debug_str[0x008b] = 
"_")
DW_AT_decl_file [DW_FORM_data1]   
("/terrannew/hstong/.Lpcoral03/llvmbld/placeholderDbg2.cc")
DW_AT_decl_line [DW_FORM_data1]   (8)
DW_AT_type [DW_FORM_ref4] (cu + 0x005a => {0x005a} 
"A")
  
  0x00c3: NULL


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153536

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

[PATCH] D153536: [Clang] Implement P2169 A nice placeholder with no name

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/SemaCXX/cxx2c-placeholder-vars.cpp:46
+int arr[2] = {0, 1};
+static auto [_, _] = arr; // expected-error {{redefinition of '_'}} \
+// expected-note  {{previous definition is here}}

Just noting: This case is allowed by the wording, and I suspect it requires 
extended discussion.




Comment at: clang/test/SemaCXX/cxx2c-placeholder-vars.cpp:2
+// RUN: %clang -cc1 -fsyntax-only -verify -std=c++2c -Wunused-parameter 
-Wunused %s
+
+void static_var() {

hubert.reinterpretcast wrote:
> hubert.reinterpretcast wrote:
> > cor3ntin wrote:
> > > cor3ntin wrote:
> > > > cor3ntin wrote:
> > > > > hubert.reinterpretcast wrote:
> > > > > > hubert.reinterpretcast wrote:
> > > > > > > hubert.reinterpretcast wrote:
> > > > > > > > hubert.reinterpretcast wrote:
> > > > > > > > > Can we have tests for:
> > > > > > > > > ```
> > > > > > > > > struct { int _, _; } a = { ._ = 0 };
> > > > > > > > > ```
> > > > > > > > > 
> > > > > > > > > and
> > > > > > > > > 
> > > > > > > > > ```
> > > > > > > > > struct A {
> > > > > > > > >   A();
> > > > > > > > >   int _, _;
> > > > > > > > > };
> > > > > > > > > 
> > > > > > > > > A::A() : _(0) {}
> > > > > > > > > ```
> > > > > > > > Codegen test for
> > > > > > > > ```
> > > > > > > > static union { int _ = 42; };
> > > > > > > > int  = _;
> > > > > > > > int foo() { return 13; }
> > > > > > > > static union { int _ = foo(); };
> > > > > > > > int main(void) { return ref; }
> > > > > > > > ```
> > > > > > > > might be interesting.
> > > > > > > > 
> > > > > > > > I suspect that this case was missed in the committee discussion 
> > > > > > > > of the paper @cor3ntin.
> > > > > > > Less controversial tests to consider:
> > > > > > > ```
> > > > > > > struct A {
> > > > > > >   int _;
> > > > > > >   union { int _; };
> > > > > > > };
> > > > > > > struct B { union { int _, _; }; };
> > > > > > > ```
> > > > > > > 
> > > > > > In a similar vein, a codegen test for:
> > > > > > ```
> > > > > > struct A { A(); };
> > > > > > inline void f [[gnu::used]]() {
> > > > > >   static union { A _{}; };
> > > > > >   static union { A _{}; };
> > > > > > }
> > > > > > ```
> > > > > > 
> > > > > > Perhaps not intended to be allowed though (premise was no symbols 
> > > > > > with "linkage"?)
> > > > > What's interesting about 
> > > > > 
> > > > > ```
> > > > > static union { int _ = 42; };
> > > > > int  = _;
> > > > > int foo() { return 13; }
> > > > > static union { int _ = foo(); };
> > > > > int main(void) { return ref; }
> > > > > ```
> > > > > ?
> > > > > It's already supported by clang https://godbolt.org/z/6j89EdnEo
> > > > > 
> > > > > 
> > > > > I'm adding the other tests (and fixing the associated bugs, of which 
> > > > > there were a few...)
> > > > > 
> > > > > Perhaps not intended to be allowed though (premise was no symbols 
> > > > > with "linkage"?)
> > > > 
> > > > 
> > > > Yes, this should be ill-formed, anything where we would have to mangle  
> > > > multiple `_` should be ill-formed.
> > > > I do believe that's covered though, `_` does not have storage duration.
> > > > What's interesting about 
> > > > 
> > > > ```
> > > > static union { int _ = 42; };
> > > > int  = _;
> > > > int foo() { return 13; }
> > > > static union { int _ = foo(); };
> > > > int main(void) { return ref; }
> > > > ```
> > > > ?
> > > > It's already supported by clang https://godbolt.org/z/6j89EdnEo
> > > > 
> > > > 
> > > > I'm adding the other tests (and fixing the associated bugs, of which 
> > > > there were a few...)
> > > > 
> > > 
> > > I see it now. Thanks, I hate it. There is apparently a preexisting bug.
> > > And yes, i think we should say something about members of anonymous union 
> > > declared at namespace scope in the standard I realize now this is 
> > > missing
> > > Thanks for catching that.
> > > Thanks for catching that.
> > 
> > Glad to be of help!
> > I do believe that's covered though, `_` does not have storage duration.
> 
> It's not covered by the wording: `_` //is// a non-static data member.
> 
> 
@cor3ntin, it seems the designated initializer case has not been added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153536

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

[PATCH] D153536: [Clang] Implement P2169 A nice placeholder with no name

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D153536#4479275 , 
@hubert.reinterpretcast wrote:

> It seems the class member case trips up debuggers.

If `llvm-dwarfdump` is to be believed, it looks like a compiler bug (two 
members reported at the same offset).

  0x00a3:   DW_TAG_member [4]   (0x009d)
  DW_AT_name [DW_FORM_string] ("_")
  DW_AT_type [DW_FORM_ref4]   (cu + 0x00de => {0x00de} 
"int")
  DW_AT_decl_file [DW_FORM_data1] 
("/terrannew/hstong/.Lpcoral03/llvmbld/placeholderDbg.cc")
  DW_AT_decl_line [DW_FORM_data1] (2)
  DW_AT_data_member_location [DW_FORM_data1]  (0x00)
  
  0x00ad:   DW_TAG_member [4]   (0x009d)
  DW_AT_name [DW_FORM_string] ("_")
  DW_AT_type [DW_FORM_ref4]   (cu + 0x00de => {0x00de} 
"int")
  DW_AT_decl_file [DW_FORM_data1] 
("/terrannew/hstong/.Lpcoral03/llvmbld/placeholderDbg.cc")
  DW_AT_decl_line [DW_FORM_data1] (2)
  DW_AT_data_member_location [DW_FORM_data1]  (0x00)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153536

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

[PATCH] D153536: [Clang] Implement P2169 A nice placeholder with no name

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D153536#4474066 , @cor3ntin wrote:

> Seems to work well enough @hubert.reinterpretcast

It seems the class member case trips up debuggers.

  union U {
struct A { int _, _; } a;
struct B { int x, y; } b;
  };
  U u = {{1, 2}};
  
  int main(void) { return u.b.x; }



  (lldb) b main
  Breakpoint 1: where = a.out`main + 28 at placeholderDbg.cc:7:18, address = 
0x0001080c
  (lldb) r
  Process 2339034 launched: '/terrannew/hstong/.Lpcoral03/llvmbld/a.out' 
(powerpc64le)
  Process 2339034 stopped
  * thread #1, name = 'a.out', stop reason = breakpoint 1.1
  frame #0: 0x00010001080c a.out`main at placeholderDbg.cc:7:18
 4};
 5U u = {{1, 2}};
 6
  -> 7int main(void) { return u.b.x; }
  (lldb) print u
  (U) {
a = (_ = 1)
b = (x = 1, y = 2)
  }
  (lldb)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153536

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

[PATCH] D150221: Add option -fkeep-persistent-storage-variables to emit all variables that have a persistent storage duration

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

Update (looping back from offline discussion): The LTO use case is covered. 
There was some confusion over which meaning of "static" was meant by 
`-fkeep-static-consts`. The "static" meant was storage duration.

Furthermore, there is some discussion over covering more than just variables, 
but also artifacts with reasonable mangled names such as the symbols for 
temporaries bound to references with "persistent storage" and guard variables.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150221

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

[PATCH] D153600: Implement -frecord-command-line for XCOFF

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:2974
+RSOS << "@(#)" << MDS->getString();
+RSOS.write('\0');
+  }

hubert.reinterpretcast wrote:
> stephenpeckham wrote:
> > I would use a newline here.  The AIX **what **command looks for @(#) and 
> > echos subsequent bytes until it sees a double quote, a backslash, a > 
> > symbol, newline, or null byte.  The @(#) is not echoed, nor is the 
> > terminating character.  The **what **command prints a newline after it 
> > finds a terminating character.  This means that if the command line 
> > contains any of the special characters, the line will be truncated.
> > 
> > Exception:  If the @(#) is followed by "opt " or " opt ", the terminating 
> > characters are only a newline or null byte. This allows any of the other 
> > special characters to be part of the command line. It doesn't really matter 
> > if you use a newline or a null byte, but the legacy XL compiler uses a 
> > newline. The "opt" keyword should appear if the command line can contain a 
> > double quote, a > or a backslash.
> > 
> > The legacy compiler also uses other keywords besides "opt", including 
> > "version" and "cfg".  The **what** command doesn't do anything special with 
> > these keywords.
> As mentioned offline, newline on its own has potential of ambiguity because 
> it can appear in command line options (null bytes cannot). If there is a 
> preference for newline to be present, then having a null byte after could 
> help.
> 
> Note that `@(#)opt ` can appear on the command line too. Using `what` will 
> have limitations (but we should leave the possibility open for other 
> tools/methods to work).
> 
Thanks; I confirm that my comment has been addressed. I have no further 
comments at this time. Feel free to commit if another reviewer approves the 
patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153600

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

[PATCH] D153536: [Clang] Implement P2169 A nice placeholder with no name

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/SemaCXX/cxx2c-placeholder-vars.cpp:2
+// RUN: %clang -cc1 -fsyntax-only -verify -std=c++2c -Wunused-parameter 
-Wunused %s
+
+void static_var() {

hubert.reinterpretcast wrote:
> cor3ntin wrote:
> > cor3ntin wrote:
> > > cor3ntin wrote:
> > > > hubert.reinterpretcast wrote:
> > > > > hubert.reinterpretcast wrote:
> > > > > > hubert.reinterpretcast wrote:
> > > > > > > hubert.reinterpretcast wrote:
> > > > > > > > Can we have tests for:
> > > > > > > > ```
> > > > > > > > struct { int _, _; } a = { ._ = 0 };
> > > > > > > > ```
> > > > > > > > 
> > > > > > > > and
> > > > > > > > 
> > > > > > > > ```
> > > > > > > > struct A {
> > > > > > > >   A();
> > > > > > > >   int _, _;
> > > > > > > > };
> > > > > > > > 
> > > > > > > > A::A() : _(0) {}
> > > > > > > > ```
> > > > > > > Codegen test for
> > > > > > > ```
> > > > > > > static union { int _ = 42; };
> > > > > > > int  = _;
> > > > > > > int foo() { return 13; }
> > > > > > > static union { int _ = foo(); };
> > > > > > > int main(void) { return ref; }
> > > > > > > ```
> > > > > > > might be interesting.
> > > > > > > 
> > > > > > > I suspect that this case was missed in the committee discussion 
> > > > > > > of the paper @cor3ntin.
> > > > > > Less controversial tests to consider:
> > > > > > ```
> > > > > > struct A {
> > > > > >   int _;
> > > > > >   union { int _; };
> > > > > > };
> > > > > > struct B { union { int _, _; }; };
> > > > > > ```
> > > > > > 
> > > > > In a similar vein, a codegen test for:
> > > > > ```
> > > > > struct A { A(); };
> > > > > inline void f [[gnu::used]]() {
> > > > >   static union { A _{}; };
> > > > >   static union { A _{}; };
> > > > > }
> > > > > ```
> > > > > 
> > > > > Perhaps not intended to be allowed though (premise was no symbols 
> > > > > with "linkage"?)
> > > > What's interesting about 
> > > > 
> > > > ```
> > > > static union { int _ = 42; };
> > > > int  = _;
> > > > int foo() { return 13; }
> > > > static union { int _ = foo(); };
> > > > int main(void) { return ref; }
> > > > ```
> > > > ?
> > > > It's already supported by clang https://godbolt.org/z/6j89EdnEo
> > > > 
> > > > 
> > > > I'm adding the other tests (and fixing the associated bugs, of which 
> > > > there were a few...)
> > > > 
> > > > Perhaps not intended to be allowed though (premise was no symbols with 
> > > > "linkage"?)
> > > 
> > > 
> > > Yes, this should be ill-formed, anything where we would have to mangle  
> > > multiple `_` should be ill-formed.
> > > I do believe that's covered though, `_` does not have storage duration.
> > > What's interesting about 
> > > 
> > > ```
> > > static union { int _ = 42; };
> > > int  = _;
> > > int foo() { return 13; }
> > > static union { int _ = foo(); };
> > > int main(void) { return ref; }
> > > ```
> > > ?
> > > It's already supported by clang https://godbolt.org/z/6j89EdnEo
> > > 
> > > 
> > > I'm adding the other tests (and fixing the associated bugs, of which 
> > > there were a few...)
> > > 
> > 
> > I see it now. Thanks, I hate it. There is apparently a preexisting bug.
> > And yes, i think we should say something about members of anonymous union 
> > declared at namespace scope in the standard I realize now this is 
> > missing
> > Thanks for catching that.
> > Thanks for catching that.
> 
> Glad to be of help!
> I do believe that's covered though, `_` does not have storage duration.

It's not covered by the wording: `_` //is// a non-static data member.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153536

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

[PATCH] D153536: [Clang] Implement P2169 A nice placeholder with no name

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/SemaCXX/cxx2c-placeholder-vars.cpp:2
+// RUN: %clang -cc1 -fsyntax-only -verify -std=c++2c -Wunused-parameter 
-Wunused %s
+
+void static_var() {

cor3ntin wrote:
> cor3ntin wrote:
> > cor3ntin wrote:
> > > hubert.reinterpretcast wrote:
> > > > hubert.reinterpretcast wrote:
> > > > > hubert.reinterpretcast wrote:
> > > > > > hubert.reinterpretcast wrote:
> > > > > > > Can we have tests for:
> > > > > > > ```
> > > > > > > struct { int _, _; } a = { ._ = 0 };
> > > > > > > ```
> > > > > > > 
> > > > > > > and
> > > > > > > 
> > > > > > > ```
> > > > > > > struct A {
> > > > > > >   A();
> > > > > > >   int _, _;
> > > > > > > };
> > > > > > > 
> > > > > > > A::A() : _(0) {}
> > > > > > > ```
> > > > > > Codegen test for
> > > > > > ```
> > > > > > static union { int _ = 42; };
> > > > > > int  = _;
> > > > > > int foo() { return 13; }
> > > > > > static union { int _ = foo(); };
> > > > > > int main(void) { return ref; }
> > > > > > ```
> > > > > > might be interesting.
> > > > > > 
> > > > > > I suspect that this case was missed in the committee discussion of 
> > > > > > the paper @cor3ntin.
> > > > > Less controversial tests to consider:
> > > > > ```
> > > > > struct A {
> > > > >   int _;
> > > > >   union { int _; };
> > > > > };
> > > > > struct B { union { int _, _; }; };
> > > > > ```
> > > > > 
> > > > In a similar vein, a codegen test for:
> > > > ```
> > > > struct A { A(); };
> > > > inline void f [[gnu::used]]() {
> > > >   static union { A _{}; };
> > > >   static union { A _{}; };
> > > > }
> > > > ```
> > > > 
> > > > Perhaps not intended to be allowed though (premise was no symbols with 
> > > > "linkage"?)
> > > What's interesting about 
> > > 
> > > ```
> > > static union { int _ = 42; };
> > > int  = _;
> > > int foo() { return 13; }
> > > static union { int _ = foo(); };
> > > int main(void) { return ref; }
> > > ```
> > > ?
> > > It's already supported by clang https://godbolt.org/z/6j89EdnEo
> > > 
> > > 
> > > I'm adding the other tests (and fixing the associated bugs, of which 
> > > there were a few...)
> > > 
> > > Perhaps not intended to be allowed though (premise was no symbols with 
> > > "linkage"?)
> > 
> > 
> > Yes, this should be ill-formed, anything where we would have to mangle  
> > multiple `_` should be ill-formed.
> > I do believe that's covered though, `_` does not have storage duration.
> > What's interesting about 
> > 
> > ```
> > static union { int _ = 42; };
> > int  = _;
> > int foo() { return 13; }
> > static union { int _ = foo(); };
> > int main(void) { return ref; }
> > ```
> > ?
> > It's already supported by clang https://godbolt.org/z/6j89EdnEo
> > 
> > 
> > I'm adding the other tests (and fixing the associated bugs, of which there 
> > were a few...)
> > 
> 
> I see it now. Thanks, I hate it. There is apparently a preexisting bug.
> And yes, i think we should say something about members of anonymous union 
> declared at namespace scope in the standard I realize now this is missing
> Thanks for catching that.
> Thanks for catching that.

Glad to be of help!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153536

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

[PATCH] D153536: [Clang] Implement P2169 A nice placeholder with no name

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/SemaCXX/cxx2c-placeholder-vars.cpp:2
+// RUN: %clang -cc1 -fsyntax-only -verify -std=c++2c -Wunused-parameter 
-Wunused %s
+
+void static_var() {

hubert.reinterpretcast wrote:
> hubert.reinterpretcast wrote:
> > hubert.reinterpretcast wrote:
> > > Can we have tests for:
> > > ```
> > > struct { int _, _; } a = { ._ = 0 };
> > > ```
> > > 
> > > and
> > > 
> > > ```
> > > struct A {
> > >   A();
> > >   int _, _;
> > > };
> > > 
> > > A::A() : _(0) {}
> > > ```
> > Codegen test for
> > ```
> > static union { int _ = 42; };
> > int  = _;
> > int foo() { return 13; }
> > static union { int _ = foo(); };
> > int main(void) { return ref; }
> > ```
> > might be interesting.
> > 
> > I suspect that this case was missed in the committee discussion of the 
> > paper @cor3ntin.
> Less controversial tests to consider:
> ```
> struct A {
>   int _;
>   union { int _; };
> };
> struct B { union { int _, _; }; };
> ```
> 
In a similar vein, a codegen test for:
```
struct A { A(); };
inline void f [[gnu::used]]() {
  static union { A _{}; };
  static union { A _{}; };
}
```

Perhaps not intended to be allowed though (premise was no symbols with 
"linkage"?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153536

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

[PATCH] D153536: [Clang] Implement P2169 A nice placeholder with no name

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/SemaCXX/cxx2c-placeholder-vars.cpp:2
+// RUN: %clang -cc1 -fsyntax-only -verify -std=c++2c -Wunused-parameter 
-Wunused %s
+
+void static_var() {

hubert.reinterpretcast wrote:
> Can we have tests for:
> ```
> struct { int _, _; } a = { ._ = 0 };
> ```
> 
> and
> 
> ```
> struct A {
>   A();
>   int _, _;
> };
> 
> A::A() : _(0) {}
> ```
Codegen test for
```
static union { int _ = 42; };
int  = _;
int foo() { return 13; }
static union { int _ = foo(); };
int main(void) { return ref; }
```
might be interesting.

I suspect that this case was missed in the committee discussion of the paper 
@cor3ntin.



Comment at: clang/test/SemaCXX/cxx2c-placeholder-vars.cpp:2
+// RUN: %clang -cc1 -fsyntax-only -verify -std=c++2c -Wunused-parameter 
-Wunused %s
+
+void static_var() {

hubert.reinterpretcast wrote:
> hubert.reinterpretcast wrote:
> > Can we have tests for:
> > ```
> > struct { int _, _; } a = { ._ = 0 };
> > ```
> > 
> > and
> > 
> > ```
> > struct A {
> >   A();
> >   int _, _;
> > };
> > 
> > A::A() : _(0) {}
> > ```
> Codegen test for
> ```
> static union { int _ = 42; };
> int  = _;
> int foo() { return 13; }
> static union { int _ = foo(); };
> int main(void) { return ref; }
> ```
> might be interesting.
> 
> I suspect that this case was missed in the committee discussion of the paper 
> @cor3ntin.
Less controversial tests to consider:
```
struct A {
  int _;
  union { int _; };
};
struct B { union { int _, _; }; };
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153536

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

[PATCH] D153536: [Clang] Implement P2169 A nice placeholder with no name

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/SemaCXX/cxx2c-placeholder-vars.cpp:2
+// RUN: %clang -cc1 -fsyntax-only -verify -std=c++2c -Wunused-parameter 
-Wunused %s
+
+void static_var() {

Can we have tests for:
```
struct { int _, _; } a = { ._ = 0 };
```

and

```
struct A {
  A();
  int _, _;
};

A::A() : _(0) {}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153536

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

[PATCH] D152021: [clang][AIX] Fix Overly Strict LTO Option Checking against `data-sections` when `mxcoff-roptr` is in Effect

[Hubert Tong via Phabricator via cfe-commits]

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

LGTM with minor comment.




Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:732-735
UseSeparateSections))
 CmdArgs.push_back(
 Args.MakeArgString(Twine(PluginOptPrefix) + "-data-sections=1"));
+  else if (Args.hasArg(options::OPT_fno_data_sections)) {

Coding standards suggest that braces be used consistently for each part of an 
if/else chain.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152021

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

[PATCH] D143670: Stop claiming we support [[carries_dependency]]

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

Updated patch limited to changing the feature test macro value would match 
Varna meeting outcome (changes to allow macro to be `0` accepted as DR).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143670

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

[PATCH] D153600: Implement -frecord-command-line for XCOFF

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:2338
   // Emit bytes for llvm.commandline metadata.
-  emitModuleCommandLines(M);
+  // The command line metadata waas emitted earlier on XCOFF.
+  if (!TM.getTargetTriple().isOSBinFormatXCOFF())

Minor nit: Typo.



Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:2974
+RSOS << "@(#)" << MDS->getString();
+RSOS.write('\0');
+  }

stephenpeckham wrote:
> I would use a newline here.  The AIX **what **command looks for @(#) and 
> echos subsequent bytes until it sees a double quote, a backslash, a > symbol, 
> newline, or null byte.  The @(#) is not echoed, nor is the terminating 
> character.  The **what **command prints a newline after it finds a 
> terminating character.  This means that if the command line contains any of 
> the special characters, the line will be truncated.
> 
> Exception:  If the @(#) is followed by "opt " or " opt ", the terminating 
> characters are only a newline or null byte. This allows any of the other 
> special characters to be part of the command line. It doesn't really matter 
> if you use a newline or a null byte, but the legacy XL compiler uses a 
> newline. The "opt" keyword should appear if the command line can contain a 
> double quote, a > or a backslash.
> 
> The legacy compiler also uses other keywords besides "opt", including 
> "version" and "cfg".  The **what** command doesn't do anything special with 
> these keywords.
As mentioned offline, newline on its own has potential of ambiguity because it 
can appear in command line options (null bytes cannot). If there is a 
preference for newline to be present, then having a null byte after could help.

Note that `@(#)opt ` can appear on the command line too. Using `what` will have 
limitations (but we should leave the possibility open for other tools/methods 
to work).



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

https://reviews.llvm.org/D153600

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

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/include/clang/Basic/CodeGenOptions.def:475
 
+/// Whether to emit all variables that have a persisent storage duration,
+/// including global, static and thread local variables.

Minor nit: Typo.



Comment at: clang/include/clang/Driver/Options.td:1700
   BothFlags<[NoXarchOption], " static const variables if unused">>;
+defm keep_persistent_storage_variables : 
BoolFOption<"keep-persistent-storage-variables",
+  CodeGenOpts<"KeepPersistentStorageVariables">, DefaultFalse,

Please update the patch title and description to match.



Comment at: clang/test/CodeGen/keep-persistent-storage-variables.cpp:18-19
+// CHECK: @_ZN2ST2s6E = global i32 7, align 4
+// CHECK-ELF: @llvm.compiler.used = appending global [14 x ptr] [ptr @_ZL2g1, 
ptr @_ZL2g2, ptr @_ZL2g3, ptr @_ZL2g4, ptr @tl1, ptr @tl2, ptr @_ZL3tl3, ptr 
@_ZL3tl4, ptr @g5, ptr @g6, ptr @_ZZ5test3vE2s3, ptr @_ZN12_GLOBAL__N_12s4E, 
ptr @_ZZ5test5vE3tl5, ptr @_ZN2ST2s6E], section "llvm.metadata"
+// CHECK-NON-ELF: @llvm.used = appending global [14 x ptr] [ptr @_ZL2g1, ptr 
@_ZL2g2, ptr @_ZL2g3, ptr @_ZL2g4, ptr @tl1, ptr @tl2, ptr @_ZL3tl3, ptr 
@_ZL3tl4, ptr @g5, ptr @g6, ptr @_ZZ5test3vE2s3, ptr @_ZN12_GLOBAL__N_12s4E, 
ptr @_ZZ5test5vE3tl5, ptr @_ZN2ST2s6E], section "llvm.metadata"
+

If we don't care which of `llvm.compiler.used` or `llvm.used` is used (no pun 
intended), then maybe we can use a regex? For example, 
`@llvm{{(\.compiler)?}}.used`.



Comment at: clang/test/CodeGen/keep-persistent-storage-variables.cpp:32-39
+int test1() {
+  g1 = 3;
+  return g1;
+}
+
+int test2() {
+  return g2;

Why add functions that use `g1` and `g2`? Is there some reason to suspect that 
using the variables will cause them not to be emitted as explicitly used?

If removing these functions, please also remove `g3` and `g4` as redundant.



Comment at: clang/test/CodeGen/keep-persistent-storage-variables.cpp:50
+}
+void *test4() { return  }
+

Same comment re: why add a function?



Comment at: clang/test/Driver/fkeep-persistent-storage-variables.c:1
+// RUN: %clang -fkeep-persistent-storage-variables -c %s -### 2>&1 | FileCheck 
%s
+

Please add test for `-fkeep-persistent-storage-variables 
-fno-keep-persistent-storage-variables`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150221

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

[PATCH] D151567: [LLVM][Support] Report EISDIR when opening a directory on AIX

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D151567#4458232 , 
@hubert.reinterpretcast wrote:

> @azhan92, please incorporate a revert of 
> https://reviews.llvm.org/rG64ca650cf9f180cc0b68c0005639028084066e10. Since it 
> is an `XFAIL`. once the problem is fixed, the test will end up being an 
> "unexpected success" unless we remove the `XFAIL`.

@azhan92, I believe that the fix needs to be done at a lower level. Currently, 
we are addressing one case (in `expandResponseFiles`). That does not address 
other cases where the ability to open directories for reading on AIX causes 
unexpected behaviour for LLVM.

We should go deeper to see where the `EISDIR` error originates (likely "below" 
the `expandResponseFile` call) on other platforms.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151567

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

[PATCH] D151567: [LLVM][Support] Report EISDIR when opening a directory on AIX

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

@azhan92, rG6ace52e5e49cff6664fc301fa4985fc28c88f26f 
 and 
rGc14df228ff3ca73e3c5c00c495216bba56665fd5 
 should 
also be reverted. Same reason: `XFAIL`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151567

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

[PATCH] D151567: [LLVM][Support] Report EISDIR when opening a directory on AIX

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

@azhan92, please incorporate a revert of 
https://reviews.llvm.org/rG64ca650cf9f180cc0b68c0005639028084066e10. Since it 
is an `XFAIL`. once the problem is fixed, the test will end up being an 
"unexpected success" unless we remove the `XFAIL`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151567

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

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/CodeGen/keep-static-variables.cpp:1
+// RUN: %clang_cc1 -fkeep-static-variables -emit-llvm %s -o - 
-triple=x86_64-unknown-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-ELF
+// RUN: %clang_cc1 -fkeep-static-variables -emit-llvm %s -o - 
-triple=powerpc64-ibm-aix-xcoff | FileCheck %s 
--check-prefixes=CHECK,CHECK-NON-ELF

@qianzhen, a driver option was added. IMO, there should be tests (probably in 
another file) for the driver option too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150221

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

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2201-2210
+  if ((CodeGenOpts.KeepStaticConsts || CodeGenOpts.KeepStaticVariables) && D &&
+  isa(D)) {
 const auto *VD = cast(D);
-if (VD->getType().isConstQualified() &&
-VD->getStorageDuration() == SD_Static)
-  addUsedOrCompilerUsedGlobal(GV);
+if (VD->getStorageDuration() == SD_Static) {
+  if (CodeGenOpts.KeepStaticVariables)
+addUsedOrCompilerUsedGlobal(GV);
+  else if (CodeGenOpts.KeepStaticConsts && 
VD->getType().isConstQualified())

aaron.ballman wrote:
> Reformatted with whatever clang-format does for that, as I doubt I have the 
> formatting correct.
@qianzhen, I don't think the suggestion was applied/applied correctly.

There should not be an `isa` followed by `dyn_cast`. That said, I think 
`dyn_cast_or_null` is perhaps appropriate instead of plain `dyn_cast`. 
Additionally, `VD` being non-null should be part of the condition.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150221

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

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D150221#4351249 , @rjmccall wrote:

> Force-emitting every `static` in the translation unit can be very expensive; 
> there are a lot of headers that declare all their constants as `static 
> const`.  And removing dead globals is a pretty important optimization, e.g. 
> to eliminate the string literals used by assertions that the compiler was 
> able to fold to `true`.  So I don't think it's unreasonable for us to ask 
> whether we should just be naively adding this option instead of figuring out 
> a more targeted way of satisfying these users.

Seeing as there are application developers paying the cost of--and relying on 
at least some aspects of--this behaviour, I am looking to see how they can move 
to Clang with less migration overhead. It seems to me that more targeted ways 
to solve the issue would require that the application developers apply a 
mechanism to select variables for this treatment. That involves a trade-off for 
them between performance and uptime-friendly serviceability. If they don't have 
an analysis to make those decisions, then having a more complicated tunable 
interface does not help them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150221

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

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D150221#4350761 , @rjmccall wrote:

> I'm not sure if it's better to represent that by using 
> `__attribute__((used))` on every global variable or by setting something more 
> globally in the module.  The latter seems more impervious to the LTO problem, 
> at least.

`__attribute__((__used__))` has the advantage of being something that already 
exists. If the "more global" property can hook in as treating all variables as 
`__attribute__((__used__))` where the query for `__attribute__((__used__))` is 
implemented in the middle-end/back-end (including places where the symbol is 
marked as used with respect to linker garbage collection), then maybe it is not 
too intrusive in terms of implementation.

As for which aspect of `__attribute__((__used__))` is more important: The 
explicit user request we had was for the variables to be kept in an obvious 
manner in the symbol table. It seems multiple aspects of the 
`__attribute__((__used__))` semantics are helpful here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150221

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

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D150221#4347840 , @efriedma wrote:

>> This is an adaptation of the IBM XL compiler's -qstatsym option, which is 
>> meant to generate symbol table entries for static variables. An artifact of 
>> that compiler is that static variables are often not discarded even when 
>> unused.
>
> Oh, I see; the compiler actually doesn't guarantee that the variables are 
> preserved, it just ends up preserving them by accident because it's bad at 
> optimizing global variables?

That is my understanding, yes. However, that behaviour seems to serve the use 
case: Users can enable functions for hot-patching proactively to allow for bug 
fixes in live instances, and they would not necessarily know which static 
variables they need preserved for use in the replacement code up front. 
Additionally, sometimes the variables are not eliminated but their location is 
obfuscated by GlobalMerge (and potentially other optimizations).

The `-fkeep-static-variables` option only addresses the use case when users do 
not apply LTO though. LTO internalization exposes more variables to the 
obfuscation/removal issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150221

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

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D150221#4343546 , @efriedma wrote:

> It's not unprecedented to add flags to copy the behavior of other compilers, 
> to make porting easier, especially when it doesn't place much burden on 
> compiler maintainers.  But what compiler preserves the names/values of static 
> variables by default?  It's not the sort of feature I'd expect anyone to 
> advertise.  And we don't really want to encourage people to use global flags 
> for this sort of thing; applying behavior everywhere has weird effects, and 
> users often don't understand the flags they're using.

This is an adaptation of the IBM XL compiler's `-qstatsym` option, which is 
meant to generate symbol table entries for static variables. An artifact of 
that compiler is that static variables are often not discarded even when 
unused. We attempt to achieve the combined effect using 
`-fkeep-static-variables`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150221

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

[PATCH] D150597: [clang][AIX] Adding Revised xcoff-roptr CodeGen Test Case

[Hubert Tong via Phabricator via cfe-commits]

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

LGTM; thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150597

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

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

[Hubert Tong via Phabricator via cfe-commits]

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

LGTM with minor comments!




Comment at: clang/lib/Driver/ToolChains/AIX.cpp:132
+// possible. Then `-bforceimprw` changes such sections to RW if they 
contain
+// imported symbols that needs to be resolved.
+CmdArgs.push_back("-bforceimprw");

Minor nit: needs => need



Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:728-729
+if (HasRoptr) {
+  // On AIX, data_sections is on by default. We only need to check
+  // if data_sections is explicitly turned off.
+  if (!Args.hasFlag(options::OPT_fdata_sections,

Remove the comment. The `hasFlag` check requires no special reasoning.



Comment at: clang/test/CodeGen/PowerPC/aix-roptr.c:13
+char c1 = 10;
+char c2 = 20;
+char* const c1_ptr = 

Remove `c2`. Not needed for the test.



Comment at: clang/test/CodeGen/PowerPC/aix-roptr.c:27-30
+
+int main() {
+*(char**)_ptr = 
+}

Remove `main`. Not needed for the test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144190

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

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D150221#4332142 , @erichkeane 
wrote:

>> This is intended to prevent "excessive transformation" to enable migration 
>> of existing applications (using a non-Clang compiler) where users further 
>> manipulate the object or assembly after compilation.
>
> I don't get what you mean by this?  I don't currently see motivation for this?

The intent is to apply the `__attribute__((__used__))` semantic to static 
variables (since the front-end is likely to discard them). Additional reasons 
for using `__attribute__((__used__))` applies: The compiler cannot optimize 
with the assumption that it sees all direct accesses to the variable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150221

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

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D150221#4330504 , @MaskRay wrote:

> Can you give a more compelling reason that this option is needed?

This is intended to prevent "excessive transformation" to enable migration of 
existing applications (using a non-Clang compiler) where users further 
manipulate the object or assembly after compilation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150221

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

[PATCH] D146399: [AIX][Clang][K] Create `-K` Option for AIX.

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D146399#4244479 , @francii wrote:

> Do we need a test case for the diagnostic? If we are only supporting one 
> target, which triple would be used for the test?

I think we do need a test case for the diagnostic. I suspect it should be in a 
separate file.
Unlike a lot of the options work we did for AIX, this is adding a new option 
(not processing an existing one).
I suggest `powerpc64-unknown-linux-gnu` for the triple.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146399

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

[PATCH] D146399: [AIX][Clang][K] Create `-K` Option for AIX.

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

@francii, what happens when `-K` is used on a pure-compile (`-c`) invocation? 
Do we get an "unused" message? Should we be testing that?
I think we should be testing the diagnostic for the "wrong target" usage as 
well?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146399

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

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/Driver/ToolChains/AIX.cpp:125
 
+  // The `-mroptr` option places constants in RO sections as much as possible.
+  // Then `-bforceimprw` changes such sections to RW if they contain imported

Old option name in comment text.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1965
+// therefore, we require that separate data sections
+// are used when `-mroptr` is in effect. We respect the setting of
+// data-sections since we have not found reasons to do otherwise that

Old option name in comment text.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144190

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

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:317
+  ``-fno-data-sections``. When ``-mxcoff-roptr`` is in effect at link time,
+  read-only  data sections with relocatable address values that resolve to
+  imported symbols are made writable.

Two spaces => one space



Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:718-721
+  // On AIX, data_sections is on by default. We only need to check
+  // if data_sections is explicitly turned off.
+  if (Args.hasArg(options::OPT_fno_data_sections))
+D.Diag(diag::err_roptr_requires_data_sections);

Should use `hasFlag` to check if data_sections is off.

As background:
The logic near line 699 says to add `-data-sections=1` if data_sections is on 
and, if data_sections is off, only add `-data-sections=0` if 
`-fno-data-sections` was explicitly present. This allows the LTO default for 
data_sections to be a separate policy from the non-LTO default.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144190

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

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:647
+def err_roptr_requires_data_sections: Error<"-mxcoff-roptr is supported only 
with -fdata-sections">;
+def err_roptr_cannot_build_shared: Error<"-mxcoff-roptr is not suppored with 
-shared">;
 

Typo.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5258-5259
+if (HasRoptr) {
+  if (Args.hasArg(options::OPT_shared))
+D.Diag(diag::err_roptr_cannot_build_shared);
+

I think this belongs in the code for AIX linker jobs.



Comment at: clang/test/Driver/ppc-roptr.c:37
+// LINK: "-bforceimprw"
+// LTO_ROPTR: "-bplugin_opt:-mxcoff-roptr"
+// NO_ROPTR-NOT: "-mxcoff-roptr"

This needs the backend option also renamed. Has a commit for that landed yet?



Comment at: clang/test/Driver/ppc-roptr.c:15
+// RUN: %clang -### --target=powerpc64-ibm-aix-xcoff -mroptr -flto %s 2>&1 | \
+// RUN: FileCheck %s --check-prefix=LTO_ROPTR
+// RUN: touch %t.o

This is compile+LTO link. We can also add `ROPTR,LINK` prefixes here, right?



Comment at: clang/test/Driver/ppc-roptr.c:24
+// RUN: %s 2>&1 | FileCheck %s --check-prefix=TARGET_NOROPTR_ERR
+// RUN: %clang -### --target=powerpc64-ibm-aix-xcoff -mroptr -shared \
+// RUN: %s 2>&1 | FileCheck %s --check-prefix=SHARED_ERR

Please make this case pure-link. The diagnostic is most relevant on the link 
step because that is when `-shared` takes effect.



Comment at: clang/test/Driver/ppc-roptr.c:26
+// RUN: %s 2>&1 | FileCheck %s --check-prefix=SHARED_ERR
+// RUN: %clang -### --target=powerpc64le-unknown-linux-gnu -mroptr -flto \
+// RUN: %s 2>&1 | FileCheck %s --check-prefix=TARGET_ROPTR_ERR

Please make this case pure-link; otherwise, we will pass the test simply 
because the diagnostic is also implemented for compiling-from-source.



Comment at: clang/test/Driver/ppc-roptr.c:28
+// RUN: %s 2>&1 | FileCheck %s --check-prefix=TARGET_ROPTR_ERR
+// RUN: %clang -### --target=powerpc64-ibm-aix-xcoff -mroptr -flto 
-fno-data-sections \
+// RUN: %s 2>&1 | FileCheck %s --check-prefix=DATA_SECTION_ERR

Same comment.



Comment at: clang/test/Driver/ppc-roptr.c:30
+// RUN: %s 2>&1 | FileCheck %s --check-prefix=DATA_SECTION_ERR
+// RUN: %clang -### --target=powerpc64le-unknown-linux-gnu -mno-roptr -flto \
+// RUN: %s 2>&1 | FileCheck %s --check-prefix=TARGET_NOROPTR_ERR

Same comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144190

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

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:714-716
+if (!Args.hasFlag(options::OPT_fdata_sections,
+  options::OPT_fno_data_sections, UseSeparateSections) &&
+Args.hasArg(options::OPT_fno_data_sections))

I think this (undesirably) generates an error even `-mno-roptr` is in effect.

This logic seems otherwise convoluted. I think the main logic should only care 
that `data-sections` is off. We can separately assert that `data-sections` is 
on by default on AIX.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144190

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

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5250
 
+  if (Args.hasArg(options::OPT_mroptr) || Args.hasArg(options::OPT_mno_roptr)) 
{
+bool HasRoptr =

qiongsiwu1 wrote:
> hubert.reinterpretcast wrote:
> > This only checks for `-m[no-]roptr` when the front-end is invoked. When the 
> > driver is used just for linking, we get no diagnostic at all for using 
> > these options on other platforms.
> > 
> > @MaskRay:
> > 
> > As Clang code owner for the Driver, are you concerned if platform-specific 
> > options are sometimes silently ignored on other platforms? Is there a 
> > better place for the diagnostic?
> > 
> > For info, in this case, the option would have effects on AIX at both 
> > compile-from-C/C++ time and at link time. 
> > 
> > Also, should `-Wunused-command-line-argument` warnings be generated for 
> > cases where the option has no effect (e.g., `-emit-llvm` or 
> > `-fsyntax-only`)? I think `-flto` is a bit of an exception because it may 
> > be unfriendly to expect users to change other aspects of their compile 
> > invocations when adding `-flto`. At the same time, it can be argued instead 
> > that generating a `-Wunused-command-line-argument` warning for "back-end" 
> > options on `-flto` compiles helps the user realize that they need to 
> > provide the option on their link step.
> > 
> @MaskRay May I ask for your input on this issue where platform-specific 
> options are silently ignored on other platforms? What is our current opinion 
> on where these checks/diagnostics should go? 
> 
> Thanks!! 
Hi @MaskRay,

Thank you for your comments so far. I am particularly interested diagnostics 
for cases where the option does not take effect. We are hoping for a good 
approach to implement the diagnostic when the option is used on non-AIX 
platforms (without the current issue that we only check when the front-end is 
invoked). Additionally, I would like your opinion on whether a diagnostic is 
warranted for cases like `-emit-llvm` or `-fsyntax-only`. What existing 
command-line options are you aware of that can serve as a good design template 
that we can apply here?

Thanks again,
HT



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144190

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

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/Driver/ppc-roptr.c:10
+// ROPTR-NOT: "-mroptr"
+// ROPTR-NOT: "-bforceimprw"
+

hubert.reinterpretcast wrote:
> w2yehia wrote:
> > hubert.reinterpretcast wrote:
> > > Do we pass the option through to the LTO codegen when 
> > > `-fprofile-generate` is used?
> > > We may need a driver diagnostic for `-mroptr` without data sections when 
> > > linking LTO objects (because the front-end won't be involved).
> > > 
> > > @w2yehia, does LLVM trunk have a gap in LTO driver enablement for AIX?
> > > does LLVM trunk have a gap in LTO driver enablement for AIX?
> > 
> > I think trunk is up to date in terms of LTO (driver and libLTO); this was 
> > done in LLVM 16.0.
> > 
> > > Do we pass the option through to the LTO codegen when -fprofile-generate 
> > > is used?
> > 
> > what's special with  -fprofile-generate in relation to the ROPTR option?
> Sorry, I meant on the LTO link step.
It seems to me that we don't pass the option through to the LTO codegen on the 
LTO link step (which is a problem).

It also seems that there is no diagnostic generated (would likely hit the 
back-end assertion/error) when `-fno-data-sections` and `-mroptr` is specified 
at the same time for the LTO link step. I believe a driver diagnostic needs to 
be added.

I am of the opinion that we don't have to have a front-end diagnostic in 
addition to a driver diagnostic if the back-end will error out in the case 
where relevant codegen is encountered (it provides an esoteric way to quickly 
check if relevant codegen is involved for a given source file). We should apply 
the same policy between `clang -cc1` and `llc` though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144190

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

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5250
 
+  if (Args.hasArg(options::OPT_mroptr) || Args.hasArg(options::OPT_mno_roptr)) 
{
+bool HasRoptr =

This only checks for `-m[no-]roptr` when the front-end is invoked. When the 
driver is used just for linking, we get no diagnostic at all for using these 
options on other platforms.

@MaskRay:

As Clang code owner for the Driver, are you concerned if platform-specific 
options are sometimes silently ignored on other platforms? Is there a better 
place for the diagnostic?

For info, in this case, the option would have effects on AIX at both 
compile-from-C/C++ time and at link time. 

Also, should `-Wunused-command-line-argument` warnings be generated for cases 
where the option has no effect (e.g., `-emit-llvm` or `-fsyntax-only`)? I think 
`-flto` is a bit of an exception because it may be unfriendly to expect users 
to change other aspects of their compile invocations when adding `-flto`. At 
the same time, it can be argued instead that generating a 
`-Wunused-command-line-argument` warning for "back-end" options on `-flto` 
compiles helps the user realize that they need to provide the option on their 
link step.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144190

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

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:231
+  address values in the read-only data section. This option is intended to
+  be used with the ``-fdata-sections`` option. When ``-mroptr`` is in effect,
+  read-only data sections with relocatable address values that resolve to

Suggestion:
is in effect at link time,



Comment at: clang/test/CodeGen/PowerPC/aix-roptr.c:9-10
+// RUN: -S <%s 2>&1 | FileCheck %s --check-prefix=DATA_SECTION_ERR
+// RUN: not %clang_cc1 -triple=powerpc64le-unknown-linux-gnu -mroptr \
+// RUN: -S <%s 2>&1 | FileCheck %s --check-prefix=TARGET_ROPTR_ERR
+// RUN: not %clang -target powerpc64le-unknown-linux-gnu -mno-roptr \

hubert.reinterpretcast wrote:
> Note that a driver test is needed for this sort of case as well.
`-mroptr` with a wrong target is currently also a front-end diagnostic (which 
we should then be testing here).

Having such a front-end diagnostic makes sense to me because the `-cc1` option 
may get silently ignored otherwise: the back-end proper does not ensure that it 
is unset for other targets (instead `llc` has the checking).



Comment at: clang/test/Driver/ppc-roptr.c:6
+// RUN: FileCheck %s --check-prefix=ROPTR
+// RUN: %clang -### -target powerpc-ibm-aix-xcoff -mroptr -mno-roptr %s 2>&1 | 
\
+// RUN: FileCheck %s --check-prefix=NO_ROPTR

May also want to test that the default behaviour is `NO_ROPTR`.



Comment at: clang/test/Driver/ppc-roptr.c:1
+// RUN: %clang -### -target powerpc-ibm-aix-xcoff -mroptr %s 2>&1 | FileCheck 
%s
+// RUN: %clang -### -target powerpc-ibm-aix-xcoff -mroptr -mno-roptr %s 2>&1 | 
\

qiongsiwu1 wrote:
> hubert.reinterpretcast wrote:
> > Should test for `-c` and `-S` as well.
> > Should test for pure link (without compile) as well.
> > Should test for pure link (without compile) as well.
> 
> Is there a set of options that I can use to trigger pure linking? Or should I 
> use two .o input files to trigger pure linking? 
Just one `.o` file will trigger pure linking. For example, some tests do a 
`touch %t.o`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144190

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

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/Driver/ppc-roptr.c:10
+// ROPTR-NOT: "-mroptr"
+// ROPTR-NOT: "-bforceimprw"
+

w2yehia wrote:
> hubert.reinterpretcast wrote:
> > Do we pass the option through to the LTO codegen when `-fprofile-generate` 
> > is used?
> > We may need a driver diagnostic for `-mroptr` without data sections when 
> > linking LTO objects (because the front-end won't be involved).
> > 
> > @w2yehia, does LLVM trunk have a gap in LTO driver enablement for AIX?
> > does LLVM trunk have a gap in LTO driver enablement for AIX?
> 
> I think trunk is up to date in terms of LTO (driver and libLTO); this was 
> done in LLVM 16.0.
> 
> > Do we pass the option through to the LTO codegen when -fprofile-generate is 
> > used?
> 
> what's special with  -fprofile-generate in relation to the ROPTR option?
Sorry, I meant on the LTO link step.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144190

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

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a subscriber: w2yehia.
hubert.reinterpretcast added inline comments.



Comment at: clang/test/CodeGen/PowerPC/aix-roptr.c:9-10
+// RUN: -S <%s 2>&1 | FileCheck %s --check-prefix=DATA_SECTION_ERR
+// RUN: not %clang_cc1 -triple=powerpc64le-unknown-linux-gnu -mroptr \
+// RUN: -S <%s 2>&1 | FileCheck %s --check-prefix=TARGET_ROPTR_ERR
+// RUN: not %clang -target powerpc64le-unknown-linux-gnu -mno-roptr \

Note that a driver test is needed for this sort of case as well.



Comment at: clang/test/CodeGen/PowerPC/aix-roptr.c:11-16
+// RUN: not %clang -target powerpc64le-unknown-linux-gnu -mno-roptr \
+// RUN: -S %s 2>&1 | FileCheck %s --check-prefix=TARGET_NOROPTR_ERR
+// RUN: not %clang -target powerpc-ibm-aix-xcoff -mroptr -shared \
+// RUN: -S %s 2>&1 | FileCheck %s --check-prefix=SHARED_ERR
+// RUN: not %clang -target powerpc64-ibm-aix-xcoff -mroptr -shared \
+// RUN: -S %s 2>&1 | FileCheck %s --check-prefix=SHARED_ERR

These are driver tests. Please move (and use `-###` to avoid accidental 
reliance on front-end processing).



Comment at: clang/test/Driver/ppc-roptr.c:1
+// RUN: %clang -### -target powerpc-ibm-aix-xcoff -mroptr %s 2>&1 | FileCheck 
%s
+// RUN: %clang -### -target powerpc-ibm-aix-xcoff -mroptr -mno-roptr %s 2>&1 | 
\

Should test for `-c` and `-S` as well.
Should test for pure link (without compile) as well.



Comment at: clang/test/Driver/ppc-roptr.c:3
+// RUN: %clang -### -target powerpc-ibm-aix-xcoff -mroptr -mno-roptr %s 2>&1 | 
\
+// RUN: FileCheck %s --check-prefix=ROPTR
+// RUN: %clang -### -target powerpc64-ibm-aix-xcoff -mroptr %s 2>&1 | 
FileCheck %s

I think the prefix for this should be named `NO_ROPTR`.



Comment at: clang/test/Driver/ppc-roptr.c:10
+// ROPTR-NOT: "-mroptr"
+// ROPTR-NOT: "-bforceimprw"
+

Do we pass the option through to the LTO codegen when `-fprofile-generate` is 
used?
We may need a driver diagnostic for `-mroptr` without data sections when 
linking LTO objects (because the front-end won't be involved).

@w2yehia, does LLVM trunk have a gap in LTO driver enablement for AIX?



Comment at: clang/test/Driver/ppc-roptr.c:12-18
+char c1 = 10;
+char c2 = 20;
+char* const c1_ptr = 
+
+int main() {
+*(char**)_ptr = 
+}

Why have any code here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144190

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

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:229-231
+- Introduced the ``-mroptr`` option to place constant objects with relocatable
+  address values in the ready-only data section. This option is intended to
+  be used with the ``-fdata-sections`` option.

Should also mention the link-time behaviour that causes the read-only data 
sections with relocatable address values that resolve to imported symbols to be 
made writable.



Comment at: clang/docs/ReleaseNotes.rst:230
+- Introduced the ``-mroptr`` option to place constant objects with relocatable
+  address values in the ready-only data section. This option is intended to
+  be used with the ``-fdata-sections`` option.

Typo fix.



Comment at: clang/include/clang/Driver/Options.td:3896
+def mroptr : Flag<["-"], "mroptr">, Group, Flags<[CC1Option]>,
+  HelpText<"Place constant objects with relocatable address values in the RO 
data section">;
+def mno_roptr : Flag<["-"], "mno-roptr">, Group;

Also: "Implies -bforceimprw when specified at link time."



Comment at: clang/lib/Driver/ToolChains/AIX.cpp:127
+  // The `-mroptr` option places constants in RO sections as much as possible.
+  // Then `-bforceimprw` changes such sections to RW if they contain import
+  // symbols that needs to be resolved.

Minor nit: grammar


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144190

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

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

[Hubert Tong via Phabricator via cfe-commits]

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



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5251
 
+  if (Args.hasFlag(options::OPT_mroptr, options::OPT_mno_roptr, false)) {
+if (!Triple.isOSAIX())

This accepts `-mno-roptr` for other platforms despite having no semantic 
functionality (e.g., it does not move variables to a different section for ELF 
codegen).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144190

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

[PATCH] D143670: Stop claiming we support [[carries_dependency]]

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/Preprocessor/has_attribute.cpp:51
 // FIXME(201806L) CHECK: assert: 0
-// CHECK: carries_dependency: 200809L
+// CHECK: carries_dependency: 0
 // CHECK: deprecated: 201309L

aaron.ballman wrote:
> hubert.reinterpretcast wrote:
> > I'd trust this when I see the change to [cpp.cond]/6 arrive in CWG.
> This is consistent with how we handle `[[no_unique_address]]` (see line 59).
Is that driven by compatibility concerns on the platform though? For example, 
we might be doing something on line 59 to match MSVC.

Do we know if GCC intends to make a similar change for `carries_dependency`? It 
may help to be more explicit about why our situation is different from GCC's.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143670

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

[PATCH] D142867: [Clang] Add machinery to catch overflow in unary minus outside of a constant expression context

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D142867#4108079 , @eaeltsin wrote:

> The warning now fires even if overflow is prevented with `if constexpr`:
>
>   if constexpr (width <= 64) {
> if constexpr (width == 64) {
>   return 1;
> }
> return -static_cast(uint64_t{1} << (width - 1));
>   }
>
> https://godbolt.org/z/M3xdcKd3M

For reference, actually placing the second return into an `else` block (thus 
making it a discarded statement) does suppress the diagnostic:
https://godbolt.org/z/Kb6Md5PrK

I also question the focus on `if constexpr`. Replacing with `if (true) { return 
1; }` should be as effective in the non-`else`-block case in suppressing the 
warning in terms of QoI.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142867

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

[PATCH] D143891: [Clang] Adjust triviality computation in QualType::isTrivialType to C++20 cases.

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D143891#4122660 , @aaron.ballman 
wrote:

> This is an ABI breaking change, isn't it? (The type trait now returns 
> something different than it did before, which could change instantiations or 
> object layout.)

In my opinion, this is an argument for not fixing in a fix release of a 
specific version of Clang--not an argument for additional option control in new 
versions.
Instantiations change all the time from changes to constant expression 
evaluation, overload resolution, partial ordering rules for templates, etc.
As for object layout, I believe the language and the ABI rules for triviality 
diverged quite some time ago.

I know that the evaluation for this specific case was that we don't need to 
apply ABI versioning control for this because it only affects code using 
Concepts, but I think we will eventually need to (re)discover when ABI 
versioning control is the appropriate tool.

I propose an action item: Someone needed to review past application of the ABI 
versioning options to see if we can extract and document workable criteria 
(perhaps in https://clang.llvm.org/docs/InternalsManual.html).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143891

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

[PATCH] D143670: Stop claiming we support [[carries_dependency]]

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

I think it may be an option to use the `gnu++*` modes to do deliberately 
non-conforming things, but I believe there should be an RFC for that first.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143670

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

[PATCH] D143670: Stop claiming we support [[carries_dependency]]

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast requested changes to this revision.
hubert.reinterpretcast added inline comments.



Comment at: clang/test/Preprocessor/has_attribute.cpp:51
 // FIXME(201806L) CHECK: assert: 0
-// CHECK: carries_dependency: 200809L
+// CHECK: carries_dependency: 0
 // CHECK: deprecated: 201309L

I'd trust this when I see the change to [cpp.cond]/6 arrive in CWG.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143670

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