[PATCH] D142660: [AIX] supporting -X options for llvm-ranlib in AIX OS

[James Henderson via Phabricator via cfe-commits]

jhenderson accepted this revision.
jhenderson added a comment.

I'm going to accept this change, although I still have significant concerns 
about how the whole parsing logic seems more complicated than it needs to be.




Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:74
+ << "  -X{32|64|32_64|any}   - Specify which archive symbol tables "
+"should be generated if they do not already exist (AIX OS only)\n";
 }

DiggerLin wrote:
> jhenderson wrote:
> > Strictly speaking, this should be "Big Archive formats only" not "AIX OS 
> > only" since theoretically you could have Big Archive archives on other 
> > platforms. However, I'm not bothered if you don't want to change this. The 
> > same probably goes for other tools for that matter, but don't change them 
> > now.
> Strictly speaking, this should be "Big Archive format on AIX OS only" ,
> in AIX OS , you can still input the -X option , but the X option not work for 
> gnu archive format.
Ah, sorry, I misremembered the code, you are right.

It does raise a question whether the -X option at least should be available on 
non-AIX platforms, because otherwise there's no way of controlling the symbol 
table behaviour like there is on AIX. However, that's probably a different 
patch (series) and not necessarily one we need to worry about unless somebody 
has an actual use-case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142660

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

[PATCH] D142660: [AIX] supporting -X options for llvm-ranlib in AIX OS

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

clang-format is complaining in the pre-merge CI.




Comment at: clang/test/lit.cfg.py:391
 if "system-aix" in config.available_features:
-config.substitutions.append(("llvm-nm", "env OBJECT_MODE=any llvm-nm"))
-config.substitutions.append(("llvm-ar", "env OBJECT_MODE=any llvm-ar"))
+   config.environment["OBJECT_MODE"] ="any"
 

It might be a good idea for you to run the `black` tool on python changes, to 
ensure they conform to the coding standard, much like you do with clang-format 
for C++ changes.



Comment at: llvm/lib/Object/ArchiveWriter.cpp:901
   uint64_t NumSyms32 = 0; // Store symbol number of 32-bit member files.
+  bool DoesWriteSymtab = WriteSymtab != SymtabWritingMode::NoSymtab;
 

This makes the variable name more grammatically correct.



Comment at: llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp:488
+  Thin ? object::Archive::K_GNU : object::Archive::K_COFF,
+  /*Deterministic*/ true, Thin, nullptr, COFF::isArm64EC(LibMachine))) 
{
 handleAllErrors(std::move(E), [&](const ErrorInfoBase ) {

MaskRay wrote:
> 
Marked as done, but not fully addressed - in this specific style, there should 
be no space between the comment and thing it's referring to. This is a reason 
why clang-format will be failing (there may be others).



Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:74
+ << "  -X{32|64|32_64|any}   - Specify which archive symbol tables "
+"should be generated if they do not already exist (AIX OS only)\n";
 }

Strictly speaking, this should be "Big Archive formats only" not "AIX OS only" 
since theoretically you could have Big Archive archives on other platforms. 
However, I'm not bothered if you don't want to change this. The same probably 
goes for other tools for that matter, but don't change them now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142660

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

[PATCH] D142660: [AIX] supporting -X options for llvm-ranlib in AIX OS

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

In D142660#4541406 , @jhenderson 
wrote:

> In D142660#4538936 , @DiggerLin 
> wrote:
>
>>> As an alternative (but I think adds unnecessary complexity, due to needing 
>>> an extra variable), you could have both tools read the environment variable 
>>> into a string variable, then, if the -X option is present, overwrite that 
>>> variable, and finally feed that string into the parsing code that converts 
>>> into a BitMode value. If the string is invalid, the parsing code could 
>>> report an error along the lines of "invalid OBJECT_MODE or -X option value".
>>
>> if I do not think it is better than my current implement, If I implement as 
>> your suggestion, I need another variable to recoded the where the string 
>> come from(from OBJECT_MODE  or -X option value). otherwise when the invalid 
>> value of string , how can I report it is an invalid OBJECT_MODE"  or 
>> "invalid -X option value"
>
> I'm not convinced you need to distinguish the two. If a user hasn't specified 
> the -X option, then they'll know it's the OBJECT_MODE environment variable. 
> Even if they have both, it should be obvious to a quick glance of the full 
> error message what the wrong value is (because you'd include it in the 
> message) and you could then it won't take long to find which of the two is 
> wrong (even if they don't know that -X trumps OBJET_MODE). That being said, I 
> still prefer the parse OBJECT_MODE first, report an error, then later read 
> the -X option and check for an error there.

It looks like you've ignored this point in your most recent changes. I agree 
that having a variable to say whether the BitMode variable comes from 
OBJECT_MODE or -X is not great, and I'd prefer the `HasAIXOption` style you had 
before over it. However, I'd still rather the invalid OBJECT_MODE value be read 
and rejected upfront before even parsing the -X option.




Comment at: llvm/include/llvm/Object/ArchiveWriter.h:44
+enum SymtabWritingMode {
+  NoSymtab, // Not write Symbol table.
+  NormalSymtab, // Write both 32-bit and 64-bit symbol table.





Comment at: llvm/include/llvm/Object/ArchiveWriter.h:45
+  NoSymtab, // Not write Symbol table.
+  NormalSymtab, // Write both 32-bit and 64-bit symbol table.
+  Bit32Only,// Only write the 32-bit symbol table.

This comment doesn't make sense for non-Big Archive archives, because regular 
archives only have one symbol table. There is no concept of a 32- or 64-bit 
one. Perhaps "Write the symbol table. For the Big Archive format, writes both 
32-bit and 64-bit symbol tables."



Comment at: llvm/include/llvm/Object/ArchiveWriter.h:46-47
+  NormalSymtab, // Write both 32-bit and 64-bit symbol table.
+  Bit32Only,// Only write the 32-bit symbol table.
+  Bit64Only // Only write the 64-bit symbol table.
+};

I'd prefix these two with `BigArchive`, or even just name them `BigArchive32` 
and `BigArchive64` respectively, to more clearly convey the fact that they are 
specific to that file format.



Comment at: llvm/lib/Object/ArchiveWriter.cpp:966
   if (!isAIXBigArchive(Kind)) {
-if (WriteSymtab) {
+if (WriteSymtab != SymtabWritingMode::NoSymtab) {
   if (!HeadersSize)

Where this comparison is repeated more than once in the same function, it might 
be nice to store the value in a local boolean variable for use in all places 
instead of repeating the comparison.



Comment at: llvm/test/tools/llvm-ranlib/non-AIX-not-supportedwq-X-option.test:1
+## REQUIRES: !system-aix
+## Test the -X option is not supported on non-AIX os.

Looks like there's a typo in your test name?



Comment at: llvm/test/tools/llvm-ranlib/non-AIX-not-supportedwq-X-option.test:6
+
+# INVALID-X-OPTION: error: -X32 option not supported on non AIX OS 

Nit: trailing whitespace


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142660

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

[PATCH] D142660: [AIX] supporting -X options for llvm-ranlib in AIX OS

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

In D142660#4538936 , @DiggerLin wrote:

>> As an alternative (but I think adds unnecessary complexity, due to needing 
>> an extra variable), you could have both tools read the environment variable 
>> into a string variable, then, if the -X option is present, overwrite that 
>> variable, and finally feed that string into the parsing code that converts 
>> into a BitMode value. If the string is invalid, the parsing code could 
>> report an error along the lines of "invalid OBJECT_MODE or -X option value".
>
> if I do not think it is better than my current implement, If I implement as 
> your suggestion, I need another variable to recoded the where the string come 
> from(from OBJECT_MODE  or -X option value). otherwise when the invalid value 
> of string , how can I report it is an invalid OBJECT_MODE"  or "invalid -X 
> option value"

I'm not convinced you need to distinguish the two. If a user hasn't specified 
the -X option, then they'll know it's the OBJECT_MODE environment variable. 
Even if they have both, it should be obvious to a quick glance of the full 
error message what the wrong value is (because you'd include it in the message) 
and you could then it won't take long to find which of the two is wrong (even 
if they don't know that -X trumps OBJET_MODE). That being said, I still prefer 
the parse OBJECT_MODE first, report an error, then later read the -X option and 
check for an error there.




Comment at: clang/test/lit.cfg.py:391
+if 'system-aix' in config.available_features:
+   config.environment['OBJECT_MODE'] = '32_64'
 

MaskRay wrote:
> Recent Python style prefers double quotes
This also applies to the line above. See discussions on Discourse about using 
"black" to format python code (it's similar to clang-format, but for python).



Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:17-18
+## Test the OBJECT_MODE environment variable when adding symbol table.
+# RUN: unset OBJECT_MODE
+# RUN: llvm-ranlib t_X32.a
+# RUN: llvm-nm --print-armap t_X32.a 2>&1 | FileCheck --check-prefixes=GLOB32 
--implicit-check-not="in t64" %s

DiggerLin wrote:
> jhenderson wrote:
> > DiggerLin wrote:
> > > jhenderson wrote:
> > > > Assuming the unsetting is intended to be just for the llvm-ranlib line, 
> > > > I believe the preferred approach is `env -u OBJECT_MODE llvm-ranlib 
> > > > ...`. That way, you don't impact the behaviour in subsequent lines.
> > > the command `env` of AIX OS do not support -u.  and there is no mapping  
> > > option of `env -u`
> > > 
> > > https://www.ibm.com/docs/en/aix/7.2?topic=e-env-command
> > I'm 90% certain that `env` here is the built-in lit `env`, not a system 
> > `env` executable. `env -u` seems to be only rarely used in the test suite, 
> > but see llvm/utils/lit/tests/Inputs/shtest-env/env-u.txt for an example. I 
> > assume you can run this test locally?
> it run fail in AIX OS locally.
My apologies, it appears that you must be using the external shell or something 
(see lit differences internal and external shell). That might explain why `env 
-u` isn't used that much.



Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:237
 static bool Verbose = false;  ///< 'v' modifier
-static bool Symtab = true;///< 's' modifier
+static WriteSymTabType Symtab = true; ///< 's' modifier
 static bool Deterministic = true; ///< 'D' and 'U' modifiers

DiggerLin wrote:
> jhenderson wrote:
> > DiggerLin wrote:
> > > jhenderson wrote:
> > > > DiggerLin wrote:
> > > > > jhenderson wrote:
> > > > > > DiggerLin wrote:
> > > > > > > jhenderson wrote:
> > > > > > > > Maybe I'm missing something, but I don't see why you need to 
> > > > > > > > make this a custom type. You already have the BitMode value 
> > > > > > > > that you read in from the environment/command-line, and so you 
> > > > > > > > can just use that in conjunction with the `Symtab` boolean to 
> > > > > > > > determine what symbol table to write, surely?
> > > > > > > the Symtab are use to specify whether the symbol table need to 
> > > > > > > write(for non-AIX). and what the symbol table need to write for 
> > > > > > > AIX OS.
> > > > > > > 
> > > > > > > there are function like
> > > > > > > 
> > > > > > > ```
> > > > > > >   writeArchiveToBuffer(ArrayRef NewMembers,
> > > > > > >  WriteSymTabType WriteSymtab, 
> > > > > > > object::Archive::Kind Kind,
> > > > > > >  bool Deterministic, bool Thin)
> > > > > > > ```
> > > > > > > and 
> > > > > > > 
> > > > > > > ```
> > > > > > > Error writeArchive(StringRef ArcName, ArrayRef 
> > > > > > > NewMembers,
> > > > > > >WriteSymTabType WriteSymtab, 
> > > > > > > object::Archive::Kind Kind,
> > > > > > >bool Deterministic, bool Thin,
> > > > > > >std::unique_ptr 

[PATCH] D142660: [AIX] supporting -X options for llvm-ranlib in AIX OS

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

In D142660#4535693 , @stephenpeckham 
wrote:

> I don't see any reason to check the OBJECT_MODE environment variable if the 
> -X flag is used.  What would the error be:  "You specified a valid -X flag, 
> but by the way, OBJECT_MODE is set to an invalid value"?

The error would be "invalid value for OBJECT_MODE environment variable" (or 
something to that effect), which would mean the user fixes their environment, 
just the same as if they hadn't used -X. I just want to emphasise though that 
my main concern is that llvm-ar and llvm-ranlib are being inconsistent - one of 
them checks the environment variable first, the other checks the command-line 
option first, and this seems wrong - they should do the same thing. By reading 
and checking the environment variable first, it simplifies the code logic (no 
need for `HasAIXXOption` for example).

As an alternative (but I think adds unnecessary complexity, due to needing an 
extra variable), you could have both tools read the environment variable into a 
string variable, then, if the -X option is present, overwrite that variable, 
and finally feed that string into the parsing code that converts into a 
`BitMode` value. If the string is invalid, the parsing code could report an 
error along the lines of "invalid OBJECT_MODE or -X option value".

> I think all the commands that examine XCOFF files (llvm-ar, lllvm-ranlib, 
> llvm-readobj, llvm-objdump, llvm-nm, etc.) should recognize "32", "64", 
> "32_64", and "any".  I don't think it's necessary to recognize "d64", even if 
> AIX commands do.  In addition, I wouldn't bother recognizing an XCOFF file 
> with the magic number for a discontinued 64-bit object.  That means that 
> "32_64" and "any" have the same behavior.   If -X is specified and does not 
> have one of the 4 specified values, a usage message should be printed.  If -X 
> is not specified but OBJECT_MODE is in the environment, a message should be 
> printed if the value is not one of the 4 specified values.

Yes, to be clear, I'm not advocating that "d64" support should be added (I'm 
not opposed to it, should there be a use-case for it either).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142660

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

[PATCH] D142660: [AIX] supporting -X options for llvm-ranlib in AIX OS

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:68
+
+## Test the invalid -X option and OBJECT_MODE enviornment var.
+# RUN: not env OBJECT_MODE= llvm-ranlib t_X32.a 2>&1 | FileCheck 
--implicit-check-not="error:"  --check-prefixes=INVALID-OBJECT-MODE %s

Actually, you shouldn't have added "the" here. The grammatical rules are a 
little tricky to explain, so I won't bother, but you should actually say "Test 
invalid -X options and OBJECT_MODE environment variables."



Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:17-18
+## Test the OBJECT_MODE environment variable when adding symbol table.
+# RUN: unset OBJECT_MODE
+# RUN: llvm-ranlib t_X32.a
+# RUN: llvm-nm --print-armap t_X32.a 2>&1 | FileCheck --check-prefixes=GLOB32 
--implicit-check-not="in t64" %s

DiggerLin wrote:
> jhenderson wrote:
> > Assuming the unsetting is intended to be just for the llvm-ranlib line, I 
> > believe the preferred approach is `env -u OBJECT_MODE llvm-ranlib ...`. 
> > That way, you don't impact the behaviour in subsequent lines.
> the command `env` of AIX OS do not support -u.  and there is no mapping  
> option of `env -u`
> 
> https://www.ibm.com/docs/en/aix/7.2?topic=e-env-command
I'm 90% certain that `env` here is the built-in lit `env`, not a system `env` 
executable. `env -u` seems to be only rarely used in the test suite, but see 
llvm/utils/lit/tests/Inputs/shtest-env/env-u.txt for an example. I assume you 
can run this test locally?



Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1442
+
+// -X option in ranlib do not accept "any"
+if (BitMode == BitModeTy::Unknown || BitMode == BitModeTy::Any)

DiggerLin wrote:
> jhenderson wrote:
> > DiggerLin wrote:
> > > jhenderson wrote:
> > > > I know that AIX ranlib doesn't accept "any" whereas I believe AIX ar 
> > > > does. I wonder though what the benefit is of preventing llvm-ranlib 
> > > > from accepting "any"? Dropping that special case would simplify the 
> > > > code.
> > > agree with you. but we discussed about whether to accept `any` in our 
> > > internal , we decide to keep all the behavior  as AIX `ranlib`
> > > we decide to keep all the behavior as AIX ranlib
> > 
> > We aren't in your internal company here. This is the open source community, 
> > therefore you need to be able to justify your decisions in the open source 
> > conversation. What reason is there to keep rejecting this in llvm-ranlib? 
> > Perhaps worth asking yourself is "if we could control it, why would we keep 
> > that behaviour in AIX ranlib?".
> according to 
> https://www.ibm.com/docs/en/aix/7.1?topic=ar-command
> 
> -X mode , there is `32`, `64`, `32_64`, `d64`, any mode
> 
> d64
> Examines discontinued 64-bit XCOFF files (magic number == U803XTOCMAGIC).
> 
> we do not support `d64`in llvm(since it is discontinued), but we keep `any`  
> option in llvm-ar in case of we want to use llvm-ar to replace AIX `ar`  in 
> some AIX shell script which has option `any` for ar (`any = 32_64 + d64`),  
> we do no want to modify the option from `any` to `32_64` for AIX shell 
> script, so we keep the `any` option for llvm-ar.
> 
> for AIX `ranlib`, https://www.ibm.com/docs/en/aix/7.2?topic=r-ranlib-command 
> . it only support 32,64,32_64, It do not support `d64`, so there is no `any` 
> option for AIX `ranlib`, we do not need to add a additional `any` for 
> llvm-ranlib 
> we do not need to add a additional any for llvm-ranlib

As noted earlier, adding an `any` value would align llvm-ranlib and llvm-ar's 
command-line options, allowing the code to be simpler. It doesn't break 
existing users by permitting it either. You have explained why you aren't 
accepting it, but not actually what the benefit is of that approach. What is 
the benefit of NOT accepting `any` in llvm-ranlib?



Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1463
 
+  if (object::Archive::getDefaultKindForHost() == object::Archive::K_AIXBIG) {
+// If not specify -X option, get BitMode from enviorment variable

DiggerLin wrote:
> jhenderson wrote:
> > DiggerLin wrote:
> > > jhenderson wrote:
> > > > Is there a particular reason that this is after the command-line option 
> > > > parsing for llvm-ranlib, but before it in llvm-ar? If this were before 
> > > > the option parsing, you wouldn't need the `HasAixXOption` variable.
> > > in AIX OS  `ranlib` has behavior as
> > > 
> > > 
> > > ```
> > > -bash-5.0$ env OBJECT_MODE=31 ranlib tmpk.a
> > > 0654-603 The OBJECT_MODE environment variable has an invalid setting.
> > > OBJECT_MODE must be 32, 64, or 32_64.
> > > -bash-5.0$ env OBJECT_MODE=31 ranlib -X32  tmpk.a
> > > -bash-5.0$
> > > ```  
> > > 
> > > Given invalid env OBJECT_MODE , if there is no -X option in the ranlib 
> > > command, it will output as 
> > > 
> > > ```
> > > 0654-603 The OBJECT_MODE environment 

[PATCH] D142660: [AIX] supporting -X options for llvm-ranlib in AIX OS

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:17-18
+## Test the OBJECT_MODE environment variable when adding symbol table.
+# RUN: unset OBJECT_MODE
+# RUN: llvm-ranlib t_X32.a
+# RUN: llvm-nm --print-armap t_X32.a 2>&1 | FileCheck --check-prefixes=GLOB32 
--implicit-check-not="in t64" %s

Assuming the unsetting is intended to be just for the llvm-ranlib line, I 
believe the preferred approach is `env -u OBJECT_MODE llvm-ranlib ...`. That 
way, you don't impact the behaviour in subsequent lines.



Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:31
+
+## Test -X option when adding symbol table.
+# RUN: llvm-ranlib -X32 t_X32.a

jhenderson wrote:
> 
Marked as done but not addressed? Please don't do that...



Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:74-75
+ << "  -X {32|64|32_64}  - Specifies which archive symbol tables "
+"should"
+"be generated if they do not already exist (AIX OS only)\n";
 }

DiggerLin wrote:
> jhenderson wrote:
> > Please reflow this string. That should make it fairly obvious that there's 
> > a mistake in it too (hint: print the help and spot the missing space...).
> thanks
You didn't actually reflow the string. I believe the following is more correct 
(I haven't checked the column width, so make sure to run clang-format 
afterwards:

```
"should be generated if they do not already exist (AIX OS only)\n";
```



Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1442
+
+// -X option in ranlib do not accept "any"
+if (BitMode == BitModeTy::Unknown || BitMode == BitModeTy::Any)

DiggerLin wrote:
> jhenderson wrote:
> > I know that AIX ranlib doesn't accept "any" whereas I believe AIX ar does. 
> > I wonder though what the benefit is of preventing llvm-ranlib from 
> > accepting "any"? Dropping that special case would simplify the code.
> agree with you. but we discussed about whether to accept `any` in our 
> internal , we decide to keep all the behavior  as AIX `ranlib`
> we decide to keep all the behavior as AIX ranlib

We aren't in your internal company here. This is the open source community, 
therefore you need to be able to justify your decisions in the open source 
conversation. What reason is there to keep rejecting this in llvm-ranlib? 
Perhaps worth asking yourself is "if we could control it, why would we keep 
that behaviour in AIX ranlib?".



Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1463
 
+  if (object::Archive::getDefaultKindForHost() == object::Archive::K_AIXBIG) {
+// If not specify -X option, get BitMode from enviorment variable

DiggerLin wrote:
> jhenderson wrote:
> > Is there a particular reason that this is after the command-line option 
> > parsing for llvm-ranlib, but before it in llvm-ar? If this were before the 
> > option parsing, you wouldn't need the `HasAixXOption` variable.
> in AIX OS  `ranlib` has behavior as
> 
> 
> ```
> -bash-5.0$ env OBJECT_MODE=31 ranlib tmpk.a
> 0654-603 The OBJECT_MODE environment variable has an invalid setting.
> OBJECT_MODE must be 32, 64, or 32_64.
> -bash-5.0$ env OBJECT_MODE=31 ranlib -X32  tmpk.a
> -bash-5.0$
> ```  
> 
> Given invalid env OBJECT_MODE , if there is no -X option in the ranlib 
> command, it will output as 
> 
> ```
> 0654-603 The OBJECT_MODE environment variable has an invalid setting.
> OBJECT_MODE must be 32, 64, or 32_64.
> ```
> 
> Given invalid env OBJECT_MODE , and there is -X option in the ranlib command, 
>  it do not care about the invalid env OBJECT_MODE,  So I has to parse the -X 
> option before the getBitMode(getenv("OBJECT_MODE"))
> 
So with AIX ar, does an invalid OBJECT_MODE environment variable get reported 
if the -X option is specified?

In my opinion, I think it is very unlikely there will be any real users out 
there with an invalid OBJECT_MODE environment variable, because other tools 
will reject it, even if ranlib doesn't. Even if there are, they should be able 
to easily fix their variable, if they start getting an error message after 
switching to llvm-ranlib. I'm therefore thinking that there isn't a need to 
mirror this logic in llvm-ranlib, if it would make the code simpler (which it 
would).



Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:237
 static bool Verbose = false;  ///< 'v' modifier
-static bool Symtab = true;///< 's' modifier
+static WriteSymTabType Symtab = true; ///< 's' modifier
 static bool Deterministic = true; ///< 'D' and 'U' modifiers

DiggerLin wrote:
> jhenderson wrote:
> > DiggerLin wrote:
> > > jhenderson wrote:
> > > > Maybe I'm missing something, but I don't see why you need to make this 
> > > > a custom type. You already have the BitMode value that you read in from 
> > > > the 

[PATCH] D142660: [AIX] supporting -X options for llvm-ranlib in AIX OS

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:3
+## Test the -X option.
+## The option specifies the type of object file on which llvm-ranlib will 
operate. 
+

Nit: trailing whitespace



Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1394
   object::Archive::K_AIXBIG) {
 Match = *(*ArgIt + 2) != '\0' ? *ArgIt + 2 : *(++ArgIt);
 BitMode = getBitMode(Match);

Unrelated to this patch, but I spotted this when comparing the llvm-ar and 
llvm-ranlib parsing logic: what happens if -X is the very final argument, 
without a value? E.g. `llvm-ar (any other args...) -X`?



Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1437
+const char *Xarg = arg.data();
+if (Xarg[0] == '\0')
+  BitMode = BitModeTy::Unknown;

If I'm reading this correctly, llvm-ranlib will accept "-X32" but reject "-X 
32". Is that intentional? If so, what's the motivation behind it (I would say 
that there needs to be a motivation other than "that's what AIX ranlib does)?



Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1442
+
+// -X option in ranlib do not accept "any"
+if (BitMode == BitModeTy::Unknown || BitMode == BitModeTy::Any)

I know that AIX ranlib doesn't accept "any" whereas I believe AIX ar does. I 
wonder though what the benefit is of preventing llvm-ranlib from accepting 
"any"? Dropping that special case would simplify the code.



Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1463
 
+  if (object::Archive::getDefaultKindForHost() == object::Archive::K_AIXBIG) {
+// If not specify -X option, get BitMode from enviorment variable

Is there a particular reason that this is after the command-line option parsing 
for llvm-ranlib, but before it in llvm-ar? If this were before the option 
parsing, you wouldn't need the `HasAixXOption` variable.



Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1469-1471
+  if (BitMode == BitModeTy::Any || BitMode == BitModeTy::Unknown)
+fail("The OBJECT_MODE environment variable has an invalid setting. "
+ "OBJECT_MODE must be 32, 64, or 32_64.");

This is an inconsistency with llvm-ar's current behaviour: llvm-ar treats 
`Unknown` as 32, whereas here you're outright rejecting it. I'm not convinced 
this inconsistency makes sense, nor do I see it benefiting the user. In my 
opinion the two should do the same thing. I think a garbage string should be 
outright rejected in both cases. An empty string for the environment variable 
or command-line option should probably be rejected too, but I'm okay with it 
being accepted, if AIX ranlib behaves that way. However, I think llvm-ranlib 
and llvm-ar should do the same whatever that is.

Once these inconsistencies have been ironed out, I think it'll be a lot simpler 
to share code between the two tools.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142660

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

[PATCH] D142660: [AIX] supporting -X options for llvm-ranlib in AIX OS

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

My apologies, this patch fell off my review queue somehow.

I still have concerns about the option-parsing logic being duplicated, but I'm 
out of time to review it now. I'll try to look tomorrow.




Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:16
+
+## Test OBJECT_MODE environment variable when adding symbol table
+# RUN: env OBJECT_MODE= llvm-ranlib t_X32.a





Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:17
+## Test OBJECT_MODE environment variable when adding symbol table
+# RUN: env OBJECT_MODE= llvm-ranlib t_X32.a
+# RUN: env OBJECT_MODE=32 llvm-ranlib t_X32.a

Doesn't this line want an llvm-nm check after it, like the others?



Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:31
+
+## Test -X option when adding symbol table.
+# RUN: llvm-ranlib -X32 t_X32.a





Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:48
+
+## Test -X option will override the "OBJECT_MODE" environment variable.
+# RUN: env OBJECT_MODE=32_64 llvm-ranlib -X32 t_X32.a





Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:55
+
+## Test invalid -X option and invalid OBJECT_MODE
+# RUN: not env OBJECT_MODE=any llvm-ranlib t_X64.a 2>&1 | FileCheck 
--implicit-check-not="error:"  --check-prefixes=INVALID-OBJECT-MODE %s

It would be better to move this block of test cases below the check patterns 
that are used for the valid cases.



Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:62
+
+#GLOB32:  var_0x1DF in t32_1.o
+#GLOB32-NEXT: array_0x1DF in t32_1.o

Nit: here and throughout - check patterns usually have a space between '#' and 
the pattern name, e.g. "# GLOB32:"

Rather than the somewhat confusing GL64 versus GLOB64 (and similiarly for the 
32-bit case), how about "HAS64" and "NO64" (and "HAS32" and "NO32")? That being 
said, see my comment below about ordering...



Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:67
+
+#GL64-NOT:var_0x1F7 in t64_1.o
+#GL64-NOT:array_0x1F7 in t64_1.o

I'm concerned you're going to have an ordering issue in one of the 64 bit or 32 
bit "NOT" cases. FileCheck -NOT patterns only check the region between the 
previous non-NOT pattern before the -NOT one up to the next non-NOT pattern (or 
the end of the file). So, as things stand, --check-prefixes=GLOB32,GL64 will 
verify that the 32-bit table appears and then no 64-bit table is printed AFTER 
the 32-bit table, but not whether the 64-bit table appears BEFORE the 32-bit 
table. Similarly, --check-prefixes=GLOB64,GL32 will check that the 64-bit table 
is printed and then no 32-bit table is printed after it, but not whether the 
32-bit table is printed before it.

Given that the names of the symbols are irrelevant to this test case, and 
really all that's interesting is which object they're in (since this test isn't 
about checking that the symbol tables have the correct contents - that is the 
duty of a test that doesn't make use of the -X option), could you simplify the 
input objects to have a single like-named symbol and then simply ensure the 
correct object names appear in the output? You could use --implicit-check-not 
to check that e.g. "in t64" or "in t32" don't appear in your output.

If you adopted this approach, you'd have two CHECK lines as e.g. follows:
```
# GLOB32: symbol1 in t32_1.o
# GLOB32-NEXT: symbol2 in t32_2.o

# GLOB64: symbol1 in t64_1.o
# GLOB64-NEXT: symbol2 in t64_2.o
```
And you'd have FileCheck lines that look like one of the following:
```
FileCheck --check-prefixes=GLOB32 --implicit-check-not="in t64"
FileCheck --check-prefixes=GLOB64 --implicit-check-not="in t32"
FileCheck --check-prefixes=GLOB32,GLOB64
```

By simplifying down to one symbol each, you could also pass the symbol name in 
as a yaml2obj -D value and only have one YAML file in the test.



Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:82-85
+#GLOB1:  var_0x1DF in t32_1.o
+#GLOB1-NEXT: array_0x1DF in t32_1.o
+#GLOB1-NEXT: var_0x1F7 in t64_1.o
+#GLOB1-NEXT: array_0x1F7 in t64_1.o

These aren't used. Delete them.



Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:87
+
+#INVALID-OBJECT-MODE: error: The OBJECT_MODE environment variable has an 
invalid setting. OBJECT_MODE must be 32, 64, or 32_64.
+#INVALID-X-OPTION: error: The specified object mode is not valid. Specify 
-X32, -X64, or -X32_64.

"value" would be better than "setting" (environment variables have values, not 
settings).

Please also re-review the coding standards regarding error messages and fix the 
two separate issues in this and the below error message.



Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:74-75
+ << "  -X {32|64|32_64}  - 

[PATCH] D153652: [Support] Don't set "all_exe" mode by default for file written by llvm::writeToOutput

[James Henderson via Phabricator via cfe-commits]

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

The new test LGTM, albeit with one query: I assume `umask` sets some global 
state. When the lit unit tests are running, do the tests within the same 
process run sequentially, or are they in parallel at all? If the latter, there 
could be some thread-safety issue, although I suspect the tests are run 
sequentially.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153652

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

[PATCH] D153652: [Support] Don't set "all_exe" mode by default for file written by llvm::writeToOutput

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: llvm/unittests/Support/raw_ostream_test.cpp:500
+  ASSERT_TRUE(!!Perms);
+  EXPECT_EQ(0, *Perms & llvm::sys::fs::all_exe);
+

hokein wrote:
> jhenderson wrote:
> > Here and below, rather than just checking the all_exe bit, let's check the 
> > permissions are exactly what are expected (e.g. does it have the read/write 
> > perms?). 
> checking all existing bits is a bit tricky here (I tried it, then gave up):
> 
> - createTemporaryFile() creates a file with `owner_read | owner_write`
> - writeToOutput() sets the written file to `all_read | all_write`
> 
> Both API don't provide a way to customize these bits, and they're internal 
> details. We could test against them, but testing the implementation details 
> seems subtle. And here we aim to verify the exe-bit not set by the 
> `writeToOutput`, so I think just testing the exe-bit is not set should be 
> enough. 
This argument doesn't make much sense to me. Why are the `all_read` and 
`all_write` bits implementation details that shouldn't be tested when the lack 
of `all_exe` is?

This test is for testing `writeToOutput`. Part of `writeToOutput`'s behaviour 
appears to be to create a file with the `all_read` and `all_write` bits set. 
Therefore, we should be testing that behaviour. As there was already one issue 
with the permission bits of the file this method creates, and you are directly 
modifiyng a test to add permissions testing, I think it's justified to request 
testing of the other bits.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153652

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

[PATCH] D153652: [Support] Don't set "all_exe" mode by default for file written by llvm::writeToOutput

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

The updated unit test is failing on Windows in the pre-merge checks. Please 
investigate and fix as appropriate.




Comment at: llvm/unittests/Support/raw_ostream_test.cpp:500
+  ASSERT_TRUE(!!Perms);
+  EXPECT_EQ(0, *Perms & llvm::sys::fs::all_exe);
+

Here and below, rather than just checking the all_exe bit, let's check the 
permissions are exactly what are expected (e.g. does it have the read/write 
perms?). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153652

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

[PATCH] D153652: [Support] Don't set "all_exe" mode by default for file written by llvm::writeToOutput

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: llvm/test/tools/llvm-dwarfutil/ELF/X86/file-permissions.test:1
+# RUN: yaml2obj %p/Inputs/common.yaml -o %t.o
+# RUN: chmod a+x %t.o

hokein wrote:
> jhenderson wrote:
> > It might make sense to reuse the test code from llvm-objcopy's 
> > mirror-permissions tests. See my other inline comment too.
> by reusing the code, I think you mean calling the `llvm-dwardfutil` tool in 
> the `llvm-objcopy` lit test, I'm not sure it is a good idea, calling a 
> different tool in the 
> `llvm/test/tools/llvm-objcopy/mirror-permissions-*.test` seems like violating 
> the layer.
I meant that you could copy and adapt the existing llvm-objcopy test to create 
a new llvm-dwarfutil test (in the llvm-dwarfutil test folder).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153652

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

[PATCH] D153652: [llvm][Support] Don'tt set "all_exe" mode by default for file written by llvm::writeToOutput.

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

Is there anything that can be done to use gtest unit tests for this? The two 
lit tests are useful, but the problem with them is that if they switch to using 
a different approach to emitting their data, the lit tests won't cover the 
Support code you're actually changing.

Some commit message nits:

> [llvm][Support] Don'tt set "all_exe" mode by default for file written by 
> llvm::writeToOutput.

There's no need to include the `[llvm]` tag - the `[Support]` tag already 
indicates the library clearly enough IMHO. Also, typo "Don'tt" -> "Don't" and 
we don't usually end commit titles with a ".".

Does the clang include cleaner tool have testing that covers its usage of this 
API?




Comment at: llvm/test/tools/llvm-dwarfutil/ELF/X86/file-permissions.test:1
+# RUN: yaml2obj %p/Inputs/common.yaml -o %t.o
+# RUN: chmod a+x %t.o

It might make sense to reuse the test code from llvm-objcopy's 
mirror-permissions tests. See my other inline comment too.



Comment at: llvm/test/tools/llvm-objcopy/ELF/file-permissions.test:1
+# RUN: cp %p/Inputs/dwarf.dwo %t-exe.dwo
+# RUN: chmod a+x %t-exe.dwo

llvm-objcopy already has testing for permissions - see 
mirror-permissions-*.test and respect-umask.test. It seems like a new test file 
would be a mistake. Furthermore, a casual inspection to me makes it seem like 
this behaviour is already covered (and working correctly), which makes me think 
that no new testing is needed for this tool, but please review the existing 
test and see what you think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153652

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

[PATCH] D142660: [AIX] supporting -X options for llvm-ranlib in AIX OS

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

I haven't looked again at the rest of this patch. I'll do so hopefully in the 
next couple of weeks.




Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:80
+ << "  -U- Use actual timestamps and uids/gids\n"
+ << "  -X {32|64|32_64}  - Specifies the type of object files"
+"llvm-ranlib should examine (AIX OS only)\n";

DiggerLin wrote:
> stephenpeckham wrote:
> > I think the AIX documentation for ranlib isn't as helpful as it could be. I 
> > actually like a variation of the original message better:
> > 
> > "-X {32|64|32_64}  - Specifies which archive symbol tables should be 
> > generated if they do not already exist (AIX OS only)\n"
> > 
> > This implies that a 32-bit (64-bit) global symbol table is generated by 
> > examining XCOFF32 (XCOFF64) members.
> > 
> > But this wording doesn't really fit with the command description: Generate 
> > an //index// for archives. Should this be "Generate an index or symbol 
> > tables for archives"? Or just "Generate symbol tables for archives"?  The 
> > usage message for llvm-ar also mixes "index" and "symbol table"
> I think the llvm-ranlib generates the global symbol table, index is to 
> general.  if we want to change the description,  Maybe the "Generate symbol 
> tables for archives" is better and we should create a separate patch for it.  
> what do you think.@jhenderson 
So a quick look at GNU ranlib shows it uses the term "index". This makes a 
degree of sense: the "symbol table" is simply a map between symbol name and 
which archive member it is in, much like at the end of a book is from topics to 
pages. In other words "index" and "symbol table" mean the same thing in this 
context and can be used interchangeably.

Given that GNU uses the "index" terminology, I don't think we should change the 
main llvm-ranlib description. I'm happy for "symbol table" or "index" to be 
used in the help text here for this option.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142660

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

[PATCH] D149119: [CMake] Use LLVM own tools in extract_symbols.py

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

In D149119#4331207 , @ikudrin wrote:

> In D149119#4329274 , @tmatheson 
> wrote:
>
>> LGTM, thank you for doing this. Please give it a couple of days in case 
>> others have comments.
>
> Thanks!
>
> In D149119#4329285 , @jhenderson 
> wrote:
>
>> I've not really looked into this patch significantly, so this may well be 
>> addressed in the patch, given I see you have modified stuff to do with the 
>> NATIVE build, but in the past I have seen LLVM using its own tools to build 
>> other parts of its system. I believe it was using llvm-nm to extract the 
>> list of symbols needed for export, possibly to do with part of the clang 
>> build, possibly even using this script, I don't remember. The problem was 
>> that it was using the just-built version of llvm-nm, rather than 
>> specifically one from a release build. On a debug build this caused 
>> particularly slow builds for me, so much so that I stopped building the 
>> relevant parts of LLVM. Please don't introduce a similar situation/make the 
>> situation worse (it's quite possible this was fixed some time ago, but I 
>> haven't tried recently, nor do I remember the exact thing causing the 
>> issue): much like tablegen, any parts of the LLVM build that use just-built 
>> tools should make use of release builds, even in debug configuration, at 
>> least if an appropriate cmake option is specified.
>
> Your concerns are legit, but the tools in this patch follow the same 
> principle as `TableGen`, i.e. if `LLVM_OPTIMIZED_TABLEGEN` is `ON` then the 
> tools are forced to be built with optimization.

Thanks - that's fine with me (though raises the question as to whether we 
should be renaming that variable at some point...).


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

https://reviews.llvm.org/D149119

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

[PATCH] D149119: [CMake] Use LLVM own tools in extract_symbols.py

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

I've not really looked into this patch significantly, so this may well be 
addressed in the patch, given I see you have modified stuff to do with the 
NATIVE build, but in the past I have seen LLVM using its own tools to build 
other parts of its system. I believe it was using llvm-nm to extract the list 
of symbols needed for export, possibly to do with part of the clang build, 
possibly even using this script, I don't remember. The problem was that it was 
using the just-built version of llvm-nm, rather than specifically one from a 
release build. On a debug build this caused particularly slow builds for me, so 
much so that I stopped building the relevant parts of LLVM. Please don't 
introduce a similar situation/make the situation worse (it's quite possible 
this was fixed some time ago, but I haven't tried recently, nor do I remember 
the exact thing causing the issue): much like tablegen, any parts of the LLVM 
build that use just-built tools should make use of release builds, even in 
debug configuration, at least if an appropriate cmake option is specified.


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

https://reviews.llvm.org/D149119

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

[PATCH] D144638: [lit] Detect Inconsistent File Access Times

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

Related to my inline comment, your changes will result in some tests being 
disabled on Windows that weren't before (at least one of the tests pass for me 
even on my machine where atime is disabled). I think we need to understand why 
these tests pass on Windows before losing test coverage by disabling them.




Comment at: llvm/utils/lit/lit/llvm/config.py:172
+# Windows: the last access time is disabled by default in the OS, and
+# the check below is written in terms of unix utilities (touch, ls),
+# which will not work on this platform.

As mentioned, I don't think this is a fair statement: many tests use `touch` or 
`ls`, and work fine on Windows. That being said, the top Google result for how 
to detect if access time is enabled on Windows yields the following command: 
`fsutil behavior query disablelastaccess`, which prints some information about 
its state that could be easily queried in python to give a more correct answer 
for Windows.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144638

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

[PATCH] D144638: [lit] Detect Inconsistent File Access Times

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: llvm/utils/lit/lit/llvm/config.py:171
+# in the tests that do the same thing.
+(_, try_touch_err) = self.get_process_output(["touch", "-a", "-t", 
"199505050555.55", f.name])
+if try_touch_err != "":

lenary wrote:
> michaelplatings wrote:
> > lenary wrote:
> > > michaelplatings wrote:
> > > > It looks like this command will be run on Windows. I think it will fail 
> > > > and cause False to be returned, which is the desired result, but this 
> > > > appears to be by accident rather than design. Therefore I'm inclined to 
> > > > agree with @int3 that a hard-coded check would be preferable.
> > > I am going to add the hardcoded checks, but I think `touch` is available 
> > > in windows, it should be in the same directory as all the git binaries.
> > This is definitely tangential to the change, but in case it's useful to 
> > know: conventionally only `C:\Program Files\Git\bin` is added to the path 
> > on Windows, not `C:\Program Files\Git\usr\bin`. `C:\Program Files\Git\bin` 
> > only contains `bash.exe`, `git.exe` and `sh.exe`.
> Something weirder is happening in `_find_git_windows_unix_tools` (from 
> https://reviews.llvm.org/D84380), but I think it's probably right just to 
> early exit with false on Windows.
I've got no objection to explicitly omitting Windows, but just wanted to point 
out that LLVM requires various Unix-like tools to be available according to 
https://llvm.org/docs/GettingStarted.html#software. Whilst `touch` isn't 
explicitly specified, I'd be surprised if you managed to get access to that 
explicit list without including `touch` inadvertently (it's also worth noting 
that there are several other tests that use `touch` and other utilities that 
are intended to work on Windows). I thought that `Git\usr\bin` was recommended 
to be included in people's paths for Windows, as the way to get access to these 
tools, but I couldn't find that recommendation, so maybe that's just our 
downstream recommendation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144638

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

[PATCH] D144638: [lit] Detect Consistent File Access Times

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

This looks reasonable to me, with the caveat that I don't know a huge amount 
about how the different OSes access time systems work. One question though: if 
your antivirus was causing flakiness (as opposed to outright always-fails), 
won't it just move that flakiness into whether the REQUIRES calculation returns 
true or not (i.e. it could spuriously do so, causing the tests to be enabled 
but then potential still be flaky?




Comment at: llvm/utils/lit/lit/llvm/config.py:165
+#
+# This check hopefully detects both cases, and disables tests that require
+# consistent atime.

Is "hopefully" really needed here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144638

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

[PATCH] D143725: [llvm-objdump][ARM] support --symbolize-operands for ARM/ELF

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1145
+  // So far only supports ARM/Thumb, PowerPC and X86.
+  Triple triple = STI->getTargetTriple();
+  if (!triple.isPPC() && !triple.isX86() && !triple.isARM() &&

covanam wrote:
> Esme wrote:
> > Please capitalize the first letter of variable names.
> Sorry I am newcomer. clang-format didn't say anything, so I thought this is 
> okay.
> Will be fixed. Same for the other one.
clang-format is only for formatting. It won't do things like variable name 
styles. You may want to look into clang-tidy which does a lot more. Also, make 
sure you've fully read and digested the [[ 
https://llvm.org/docs/CodingStandards.html | LLVM coding standards ]].

FWIW, here you can't simply do `triple` -> `Triple`, since `Triple` is a type. 
You probably can use `TargetTriple` or something along those lines.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143725

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

[PATCH] D137338: Fix dupe word typos

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.
Herald added a reviewer: jdoerfert.
Herald added subscribers: Michael137, sstefan1, JDevlieghere.

This is going to be impossible to cleanly review as-is. Could it be broken into 
lots of smaller chunks?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137338

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

[PATCH] D136796: [llvm-objdump][Offload] Use common offload extraction method

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

In D136796#3903393 , @jhuber6 wrote:

> Is this good to land now?

The LLVM community practice is to wait a week between pings unless there's 
something urgent (and if something is urgent, please say so).

Aside from the clang bit of the patch, this seems good to me, but it would be 
worth another person involved with offloading to give the final thumbs up.




Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:1262-1265
+if (identify_magic((*BufferOrErr)->getBuffer()) ==
+file_magic::elf_shared_object)
+  continue;
+

This change seems not really part of the patch, since it's touching `clang` but 
the patch is for `llvm-objdump`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136796

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

[PATCH] D136796: [llvm-objdump][Offload] Use common offload extraction method

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: llvm/test/tools/llvm-objdump/Offloading/elf_dynamic.test:1
-## Check that we can dump an offloading binary directly.
-# RUN: yaml2obj %S/Inputs/binary.yaml -o %t.bin
-# RUN: llvm-objdump --offloading %t.bin | FileCheck %s --match-full-lines 
--strict-whitespace --implicit-check-not={{.}}
-
 ## Check that we can dump an offloading binary inside of an ELF section.
+# RUN: yaml2obj %S/Inputs/binary.yaml -o %t.bin

Rather than duplicating this and the ET_REL cases, you can use yaml2obj's -D 
option to parameterise the ELF type. Rough code:

```
# RUN: yaml2obj %S/Inputs/binary.yaml -D TYPE=ET_DYN -o %t.so
# RUN: yaml2obj %S/Inputs/binary.yaml -D TYPE=ET_EXEC -o %t.bin
# RUN: yaml2obj %S/Inputs/binary.yaml -D TYPE=ET_REL -o %t.o

...
  Data: ELFDATA2LSB
  Type: [[TYPE]]
...
```



Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:63
 
-// Print out all the binaries that are contained in this buffer. If we fail
-// to parse a binary before reaching the end of the buffer emit a warning.
-if (Error Err = visitAllBinaries(Binary))
-  reportWarning("while parsing offloading files: " +
-toString(std::move(Err)),
-O.getFileName());
-  }
+  // Print out all the binaries that are contained at this buffer.
+  for (uint64_t I = 0, E = Binaries.size(); I != E; ++I)

Not sure why this was changed.



Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:76
+
+  // Print out all the binaries that are contained at this buffer.
+  for (uint64_t I = 0, E = Binaries.size(); I != E; ++I)

Should this be "contained in this buffer" rather than "at"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136796

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

[PATCH] D136796: [llvm-objdump][Offload] Use common offload extraction method

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

As a general rule, dumping tools should avoid hard errors, because usually they 
prevent the dumping tool from continuing to dump other useful and valid 
information as part of the same execution. I don't have a strong objection in 
this particular case, because I presume this error could be avoided by simply 
not dumping the offloading sections, but I wanted to make the context clear.




Comment at: llvm/lib/Object/OffloadBinary.cpp:261-263
+  case file_magic::elf_relocatable:
+  case file_magic::elf_executable:
+  case file_magic::elf_shared_object: {

Is there test code showing the ET_EXEC and ET_DYN objects work here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136796

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

[PATCH] D135067: [lit] RUN commands without stdin.

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: llvm/utils/lit/tests/Inputs/shtest-stdin/print-stdin.py:6
+print(sys.stdin.read())
\ No newline at end of file


Nit: add newline at EOF.



Comment at: llvm/utils/lit/tests/shtest-stdin.py:28
+# CHECK-PIPE: foobar
+

Nit: Don't have multiple \n at EOF (there should be exactly one).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135067

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

[PATCH] D134284: [AIX] change "llvm-nm" to "env OBJECT_MODE=any llvm-nm" in clang/test for AIX OS

[James Henderson via Phabricator via cfe-commits]

jhenderson accepted this revision.
jhenderson added a comment.

LGTM, with nit.




Comment at: clang/test/lit.cfg.py:287
+
+# llvm-nm tools support an environment variable "OBJECT_MODE" on AIX OS, which
+# controls the kind of objects they will support. If there is no "OBJECT_MODE"




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134284

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

[PATCH] D134284: [AIX] change the clang tests with llvm-nm -Xany

[James Henderson via Phabricator via cfe-commits]

jhenderson added a subscriber: jasonliu.
jhenderson added a comment.

In D134284#3802793 , @DiggerLin wrote:

> In D134284#3802791 , @jhenderson 
> wrote:
>
>> In D134284#3802766 , @DiggerLin 
>> wrote:
>>
>>> In D134284#3802763 , @jhenderson 
>>> wrote:
>>>
 Wouldn't it be better to change the lit config to specify the 
 `OBJECT_MODE` environment variable on AIX?
>>>
>>> I have tried it before, I added the following in clang/test/lit.cfg.py
>>>
>>>   if 'system-aix' in config.available_features:
>>>config.environment['OBJECT_MODE'] = 'any' 
>>>
>>> it will cause  clang some problem in some test cases. something like 
>>> ,"clang-16: error: OBJECT_MODE setting any is not recognized and is not a 
>>> valid setting"
>>
>> Not sure I entirely follow. If `OBJECT_MODE` isn't an environment variable 
>> that clang uses, surely it should ignore it? What clang tests throw this 
>> sort of error?
>
> for example: clang/test/Parser/parser_overflow.c
>
>   Input was:
> 1: clang-16: error: OBJECT_MODE setting any is not recognized and is 
> not a valid setting

It looks to me like 
https://github.com/llvm/llvm-project/blob/b982ba2a6e0f11340b4e75d1d4eba9ff62a81df7/clang/lib/Driver/Driver.cpp#L554
 should be modified to accept the OBJECT_MODE values you've implemented for 
llvm-nm and llvm-ar. Otherwise, you'll never be able to use the `any` and 
`32_64` values supported by those tools as full environment variables (as 
opposed to variables set for a single or limited set of commands) on a system 
where clang is also used.

I'm neither a clang nor an AIX developer though, so my opinion is based only on 
what I've been reviewing in the llvm tools so far. Pinging @daltenty who 
implemented the functionality (see D82476 ) 
and @hubert.reinterpretcast and @jasonliu who reviewed it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134284

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

[PATCH] D134284: [AIX] change the clang tests with llvm-nm -Xany

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

In D134284#3802766 , @DiggerLin wrote:

> In D134284#3802763 , @jhenderson 
> wrote:
>
>> Wouldn't it be better to change the lit config to specify the `OBJECT_MODE` 
>> environment variable on AIX?
>
> I have tried it before, I added the following in clang/test/lit.cfg.py
>
>   if 'system-aix' in config.available_features:
>config.environment['OBJECT_MODE'] = 'any' 
>
> it will cause  clang some problem in some test cases. something like 
> ,"clang-16: error: OBJECT_MODE setting any is not recognized and is not a 
> valid setting"

Not sure I entirely follow. If `OBJECT_MODE` isn't an environment variable that 
clang uses, surely it should ignore it? What clang tests throw this sort of 
error?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134284

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

[PATCH] D134284: [AIX] change the clang tests with llvm-nm -Xany

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

Wouldn't it be better to change the lit config to specify the `OBJECT_MODE` 
environment variable on AIX?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134284

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

[PATCH] D130689: [LLVM] Update C++ standard to 17

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

FWIW, I've got a Visual Studio configuration (admittedly using a direct Visual 
Studio generator, not ninja), and I don't have any issues - C++17 is being 
used. However, I currently only have LLD as an additional project enabled, and 
don't build compiler-rt.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130689

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

[PATCH] D130689: [LLVM] Update C++ standard to 17

[James Henderson via Phabricator via cfe-commits]

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

In D130689#3684330 , @mehdi_amini 
wrote:

> Nice, LGTM, thanks for driving this!
>
>> Remember that if we want to adopt some new feature in a bigger way it should 
>> be discussed and added to the CodingStandard document.
>
> What does it mean exactly? We can't use **anything** C++17 without writing it 
> in the coding standards?
> I'm not sure it'll be manageable: how do you see this playing out?

Based on the release note, I don't think that was what was intended, although I 
am curious to understand what //was// intended!

Looks good to me anyway (but get more buy-in, perhaps a new Discourse post too, 
not just an update on the existing thread, since new threads get emailed out to 
more people).




Comment at: llvm/cmake/modules/CheckCompilerVersion.cmake:20
 
 # Map the above GCC versions to dates: 
https://gcc.gnu.org/develop.html#timeline
+set(LIBSTDCXX_MIN 7)

This comment seems out-of-date now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130689

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

[PATCH] D130161: Added command-line options to exclude functions from function instrumentation output.

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

In D130161#3665527 , @jlegare wrote:

> @jhenderson Apologies, I'm new to the process here.

Usually, you should look at who has recently modified the touched files, or 
reviewed those same files, and add them as reviewers. Often, it's a good idea 
to add several people as reviewers at once, since not everybody has the time to 
review everything. You should also look to see if there's a code owner for the 
impacted code. There is some documentation on finding reviewers here: 
https://llvm.org/docs/Phabricator.html#finding-potential-reviewers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130161

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

[PATCH] D130161: Added command-line options to exclude functions from function instrumentation output.

[James Henderson via Phabricator via cfe-commits]

jhenderson resigned from this revision.
jhenderson added a comment.

I'm not sure why I've been added as a reviewer, as I don't develop or review 
things within clang...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130161

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

[PATCH] D128612: RISC-V big-endian support implementation

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

Objcopy aspects look good, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128612

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

[PATCH] D128667: [WIP] Add Zstd ELF support

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: llvm/lib/ObjCopy/ELF/ELFObject.cpp:567-568
+compression::zstd::compress(
+StringRef(reinterpret_cast(OriginalData.data()),
+  OriginalData.size()),
+CompressedData);

This StringRef construction is identical in both parts of the if, so should be 
pulled out of the if statement into a local variable.



Comment at: llvm/tools/llvm-objcopy/ObjcopyOptions.cpp:1009-1019
+  if (Config.CompressionType != DebugCompressionType::Zstd) {
+if (Config.DecompressDebugSections && !compression::zlib::isAvailable())
+  return createStringError(
+  errc::invalid_argument,
+  "LLVM was not compiled with LLVM_ENABLE_ZLIB: cannot decompress");
+  } else {
+if (Config.DecompressDebugSections && !compression::zstd::isAvailable())

This is identical (I think?) to a block further up in the file. Could we pull 
it out into a helper function, please? Something like the following:

```
Error checkCompressionAvailability() {
   /* moved code here */
}

// Usage would look like:
if (Error err = checkCompressionAvailability())
  return err;
```


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

https://reviews.llvm.org/D128667

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

[PATCH] D128612: RISC-V big-endian support implementation

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: llvm/include/llvm/ADT/Triple.h:864
 
   /// Tests whether the target is RISC-V (32- and 64-bit).
   bool isRISCV() const {

Perhaps worth updating to mention big and little endian here, like `isPPC64` 
above?



Comment at: llvm/tools/llvm-objcopy/ObjcopyOptions.cpp:304-305
 {"elf64-littleriscv", {ELF::EM_RISCV, true, true}},
+{"elf32-bigriscv", {ELF::EM_RISCV, false, false}},
+{"elf64-bigriscv", {ELF::EM_RISCV, true, false}},
 // PowerPC

We need llvm-objcopy testing for these new targets.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128612

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

[PATCH] D125604: [FileCheck] Catch missspelled directives.

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

Code change looks good to me. Are the TODO cases where the test fails if 
changing them?




Comment at: llvm/test/FileCheck/missspelled-directive.txt:18
+
+P4_COUNT-2: foo
+CHECK4: error: misspelled directive 'P4_COUNT-2:'

What about `P4-COUNT_2`? Is that case not really related?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125604

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

[PATCH] D123538: [symbolizer] Parse DW_TAG_variable DIs to show line info for globals

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

Okay, nothing else from me, but @dblaikie or another debuginfo person should 
review it too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123538

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

[PATCH] D123538: [symbolizer] Parse DW_TAG_variable DIs to show line info for globals

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: llvm/test/tools/llvm-symbolizer/data-location.yaml:46
+## symbolized correctly. In future (if D123534 gets merged), this can be 
updated
+## to include a check that llvm-symbolize can also symbolize constant strings,
+## like `const char* string = "123456"` (and &"123456" should be symbolizable)

Nit


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123538

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

[PATCH] D123957: Update the developer policy to mention release notes

[James Henderson via Phabricator via cfe-commits]

jhenderson accepted this revision.
jhenderson added a comment.

LGTM, with suggested nits.




Comment at: llvm/docs/DeveloperPolicy.rst:188
+notes, typically found in ``docs/ReleaseNotes.rst`` for the project. Changes to
+a project that are user-facing or users may wish to know about should be added
+to the project's release notes. Examples of changes that would typically





Comment at: llvm/docs/DeveloperPolicy.rst:192
+
+* Adding, removing, or modifying command line options.
+* Adding or removing a diagnostic.

I think official coding documentation style guides say to use "command-line" - 
at least, that's what our internal docs team has told me in the past.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123957

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

[PATCH] D123682: [clang-tblgen] Automatically document options values

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

In D123682#3455793 , @aaron.ballman 
wrote:

> In D123682#3454627 , 
> @serge-sans-paille wrote:
>
>> @aaron.ballman Any thoughs on the above suggestion? I'd be happy to adopt 
>> any of those :-)
>
> I'd go with:
>
> More than two choices: ` should be 'return', 'branch', 'full', or 'none'`
> Only two choices: ` should be 'split' or 'single'.`

FWIW, I'd actually use "must" rather than "should", but otherwise I agree with 
this. "should" implies there are cases where it is okay to use a different 
value, which is obviously not the intent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123682

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

[PATCH] D123100: [Support/Hash functions] Change the `final()` and `result()` of the hashing functions to return an array of bytes

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: bolt/lib/Core/DebugData.cpp:823
 Hasher.update(AbbrevData);
-StringRef Key = Hasher.final();
+auto Hash = Hasher.final();
+StringRef Key((const char *)Hash.data(), Hash.size());

Amir wrote:
> I think it would be more in line with LLVM coding standards to expand `auto` 
> in this case (and others) – see 
> https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable.
>  
> My understanding is that `auto` is fine where the type is obvious from the 
> context (is explicitly available on the same line e.g. with casts), is 
> abstract (T::iterator types), or doesn't matter (e.g. lambdas).
+1 (also relevant in other places in this patch)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123100

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

[PATCH] D120713: [clangd] Make dexp command line options sticky

[James Henderson via Phabricator via cfe-commits]

jhenderson resigned from this revision.
jhenderson added a comment.

I don't know anything about clangd either. Not sure why I was added as a 
reviewer :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120713

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

[PATCH] D116351: Update Bug report URL to Github Issues

[James Henderson via Phabricator via cfe-commits]

jhenderson accepted this revision.
jhenderson added a comment.

LGTM, from my point of view.


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

https://reviews.llvm.org/D116351

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

[PATCH] D116351: Update Bug report URL to Github Issues

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: llvm/docs/CommandGuide/llvm-install-name-tool.rst:79
 
-To report bugs, please visit .
+To report bugs, please visit .
 

I believe llvm-install-name-tool is a variation of llvm-objcopy, so should 
probably reference the llvm-objcopy URL.



Comment at: llvm/docs/CommandGuide/llvm-otool.rst:135
 
-To report bugs, please visit .
+To report bugs, please visit .
 

I believe llvm-otool is a variation of llvm-objdump, so should reference the 
llvm-objdump URL.


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

https://reviews.llvm.org/D116351

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

[PATCH] D114639: Raise the minimum Visual Studio version to VS2019

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

Re. the bots: I'd hope we'd have at least some bots using VS2019 rather than 
all rushing to VS2022. There's nothing worse than claiming to support a minimum 
version of something and then not actually supporting it...! Also internally, 
we are switching to a default of VS2019, not direct to VS2022, so having 
upstream coverage there would be useful to avoid downstream breakages that are 
actually upstream problems - we're working on adding our own Windows build bot, 
but I don't think it's there yet.




Comment at: clang/docs/UsersManual.rst:3546
 
-cmake -G"Visual Studio 15 2017" -T LLVM ..
+cmake -G"Visual Studio 17 2022" -T LLVM ..
 

stella.stamenova wrote:
> compnerd wrote:
> > Meinersbur wrote:
> > > jhenderson wrote:
> > > > RKSimon wrote:
> > > > > aaron.ballman wrote:
> > > > > > jhenderson wrote:
> > > > > > > I think the missing space should be fixed to :)
> > > > > > +1 to the missing space.
> > > > > This one is confusing - it isn't in my local diff, the raw diff, or 
> > > > > if I reapply the raw diff - it looks to have just appeared when you 
> > > > > quote it in phab comments?
> > > > The fact that the space is missing? It's missing in the current repo 
> > > > too.
> > > It works with and without the space, like `-GNinja` and `-G Ninja` both 
> > > work.
> > It would be nice to mention the CMake minimum version.  I think that you 
> > need 3.22 or newer for the 2022 generator/toolset definition.
> It works with or without the space, but it reads better with the space.
We should probably drop the space issue: I noticed further down that it doesn't 
have a space in either for e.g. Ninja. Don't mind either way though.



Comment at: clang/docs/UsersManual.rst:3546
 
-cmake -G"Visual Studio 15 2017" -T LLVM ..
+cmake -G"Visual Studio 16 2019" -T LLVM ..
 

stella.stamenova wrote:
> jhenderson wrote:
> > Maybe make this VS2022 instead, to help it last longer?
> I think it makes more sense to make it 2019 because I expect most people to 
> still be using 2019 and it's convenient to have instructions that just work. 
> Maybe add both?
No objections either way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114639

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

[PATCH] D114639: Raise the minimum Visual Studio version to VS2019

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: clang/docs/UsersManual.rst:3546
 
-cmake -G"Visual Studio 15 2017" -T LLVM ..
+cmake -G"Visual Studio 17 2022" -T LLVM ..
 

RKSimon wrote:
> aaron.ballman wrote:
> > jhenderson wrote:
> > > I think the missing space should be fixed to :)
> > +1 to the missing space.
> This one is confusing - it isn't in my local diff, the raw diff, or if I 
> reapply the raw diff - it looks to have just appeared when you quote it in 
> phab comments?
The fact that the space is missing? It's missing in the current repo too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114639

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

[PATCH] D114639: Raise the minimum Visual Studio version to VS2019

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

No more comments from me (apart from one minor nit). This should definitely get 
someone with more familiarity with how these things are configured to take a 
look though.




Comment at: clang/docs/UsersManual.rst:3546
 
-cmake -G"Visual Studio 15 2017" -T LLVM ..
+cmake -G"Visual Studio 17 2022" -T LLVM ..
 

I think the missing space should be fixed to :)



Comment at: llvm/include/llvm/Support/Compiler.h:106-108
 /// Sadly, this is separate from just rvalue reference support because GCC
 /// and MSVC implemented this later than everything else. This appears to be
 /// corrected in MSVC 2019 but not MSVC 2017.

RKSimon wrote:
> jhenderson wrote:
> > Does this comment need changing?
> An even bigger question is - can we get rid of the 
> LLVM_HAS_RVALUE_REFERENCE_THIS define entirely now? Either as part of this 
> patch or as a followup
Yeah, another patch entirely to sort this would be fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114639

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

[PATCH] D114639: Raise the minimum Visual Studio version to VS2019

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: clang/docs/UsersManual.rst:3546
 
-cmake -G"Visual Studio 15 2017" -T LLVM ..
+cmake -G"Visual Studio 16 2019" -T LLVM ..
 

Maybe make this VS2022 instead, to help it last longer?



Comment at: llvm/docs/GettingStarted.rst:276
 do. Windows does not have a "system compiler", so you must install either 
Visual
-Studio 2017 or a recent version of mingw64. FreeBSD 10.0 and newer have a 
modern
+Studio 2019 or a recent version of mingw64. FreeBSD 10.0 and newer have a 
modern
 Clang as the system compiler.

Perhaps add "or Visual Studio 2022" or similar?



Comment at: llvm/include/llvm/Support/Compiler.h:106-108
 /// Sadly, this is separate from just rvalue reference support because GCC
 /// and MSVC implemented this later than everything else. This appears to be
 /// corrected in MSVC 2019 but not MSVC 2017.

Does this comment need changing?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114639

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

[PATCH] D104804: [AMDGPU] Add gfx1035 target

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: llvm/test/Object/AMDGPU/elf-header-flags-mach.yaml:189
 
+# RUN: sed -e 's//64/' -e 's//AMDGCN_GFX1035/' %s | yaml2obj -o 
%t.o.AMDGCN_GFX1035
+# RUN: llvm-readobj -S --file-headers %t.o.AMDGCN_GFX1035 | FileCheck 
--check-prefixes=ELF-AMDGCN-ALL,ELF-AMDGCN-GFX1035 %s

MaskRay wrote:
> sed can be replaced by `FileCheck -D` (search examples in `test/tools/`).
> 
> The long list becomes unwieldy now. @jhenderson Suggestions on decreasing the 
> number of RUN lines?
> sed can be replaced by `FileCheck -D` (search examples in `test/tools/`).

Is that true in this context? The `sed` command is controlling input to 
yaml2obj, not some FileCheck stuff.

> The long list becomes unwieldy now. @jhenderson Suggestions on decreasing the 
> number of RUN lines?

I think you could use additional -D options for FileCheck. Something like the 
following:

```
# RUN: llvm-readobj ... | FileCheck --check-prefixes=ELF-ALL,ELF-AMDGCN 
-DNAME=EF_AMDGPU_MACH_AMDGCN_GFX1035 -DVAL=0x3D
# RUN: obj2yaml ... | FileCheck %s --check-prefixes=YAML-ALL,YAML-AMDGCN 
-DFLAG=EF_AMDGPU_MACH_AMDGCN_GFX1035
## Repeat for all the other values.

## NB: Some of these might be better with the -NEXT suffix.
# ELF-R600: Format: elf32-amdgpu
# ELF-R600: Arch: r600
# ELF-R600: AddressSize: 32bit

# ELF-AMDGCN: Format: elf64-amdgpu
# ELF-AMDGCN: Arch: amdgcn
# ELF-AMDGCN: AddressSize: 64bit

# ELF-ALL:  Flags [
# ELF-ALL-NEXT:   [[NAME]] ([[VAL]])
# ELF-ALL-NEXT: ]

# YAML-R600:   Class: ELFCLASS32
# YAML-AMDGCN: Class: ELFCLASS64
# YAML-ALL:Flags: [ [[FLAG]] ]
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104804

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

[PATCH] D104088: Add clang frontend flags for MIP

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:59
+ StringRef(Sec.Name).startswith(".zdebug") ||
+ Sec.Name == ".gdb_index" || Sec.Name == "__llvm_mipmap";
 }

This doesn't look like it belongs as part of this commit?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104088

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

[PATCH] D103125: [Clang][WIP] Allow renaming of "clang"

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

In D103125#2780936 , @dblaikie wrote:

> Can't say I'm super enthusiastic about this (I assume the build already 
> supports prefixes and suffixes, which I'd hope would be adequate - but 
> presumably are not for your use case), though there's some, I think, related 
> prior art: Sony folks (@probinson @jhenderson) have (or had at some point) 
> different C++ language standard/version defaults than upstream and have 
> maintained/made changes to upstream test cases that assume the upstream 
> default version to not make that assumption (to have it explicit). So having 
> some costs/changes upstream for downstream differences like this seems at 
> least vaguely plausible to me.

We do our executable renaming post build and lit testing. We do the renaming to 
nearly all our built tools, not just the clang (clang++ etc) family, e.g. the 
LLVM binutils like llvm-objdump becomes xxx-llvm-objdump, so unless the scope 
of this increases to include those, I'm not sure how useful it would be to us 
(and expanding the scope to other tools becomes problematic because there isn't 
a `%clang` equivalent for testing purposes for those other tools, so presumably 
would require significantly more updates?).

@probinson may have more thoughts on this though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103125

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

[PATCH] D95808: [test] Use host platform specific error message substitution in lit tests - continued

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

@ASDenysPetrov, I think you need to mark this patch as Accepted so that someone 
can close this review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95808

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

[PATCH] D99360: [OpenMP][WIP] Add standard notes for ELF offload images

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

It might make sense to do the llvm-readobj portions of this patch in a separate 
review, since they are somewhat independent.


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

https://reviews.llvm.org/D99360

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

[PATCH] D99182: [NFC] Reordering parameters in getFile and getFileOrSTDIN

[James Henderson via Phabricator via cfe-commits]

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

LGTM, with nit.




Comment at: llvm/lib/Support/MemoryBuffer.cpp:275
+  return getFileAux(
+  Filename, /*MapSize=*/-1, 0, /*IsText=*/false,
+  /*RequiresNullTerminator=*/false, IsVolatile);

Whilst you're modifying, add the named parameter comment for the `0`, so that 
it's not just a magic number.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99182

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

[PATCH] D99182: [NFC] Reordering parameters in getFile and getFileOrSTDIN

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

Overall, this seems reasonable to me. I have a slight concern that it might 
cause surprising failures for downstream users, that aren't picked up at build 
time (due to implicit conversions), but I don't think you need to worry too 
much about that.




Comment at: llvm/include/llvm/Support/MemoryBuffer.h:78-80
   /// if successful, otherwise returning null. If FileSize is specified, this
   /// means that the client knows that the file exists and that it has the
   /// specified size.

You need to remove the reference to FileSize from the description here.



Comment at: llvm/lib/Support/MemoryBuffer.cpp:108
 static ErrorOr>
 getFileAux(const Twine , int64_t FileSize, uint64_t MapSize,
+   uint64_t Offset, bool IsText, bool RequiresNullTerminator,

Are there users of the `FileSize` here? If not, can you repeat the change for 
this function?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99182

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

[PATCH] D98278: [test] Add ability to get error messages from CMake for errc substitution

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

In D98278#2628433 , @zero9178 wrote:

> I addressed your comments in 
> https://reviews.llvm.org/rG4a17ac0387f078529da02e355a24df99f645d364. Hope it 
> should be alright now

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98278

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

[PATCH] D98278: [test] Add ability to get error messages from CMake for errc substitution

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: llvm/cmake/modules/GetErrcMessages.cmake:3-6
+# The purpose of this function is to supply those error messages to llvm-lit 
using the errc_messages config
+# Currently supplied and needed error codes: ENOENT, EISDIR, EINVAL and EACCES
+# Messages are semi colon separated
+# Keep amount, order and tested error codes in sync with 
llvm/utils/lit/lit/llvm/config.py

zero9178 wrote:
> jhenderson wrote:
> > Each of these lines look like they need a trailing full stop added.
> Just to double check, that means adding a `.` at the end of the lines right?
Yes, that's right. Full stop (British English) == period (American English) == 
`.`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98278

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

[PATCH] D98278: [test] Add ability to get error messages from CMake for errc substitution

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

Thanks for working on this. A couple of post-commit comments to look at, 
otherwise looks good. Thanks for figuring out how to do this in a portable 
manner!




Comment at: llvm/cmake/modules/GetErrcMessages.cmake:1
+
+# This function returns the messages of various POSIX error codes as they are 
returned by std::error_code.

Delete this blank line?



Comment at: llvm/cmake/modules/GetErrcMessages.cmake:3-6
+# The purpose of this function is to supply those error messages to llvm-lit 
using the errc_messages config
+# Currently supplied and needed error codes: ENOENT, EISDIR, EINVAL and EACCES
+# Messages are semi colon separated
+# Keep amount, order and tested error codes in sync with 
llvm/utils/lit/lit/llvm/config.py

Each of these lines look like they need a trailing full stop added.



Comment at: llvm/utils/lit/lit/llvm/config.py:14
 lit_path_displayed = False
+python_errc_displayed = False
 

Seems like this variable is unused and got leftover from an experiment at some 
point?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98278

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

[PATCH] D95246: [test] Use host platform specific error message substitution in lit tests

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

In D95246#2587410 , @ASDenysPetrov 
wrote:

> @jhenderson
> I think I'm done.
> Here is output: F15648076: linker_trace_output.txt 
> 
> Is it OK now?

Thanks, yes that does the trick. As I expected, you can see that all the 
libraries that are used are contained withing the msys2/mingw subdirectory, and 
therefore are not the standard Windows SDK files that you would typically use 
for a Windows build using Visual Studio. One of those will contain the 
definitions we are talking about.

@abhina.sreeskantharajan - I think you could this issue in one of two ways. One 
would be to detect when the tools/libraries used are within the msys2/mingw 
installation. Another way might be to actually build and run a miniature 
program at CMake time that prints out the messages, in such a way that CMake 
can inspect the output and use it to populate the lit configuration. Note: I am 
not a CMake expert, so don't know if this is strictly possible. The advantage 
of this approach is that you can remove the reliance on the system.platform 
call in python, and just always use the output of this simple program.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95246

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

[PATCH] D95246: [test] Use host platform specific error message substitution in lit tests

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

In D95246#2587277 , @ASDenysPetrov 
wrote:

> @jhenderson
>
>> Sorry, but `-W --trace` is not the same as `-Wl,--trace`. The former is a 
>> pair of options used by the compiler (one of which describes the files used 
>> by the compiler). The latter is an option passed to the linker, and is what 
>> we need. Please try again!
>
> I used `-W`, because my `c++` doesn't know `-Wl` option.
>
>   C:\Users\Denis>c++ -Wl
>   c++: error: unrecognized command-line option '-Wl'; did you mean '-W'?
>   c++: fatal error: no input files
>   compilation terminated.
>
> Could you provide exact cmd line me to use if I'm doing smth wrong?

`-Wl` on its own isn't an option. It is used as a prefix to pass an option to 
the linker, so the exact string you pass to the compiler is `-Wl,--trace` (no 
spaces, comma required). The compiler sees the `-Wl,` prefix, removes it and 
then passes the remainder straight to the linker. You can see examples of this 
in your existing commandline with things like `-Wl,--stack,16777216`.

The cmdline should be:

  D:\WORK\Utilities\MSYS2\mingw64\bin\c++.exe -DGTEST_HAS_RTTI=0 -D_DEBUG 
-D_FILE_OFFSET_BITS=64 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS 
-D__STDC_LIMIT_MACROS -Itools/llvm-ar 
-ID:/WORK/LLVM/llvm-project/llvm/tools/llvm-ar -Iinclude 
-ID:/WORK/LLVM/llvm-project/llvm/include -Wa,-mbig-obj -Werror=date-time -Wall 
-Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual 
-Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough 
-Wno-maybe-uninitialized -Wno-class-memaccess -Wno-redundant-move 
-Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment  
-O2   -fno-exceptions -fno-rtti -UNDEBUG -std=c++14 -MD -MT 
tools/llvm-ar/CMakeFiles/llvm-ar.dir/llvm-ar.cpp.obj -MF 
tools\llvm-ar\CMakeFiles\llvm-ar.dir\llvm-ar.cpp.obj.d -o 
tools/llvm-ar/CMakeFiles/llvm-ar.dir/llvm-ar.cpp.obj -c 
D:/WORK/LLVM/llvm-project/llvm/tools/llvm-ar/llvm-ar.cpp
  [2/2] cmd.exe /C "cd . && D:\WORK\Utilities\MSYS2\mingw64\bin\c++.exe 
-Wa,-mbig-obj -Werror=date-time -Wall -Wextra -Wno-unused-parameter 
-Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic 
-Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized 
-Wno-class-memaccess -Wno-redundant-move -Wno-noexcept-type 
-Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment  -O2 
-Wl,--stack,16777216 tools/llvm-ar/CMakeFiles/llvm-ar.dir/llvm-ar.cpp.obj -o 
bin\llvm-ar.exe -Wl,--out-implib,lib\libllvm-ar.dll.a 
-Wl,--major-image-version,0,--minor-image-version,0  lib/libLLVMX86AsmParser.a  
lib/libLLVMX86Desc.a  lib/libLLVMX86Info.a  lib/libLLVMBinaryFormat.a  
lib/libLLVMCore.a  lib/libLLVMDlltoolDriver.a  lib/libLLVMLibDriver.a  
lib/libLLVMObject.a  lib/libLLVMSupport.a  lib/libLLVMMCDisassembler.a  
lib/libLLVMMCParser.a  lib/libLLVMMC.a  lib/libLLVMDebugInfoCodeView.a  
lib/libLLVMDebugInfoMSF.a  lib/libLLVMTextAPI.a  lib/libLLVMOption.a  
lib/libLLVMBitReader.a  lib/libLLVMCore.a  lib/libLLVMBinaryFormat.a  
lib/libLLVMRemarks.a  lib/libLLVMBitstreamReader.a  lib/libLLVMSupport.a  
-lpsapi  -lshell32  -lole32  -luuid  -ladvapi32  
D:/WORK/Utilities/MSYS2/mingw64/lib/libz.dll.a  lib/libLLVMDemangle.a  
-lkernel32 -luser32 -lgdi32 -lwinspool -lshell32 -lole32 -loleaut32 -luuid 
-lcomdlg32 -ladvapi32 -Wl,--trace


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95246

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

[PATCH] D95246: [test] Use host platform specific error message substitution in lit tests

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

In D95246#2587167 , @ASDenysPetrov 
wrote:

> @jhenderson
>
>> Thanks (for clarity, `libz` isn't what I'm looking for, it was just an 
>> example of how this information might be useful). As it is, I think we might 
>> need more information still - that commandline hides which files are 
>> actually used slightly. Could you try explicitly running the c++ command 
>> that is being requested, but with the `-Wl,--trace` option added too. That 
>> will tell the linker to print what files it opens when it runs the link, and 
>> should give us an hint as to which system library is different for your 
>> usage than ours.
>
> I added `-W --trace` to the end of cmdline. The output is in the file: 
> F15646704: trace_output.txt 

Sorry, but `-W --trace` is not the same as `-Wl,--trace`. The former is a pair 
of options used by the compiler (one of which describes the files used by the 
compiler). The latter is an option passed to the linker, and is what we need. 
Please try again!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95246

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

[PATCH] D95246: [test] Use host platform specific error message substitution in lit tests

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

In D95246#2587123 , @ASDenysPetrov 
wrote:

>> This shows, for example, that the `libz.so` my build uses is located in 
>> `/usr/lib/x86_64-linux-gnu`. By having your equivalent path, we can 
>> hopefully confirm that the issue is caused by a different set of system 
>> libraries being used.
>
> Hi, @jhenderson
> I changed `llvm-ar.cpp` then ran `ninja -v llvm-ar` and got this:
>
>   [1/2] D:\WORK\Utilities\MSYS2\mingw64\bin\c++.exe -DGTEST_HAS_RTTI=0 
> -D_DEBUG -D_FILE_OFFSET_BITS=64 -D__STDC_CONSTANT_MACROS 
> -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/llvm-ar 
> -ID:/WORK/LLVM/llvm-project/llvm/tools/llvm-ar -Iinclude 
> -ID:/WORK/LLVM/llvm-project/llvm/include -Wa,-mbig-obj -Werror=date-time 
> -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual 
> -Wno-missing-field-initializers -pedantic -Wno-long-long 
> -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-class-memaccess 
> -Wno-redundant-move -Wno-noexcept-type -Wdelete-non-virtual-dtor 
> -Wsuggest-override -Wno-comment  -O2   -fno-exceptions -fno-rtti -UNDEBUG 
> -std=c++14 -MD -MT tools/llvm-ar/CMakeFiles/llvm-ar.dir/llvm-ar.cpp.obj -MF 
> tools\llvm-ar\CMakeFiles\llvm-ar.dir\llvm-ar.cpp.obj.d -o 
> tools/llvm-ar/CMakeFiles/llvm-ar.dir/llvm-ar.cpp.obj -c 
> D:/WORK/LLVM/llvm-project/llvm/tools/llvm-ar/llvm-ar.cpp
>   [2/2] cmd.exe /C "cd . && D:\WORK\Utilities\MSYS2\mingw64\bin\c++.exe 
> -Wa,-mbig-obj -Werror=date-time -Wall -Wextra -Wno-unused-parameter 
> -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic 
> -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized 
> -Wno-class-memaccess -Wno-redundant-move -Wno-noexcept-type 
> -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment  -O2 
> -Wl,--stack,16777216 tools/llvm-ar/CMakeFiles/llvm-ar.dir/llvm-ar.cpp.obj -o 
> bin\llvm-ar.exe -Wl,--out-implib,lib\libllvm-ar.dll.a 
> -Wl,--major-image-version,0,--minor-image-version,0  
> lib/libLLVMX86AsmParser.a  lib/libLLVMX86Desc.a  lib/libLLVMX86Info.a  
> lib/libLLVMBinaryFormat.a  lib/libLLVMCore.a  lib/libLLVMDlltoolDriver.a  
> lib/libLLVMLibDriver.a  lib/libLLVMObject.a  lib/libLLVMSupport.a  
> lib/libLLVMMCDisassembler.a  lib/libLLVMMCParser.a  lib/libLLVMMC.a  
> lib/libLLVMDebugInfoCodeView.a  lib/libLLVMDebugInfoMSF.a  
> lib/libLLVMTextAPI.a  lib/libLLVMOption.a  lib/libLLVMBitReader.a  
> lib/libLLVMCore.a  lib/libLLVMBinaryFormat.a  lib/libLLVMRemarks.a  
> lib/libLLVMBitstreamReader.a  lib/libLLVMSupport.a  -lpsapi  -lshell32  
> -lole32  -luuid  -ladvapi32  D:/WORK/Utilities/MSYS2/mingw64/lib/libz.dll.a  
> lib/libLLVMDemangle.a  -lkernel32 -luser32 -lgdi32 -lwinspool -lshell32 
> -lole32 -loleaut32 -luuid -lcomdlg32 -ladvapi32 && cd ."
>
> As you can `libz` is here `D:/WORK/Utilities/MSYS2/mingw64/lib/libz.dll.a`

Thanks (for clarity, `libz` isn't what I'm looking for, it was just an example 
of how this information might be useful). As it is, I think we might need more 
information still - that commandline hides which files are actually used 
slightly. Could you try explicitly running the c++ command that is being 
requested, but with the `-Wl,--trace` option added too. That will tell the 
linker to print what files it opens when it runs the link, and should give us 
an hint as to which system library is different for your usage than ours.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95246

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

[PATCH] D95246: [test] Use host platform specific error message substitution in lit tests

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

@ASDenysPetrov, if you run "ninja -v llvm-ar", what is the output of the line 
that actually builds the llvm-ar executable? That'll tell us which libraries 
are in use, which should hopefully point to the difference to our own 
environments. For example, the last line of output for this on my Linux machine 
is:

  [811/811] : && /usr/bin/c++ -fPIC -fvisibility-inlines-hidden 
-Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings 
-Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long 
-Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-noexcept-type 
-Wdelete-non-virtual-dtor -Wno-comment -fdiagnostics-color -ffunction-sections 
-fdata-sections -O3 
-Wl,-rpath-link,/home/binutils/llvm/llvm-project/build/./lib  -Wl,-O3 
-Wl,--gc-sections tools/llvm-ar/CMakeFiles/llvm-ar.dir/llvm-ar.cpp.o -o 
bin/llvm-ar  -Wl,-rpath,"\$ORIGIN/../lib"  lib/libLLVMAArch64AsmParser.a  
lib/libLLVMAMDGPUAsmParser.a  lib/libLLVMARMAsmParser.a  
lib/libLLVMAVRAsmParser.a  lib/libLLVMBPFAsmParser.a  
lib/libLLVMHexagonAsmParser.a  lib/libLLVMLanaiAsmParser.a  
lib/libLLVMMipsAsmParser.a  lib/libLLVMMSP430AsmParser.a  
lib/libLLVMPowerPCAsmParser.a  lib/libLLVMRISCVAsmParser.a  
lib/libLLVMSparcAsmParser.a  lib/libLLVMSystemZAsmParser.a  
lib/libLLVMWebAssemblyAsmParser.a  lib/libLLVMX86AsmParser.a  
lib/libLLVMAArch64Desc.a  lib/libLLVMAMDGPUDesc.a  lib/libLLVMARMDesc.a  
lib/libLLVMAVRDesc.a  lib/libLLVMBPFDesc.a  lib/libLLVMHexagonDesc.a  
lib/libLLVMLanaiDesc.a  lib/libLLVMMipsDesc.a  lib/libLLVMMSP430Desc.a  
lib/libLLVMNVPTXDesc.a  lib/libLLVMPowerPCDesc.a  lib/libLLVMRISCVDesc.a  
lib/libLLVMSparcDesc.a  lib/libLLVMSystemZDesc.a  lib/libLLVMWebAssemblyDesc.a  
lib/libLLVMX86Desc.a  lib/libLLVMXCoreDesc.a  lib/libLLVMAArch64Info.a  
lib/libLLVMAMDGPUInfo.a  lib/libLLVMARMInfo.a  lib/libLLVMAVRInfo.a  
lib/libLLVMBPFInfo.a  lib/libLLVMHexagonInfo.a  lib/libLLVMLanaiInfo.a  
lib/libLLVMMipsInfo.a  lib/libLLVMMSP430Info.a  lib/libLLVMNVPTXInfo.a  
lib/libLLVMPowerPCInfo.a  lib/libLLVMRISCVInfo.a  lib/libLLVMSparcInfo.a  
lib/libLLVMSystemZInfo.a  lib/libLLVMWebAssemblyInfo.a  lib/libLLVMX86Info.a  
lib/libLLVMXCoreInfo.a  lib/libLLVMBinaryFormat.a  lib/libLLVMCore.a  
lib/libLLVMDlltoolDriver.a  lib/libLLVMLibDriver.a  lib/libLLVMObject.a  
lib/libLLVMSupport.a  -lpthread  lib/libLLVMAArch64Utils.a  
lib/libLLVMAMDGPUUtils.a  lib/libLLVMARMUtils.a  lib/libLLVMMCDisassembler.a  
lib/libLLVMMCParser.a  lib/libLLVMMC.a  lib/libLLVMDebugInfoCodeView.a  
lib/libLLVMDebugInfoMSF.a  lib/libLLVMTextAPI.a  lib/libLLVMOption.a  
lib/libLLVMBitReader.a  lib/libLLVMCore.a  lib/libLLVMBinaryFormat.a  
lib/libLLVMRemarks.a  lib/libLLVMBitstreamReader.a  lib/libLLVMSupport.a  -lrt  
-ldl  -lpthread  -lm  /usr/lib/x86_64-linux-gnu/libz.so  
/usr/lib/x86_64-linux-gnu/libtinfo.so  lib/libLLVMDemangle.a && :

This shows, for example, that the `libz.so` my build uses is located in 
`/usr/lib/x86_64-linux-gnu`. By having your equivalent path, we can hopefully 
confirm that the issue is caused by a different set of system libraries being 
used.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95246

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

[PATCH] D97138: [Driver] replace argc_ with argc

[James Henderson via Phabricator via cfe-commits]

jhenderson resigned from this revision.
jhenderson added a comment.

In general, I don't work on the clang side. As such, I don't know the norms 
surrounding this sort of change in this area, and don't feel comfortable 
reviewing. You should look at getting reviewers who do work in that part of the 
repository.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97138

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

[PATCH] D95246: [test] Use host platform specific error message substitution in lit tests

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

In D95246#2568084 , @ASDenysPetrov 
wrote:

> @jhenderson
>
>> @ASDenysPetrov, could you provide your link command-line for one of the 
>> tools that produces the failing message, please, e.g. llvm-ar?
>
> Here is output of running tests in llvm-ar folder: F15562458: 
> fail_output_llvm_ar.txt 

Sorry, what I meant was, what is the command used to build the llvm-ar 
executable, not what does it print when used. The command used to build llvm-ar 
should tell us what libraries are being used, which could impact this behaviour.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95246

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

[PATCH] D95246: [test] Use host platform specific error message substitution in lit tests

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

@ASDenysPetrov, could you provide your link command-line for one of the tools 
that produces the failing message, please, e.g. llvm-ar? That may give us a 
hint about whether different libraries are being used. It's possible we need 
some additional cmake-time configuration steps to detect this difference. Also 
are you using cygwin at all?

Not that I'm saying we shouldn't try to fix your use-case, but if you aren't 
using Cygwin, it's worth noting that 
https://llvm.org/docs/GettingStarted.html#hardware states that only Visual 
Studio builds (whether via the IDE or some other tool like ninja) are known to 
work for Windows.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95246

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

[PATCH] D95246: [test] Use host platform specific error message substitution in lit tests

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

In D95246#2565351 , 
@abhina.sreeskantharajan wrote:

> Do you know what is different between your environments which is causing the 
> spelling to be capitalized? This might help me determine how to fix the 
> current host platform check in llvm/utils/lit/lit/llvm/config.py.

Could it be something to do with a different standard library being used?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95246

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

[PATCH] D95808: [test] Use host platform specific error message substitution in lit tests - continued

[James Henderson via Phabricator via cfe-commits]

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

LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95808

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

[PATCH] D95849: [FileCheck] Default --allow-unused-prefixes to false

[James Henderson via Phabricator via cfe-commits]

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

I assume this now runs cleanly. If so, LGTM. Probably worth mentioning on the 
mailing list thread too, so wrap up that conversation. By the way, did we ever 
figure out how many of the failures were genuine bugs versus deliberate 
isntances?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95849

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

[PATCH] D95808: [test] Use host platform specific error message substitution in lit tests - continued

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

I think you need to add the new values to the list of recognised substitutions 
in the documentation?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95808

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

[PATCH] D95246: [test] Use host platform specific error message substitution in lit tests

[James Henderson via Phabricator via cfe-commits]

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

Great work! LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95246

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

[PATCH] D95246: [SystemZ][z/OS] Fix No such file or directory expression error matching in lit tests - continued

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

In D95246#2527999 , 
@abhina.sreeskantharajan wrote:

> In D95246#2527961 , @jhenderson 
> wrote:
>
>> One nit, but otherwise looks good to me, thanks! Please go ahead with the 
>> other test updates. Do you plan on doing them in this patch?
>
> This was my initial thought. But if it's preferred to create a second patch 
> to make the remaining changes to affected testcases, I have no issue with 
> doing that.

I'm happy either way, whichever you prefer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95246

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

[PATCH] D95246: [SystemZ][z/OS] Fix No such file or directory expression error matching in lit tests - continued

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

One nit, but otherwise looks good to me, thanks! Please go ahead with the other 
test updates. Do you plan on doing them in this patch?




Comment at: llvm/docs/TestingGuide.rst:545
+
+   The following error codes are supported: ENOENT, EISDIR.
+

Adding "currently" implies to me that someone could add to the list easily.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95246

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

[PATCH] D95246: [SystemZ][z/OS] Fix No such file or directory expression error matching in lit tests - continued

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: clang/test/Driver/clang-offload-bundler.c:74
+// RUN: not clang-offload-bundler -type=i 
-targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu
 -inputs=%t.i,%t.tgt1,%t.tgt2.notexist -outputs=%t.bundle.i 2>&1 | \
+// RUN:  FileCheck %s -DFILE=%t.tgt2.notexist -DMSG=%errc_ENOENT 
--check-prefix CK-ERR5
+// RUN: not clang-offload-bundler -type=i 
-targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu
 -outputs=%t.i,%t.tgt1,%t.tgt2 -inputs=%t.bundle.i.notexist -unbundle 2>&1 | \

Add one more space - it helps the indentation stand out more and therefore make 
it clearer this line is a continuation.



Comment at: clang/test/Driver/clang-offload-bundler.c:76
+// RUN: not clang-offload-bundler -type=i 
-targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu
 -outputs=%t.i,%t.tgt1,%t.tgt2 -inputs=%t.bundle.i.notexist -unbundle 2>&1 | \
+// RUN:  FileCheck %s -DFILE=%t.bundle.i.notexist -DMSG=%errc_ENOENT 
--check-prefix CK-ERR5
+// CK-ERR5: error: '[[FILE]]': [[MSG]]





Comment at: llvm/docs/TestingGuide.rst:545-547
+   Example (%errc_ENOENT): ``No such file or directory``
+
+   Example (%errc_ENOENT): ``no such file or directory``

jhenderson wrote:
> This is a slightly different format to the other examples above. I think it 
> should be like the inline edit. this also helps distinguish the difference 
> between the two.
> 
> Note: I can't remember which way around the two versions are, so you might 
> need to swap them!
We probably need to list what error codes are currently supported somewhere.



Comment at: llvm/utils/lit/lit/llvm/config.py:349-351
+triple = ""
+if hasattr(self.config, 'host_triple'):
+triple = self.config.host_triple

abhina.sreeskantharajan wrote:
> jhenderson wrote:
> > I'm concerned that someone might start using these substitutions in a 
> > project for the first time, and get confused why they don't work on 
> > non-windows platforms. Maybe the solution is simply to require 
> > LLVM_HOST_TRIPLE to be set in all projects, i.e. go back to what you were 
> > doing before, and letting python fail if it isn't set.
> > 
> > Happy to hear other ideas too.
> I think using sys.platform or platform.system() could be a better 
> alternative. What do you think?
Makes sense to me. Slight issue: cygwin on Windows uses `cygwin`. What error 
message does it produce though?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95246

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

[PATCH] D95246: [SystemZ][z/OS] Fix No such file or directory expression error matching in lit tests - continued

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

In D95246#2522913 , 
@abhina.sreeskantharajan wrote:

> Please let me know if there are other guides I will need to update that I'm 
> not aware of.

There's also the lit CommandGuide located at `llvm/docs/CommandGuide/lit.rst`.




Comment at: clang/test/Driver/clang-offload-bundler.c:73-74
 
-// RUN: not clang-offload-bundler -type=i 
-targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu
 -inputs=%t.i,%t.tgt1,%t.tgt2.notexist -outputs=%t.bundle.i 2>&1 | FileCheck %s 
-DFILE=%t.tgt2.notexist --check-prefix CK-ERR5
-// RUN: not clang-offload-bundler -type=i 
-targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu
 -outputs=%t.i,%t.tgt1,%t.tgt2 -inputs=%t.bundle.i.notexist -unbundle 2>&1 | 
FileCheck %s -DFILE=%t.bundle.i.notexist --check-prefix CK-ERR5
-// CK-ERR5: error: '[[FILE]]': {{N|n}}o such file or directory
+// RUN: not clang-offload-bundler -type=i 
-targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu
 -inputs=%t.i,%t.tgt1,%t.tgt2.notexist -outputs=%t.bundle.i 2>&1 | FileCheck %s 
-DFILE=%t.tgt2.notexist -DMSG=%errc_ENOENT --check-prefix CK-ERR5
+// RUN: not clang-offload-bundler -type=i 
-targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu
 -outputs=%t.i,%t.tgt1,%t.tgt2 -inputs=%t.bundle.i.notexist -unbundle 2>&1 | 
FileCheck %s -DFILE=%t.bundle.i.notexist -DMSG=%errc_ENOENT --check-prefix 
CK-ERR5
+// CK-ERR5: error: '[[FILE]]': [[MSG]]

Nit: These are probably good candidates to split up over multiple lines, as in 
the inline edit.



Comment at: lld/CMakeLists.txt:115
+if (LLD_BUILT_STANDALONE)
+  set(LLVM_HOST_TRIPLE ${TARGET_TRIPLE})
+endif()

The target and the host may be two completely different things. This variable 
setting doesn't look right to me. In a situation where someone is building LLD 
to run on Windows but targeting Linux, the host triple value will end up being 
set to a Linux triple, and the tests would fail.

The variable setting can also be moved inside the earlier if about 5 lines up, 
because that block is hit when `LLD_BUILD_STANDALONE` is set.



Comment at: llvm/docs/TestingGuide.rst:545-547
+   Example (%errc_ENOENT): ``No such file or directory``
+
+   Example (%errc_ENOENT): ``no such file or directory``

This is a slightly different format to the other examples above. I think it 
should be like the inline edit. this also helps distinguish the difference 
between the two.

Note: I can't remember which way around the two versions are, so you might need 
to swap them!



Comment at: llvm/utils/lit/lit/llvm/config.py:349-351
+triple = ""
+if hasattr(self.config, 'host_triple'):
+triple = self.config.host_triple

I'm concerned that someone might start using these substitutions in a project 
for the first time, and get confused why they don't work on non-windows 
platforms. Maybe the solution is simply to require LLVM_HOST_TRIPLE to be set 
in all projects, i.e. go back to what you were doing before, and letting python 
fail if it isn't set.

Happy to hear other ideas too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95246

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

[PATCH] D95246: [SystemZ][z/OS] Fix No such file or directory expression error matching in lit tests - continued

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

I did briefly consider one alternative, which was to build these directly into 
FileCheck, but it doesn't quite feel like the right approach for FileCheck to 
need to know system level details. In practice, I think the lit substitution 
may be the best way forward overall. In the future, it should be easy to expand 
with more error messages.

You should add the new substitution to the lit command guide, so that it's 
documented. It might also be worth a heads up on llvm-dev to get feedback from 
those not directly on this review, to see if there are other ideas.




Comment at: llvm/utils/lit/lit/llvm/config.py:349-354
+if (re.match(r's390x-.*-zos', triple)):
+
self.config.substitutions.append(('%err_no_such_file_or_directory', '\'EDC5129I 
No such file or directory.\''))
+elif (re.match(r'.*windows.*', triple)):
+
self.config.substitutions.append(('%err_no_such_file_or_directory', '\'no such 
file or directory\''))
+else:
+
self.config.substitutions.append(('%err_no_such_file_or_directory', '\'No such 
file or directory\''))

These lines are quite long, so probably want reflowing.

I wonder if `%errc_...` might be a better name? That way, it ties to the 
`std::errc` values these match up with.



Comment at: llvm/utils/lit/lit/llvm/config.py:369-370
 
+if hasattr(self.config, 'host_triple'):
+   self.add_err_msg_substitutions(self.config.host_triple) 
+

Under what conditions can there not be a `host_triple`? In those cases, what 
happens to the tests that use the new substitution?



Comment at: llvm/utils/lit/lit/llvm/config.py:372
+
+
 def use_llvm_tool(self, name, search_env=None, required=False, 
quiet=False):

Nit: too many blank lines (compared to what was there before)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95246

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

[PATCH] D85474: Add -fbinutils-version= to gate ELF features on the specified binutils version

[James Henderson via Phabricator via cfe-commits]

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

Basically LGTM - (I'm happy for this to land either with or without my comments 
being addressed). Would be nice to get a second reviewer to confirm they're 
happy.




Comment at: clang/test/Driver/fbinutils-version.c:1
+// RUN: %clang -### -c -target x86_64-linux %s -fbinutils-version=none 2>&1 | 
FileCheck %s --check-prefix=NONE
+

Maybe also add a case for `-fbinutils-version=2` (i.e. no minor version at all).



Comment at: llvm/lib/CodeGen/LLVMTargetMachine.cpp:64
 
+  if (Options.BinutilsVersion.first > 0)
+TmpAsmInfo->setBinutilsVersion(Options.BinutilsVersion);

Is it possible somebody might do something weird like `-fbinutils-version=0.1` 
and get behaviour different to what might be expected? Should we explicitly 
forbid major versions of 0?



Comment at: llvm/lib/Target/TargetMachine.cpp:239
+  if (Version == "none")
+return {INT_MAX, 0}; // Make binutilsIsAtLeast() return true.
+  std::pair Ret;

Maybe `{INT_MAX, INT_MAX}`? Doesn't really matter in practice, but `{INT_MAX, 
INT_MAX} > {INT_MAX, 0}` in the versioning scheme.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85474

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

[PATCH] D95246: [SystemZ][z/OS] Fix No such file or directory expression error matching in lit tests - continued

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

In D95246#2519642 , 
@abhina.sreeskantharajan wrote:

> In D95246#2518989 , @jhenderson 
> wrote:
>
>> Sorry, could you revert this please. I don't think this is a good fix, as 
>> you've reduced coverage within the test, and some changes are completly 
>> redundant/add extra noise. I've commented inline with examples. Skimming 
>> D94239  suggests that has the same issue.
>>
>> Could you also please explain the messages your system is actually producing 
>> so we can provide a better solution than this fix.
>>
>> I'm also concerned that you are doing this to fix this test for a system, 
>> yet there are no build bots that report this error. A future contributor is 
>> likely to break them in the same way/add new tests with the same issue. If 
>> your system is a system that is supposed to be supported by LLVM, there 
>> needs to be a build bot. If it isn't supported, you should bring this up on 
>> llvm-dev (if you haven't already) to get buy-in for support.
>
> Thanks for the feedback. I've reverted my changes from these two patches. We 
> have indicated that we wish to add support for the z/OS platform but we have 
> not set up a buildbot yet.
>
> In D95246#2519086 , @grimar wrote:
>
>> As far I understand, looking on the description of D94239 
>> , the message on z/OS looks like "EDC5129I 
>> No such file or directory.".
>> I guess the `EDC5129I` is a stable error code? So why not to check for a 
>> possible `EDC5129I` prefix word instead of `.*`?
>> (The same applies for other possible errors)
>
> As grimar noted, this is indeed the correct error message.  "EDC5129I No such 
> file or directory." (Note the extra period at the end)
> Based on your feedback, these are the better alternatives that were suggested:

Slightly off-the-wall idea: I'm assuming you don't control your system in such 
a way that you can change the error message to omit the error code?

>   '{{.*N|n}}o such file or directory'

>   {{EDC5129I N|N|n}}o such file or directory'
>
> Some testcases fail because of the extra period at the end. For those 
> testcases, this is a possible alternative.
>
>   {{.*N|n}}o such file or directory{{\.?}}
>
> Please let me know if there are better alternatives I could look into.

I think you can just omit the trailing full stop in those cases. If the test 
isn't using --match-full-lines, it should be fine. If it is, adding `{{.?}}` 
seems reasonable.

Having the error code explicitly in the pattern looks like the right solution 
for now, but a thought on that - it seems like tests will still have the 
fragility problem for when someone else writes a new test that checks the 
message due to the error code not being present on most systems. Is the error 
code different for each system error message (I'm guessing it is)? I wonder if 
we would be better off adding some sort of lit substitution or similar that 
expands to the right string for the given host OS, which could in turn be fed 
to FileCheck. It might look a bit like this in practice:

  # RUN: not do-a-thing %t 2>&1 | FileCheck %s -DMSG=%enoent -DFILE=%t
  
  # CHECK: error: '[[FILE]]': [[MSG]]

What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95246

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

[PATCH] D95246: [SystemZ][z/OS] Fix No such file or directory expression error matching in lit tests - continued

[James Henderson via Phabricator via cfe-commits]

jhenderson added a subscriber: grimar.
jhenderson added a comment.

Sorry, could you revert this please. I don't think this is a good fix, as 
you've reduced coverage within the test, and some changes are completly 
redundant/add extra noise. I've commented inline with examples. Skimming D94239 
 suggests that has the same issue.

Could you also please explain the messages your system is actually producing so 
we can provide a better solution than this fix.

I'm also concerned that you are doing this to fix this test for a system, yet 
there are no build bots that report this error. A future contributor is likely 
to break them in the same way/add new tests with the same issue. If your system 
is a system that is supposed to be supported by LLVM, there needs to be a build 
bot. If it isn't supported, you should bring this up on llvm-dev (if you 
haven't already) to get buy-in for support.




Comment at: clang/test/Driver/clang-offload-bundler.c:75
 // RUN: not clang-offload-bundler -type=i 
-targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu
 -outputs=%t.i,%t.tgt1,%t.tgt2 -inputs=%t.bundle.i.notexist -unbundle 2>&1 | 
FileCheck %s -DFILE=%t.bundle.i.notexist --check-prefix CK-ERR5
-// CK-ERR5: error: '[[FILE]]': {{N|n}}o such file or directory
+// CK-ERR5: error: '[[FILE]]': {{.*}}{{N|n}}o such file or directory
 

Here and everywhere you've added the `{{.*}}`, the test will no longer pick up 
changes in the message that occur between the end of the literal match and the 
start of the "no such file..." part. For example, this will no longer fail if 
the message reported became "error: 'afile': fsdufbsdufbno such file or 
directory"

Even were adding `.*` desirable, it should be part of the following regex, i.e. 
`{{.*N|n}}` to reduce the noise of the `{{` and `}}`.



Comment at: llvm/test/Object/archive-extract-dir.test:11
 
-CHECK: foo: {{[Ii]}}s a directory
+CHECK: foo: {{.*}}{{[Ii]}}s a directory{{.*}}

`{{.*}}` at the end of a FileCheck line has literally no benefit - it is 
implicit unless `--match-full-lines` is specified to `FileCheck`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95246

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

[PATCH] D85474: Add -fbinutils-version= to gate ELF features on the specified binutils version

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

In D85474#2398858 , @MaskRay wrote:

> Sent https://lists.llvm.org/pipermail/llvm-dev/2020-November/146676.html "Add 
> -fbinutils-version=" (cross posted to cfe-dev) so that more folks can notice 
> it.
>
> About "generate code that doesn't care about GNU as/ld 
> compatibility/limitation", I am open for suggestions. Currently I like 
> `-fbinutils-version=none` the most (after considering `future` and `latest` 
> and `99` (assuming GNU binutils 99 implements everything we need...))

I like `none`. `latest` has some connotations that we attempt compatibility 
with some specific bleeding edge set of tools, which might not be true, 
especially if they've released at a different time point to us. `none` implies 
there is no specific guarantee for compatibility to me. `99` (or other number) 
seems risky/short-sighted given the way versioning sometimes shoots up versus 
the longetivity of some of these code bases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85474

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

[PATCH] D91229: [FileCheck] Flip -allow-unused-prefixes to false by default

[James Henderson via Phabricator via cfe-commits]

jhenderson added a subscriber: RKSimon.
jhenderson added a comment.

Maybe one way to do this is to do what @RKSimon suggests in the D90281 
, and to enable it on a per-directory basis 
using lit.local.cfg. That would potentially require changing the "0 or 1" 
occurrences limitation of the option, in favour of "last one wins" (or first 
one). Then, you make the change in the lit.local.cfg, flipping the default in 
each directory as you go. Eventually, once all tests that need it are covered, 
you can just change the default in FileCheck itself. How does that sound?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91229

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

[PATCH] D88916: [AMDGPU] Add gfx602, gfx705, gfx805 targets

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: clang/lib/Basic/Cuda.cpp:1
 #include "clang/Basic/Cuda.h"
 

(aside - this file seems to be missing the copyright header - probably should 
be fixed separately though)



Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:1733
 static const EnumEntry ElfHeaderAMDGPUFlags[] = {
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_NONE),
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_R600_R600),

I'm not sure what exactly clang-format is complaining about here, but it might 
be worth reformatting this enum as requested, possibly as a separate commit, I 
don't mind.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88916

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

[PATCH] D78938: Make LLVM build in C++20 mode

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

In D78938#2261411 , @jfb wrote:

> On C++20 mode rotting: it won't if someone sets up a bot. If it rots, then 
> it's easier to un-rot with Barry's patch.

I assume this would be a private bot? It can't be a public bot, since LLVM 
isn't even on C++17, let alone C++20, and so it shouldn't be part of minimum 
requirements that somebody has a compiler that can build C++20. Whilst I 
personally am quite happy with moving LLVM forward, I develop on Windows 
primarily, so don't have the same need to support a long tail of old *nix 
versions etc.

@BRevzin, you should a) mention the u8/const char* issue in the description 
too, and also what compiler you used to build this with. I fully expect at this 
stage that there are some C++20 compilers that might have slightly different 
interpretations of things which this won't resolve, so knowing which one this 
is intended to work with could help with historical research.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78938

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

[PATCH] D78938: Fixing all comparisons for C++20 compilation.

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

In D78938#2259342 , @BRevzin wrote:

> In D78938#2258557 , @jhenderson 
> wrote:
>
>> Not that I have anything particularly against this, but won't this likely 
>> rot fairly rapidly? It's not like LLVM is even on C++17 let alone C++20 yet, 
>> so trying to make it work like the latter when it's just going to break 
>> again seems a bit like wasted effort to me.
>
> People will want to write C++20 programs that use LLVM headers, so I think 
> it's important to help let them do that. Sure, it may rot, but incremental 
> fixes down the line will be smaller.

Makes sense, thanks.




Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:805-817
+  return IsASCII ? "^" : (const char *)u8"\u2548";
 case LineChar::RangeMid:
-  return IsASCII ? "|" : u8"\u2503";
+  return IsASCII ? "|" : (const char *)u8"\u2503";
 case LineChar::RangeEnd:
-  return IsASCII ? "v" : u8"\u253b";
+  return IsASCII ? "v" : (const char *)u8"\u253b";
 case LineChar::LabelVert:
+  return IsASCII ? "|" : (const char *)u8"\u2502";

BRevzin wrote:
> jhenderson wrote:
> > This seems unrelated to comparison checking?
> > This seems unrelated to comparison checking?
> 
> It is unrelated. But In C++20, `u8` literals become their own type so this no 
> longer compiled and I wanted to ensure that I could actually run the tests. 
Could it be a pre-requisite patch then?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78938

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

[PATCH] D78938: Fixing all comparisons for C++20 compilation.

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

Not that I have anything particularly against this, but won't this likely rot 
fairly rapidly? It's not like LLVM is even on C++17 let alone C++20 yet, so 
trying to make it work like the latter when it's just going to break again 
seems a bit like wasted effort to me.




Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:805-817
+  return IsASCII ? "^" : (const char *)u8"\u2548";
 case LineChar::RangeMid:
-  return IsASCII ? "|" : u8"\u2503";
+  return IsASCII ? "|" : (const char *)u8"\u2503";
 case LineChar::RangeEnd:
-  return IsASCII ? "v" : u8"\u253b";
+  return IsASCII ? "v" : (const char *)u8"\u253b";
 case LineChar::LabelVert:
+  return IsASCII ? "|" : (const char *)u8"\u2502";

This seems unrelated to comparison checking?



Comment at: llvm/unittests/ADT/STLExtrasTest.cpp:465
   // Check fancy pointer overload for unique_ptr
+  // Parenthesizing to_address to avoid ADL finding std::to_address
   std::unique_ptr V2 = std::make_unique(0);

Nit: trailing full stop.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78938

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

[PATCH] D87095: [Triple][MachO] Define "arm64e", an AArch64 subarch for Pointer Auth.

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: llvm/lib/Object/MachOObjectFile.cpp:2745
 ArrayRef MachOObjectFile::getValidArchs() {
-  static const std::array validArchs = {{
+  static const std::array validArchs = {{
   "i386",   "x86_64", "x86_64h",  "armv4t",  "arm","armv5e",

Nit: it's probably worth fixing these two Linter issues, since this function is 
small anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87095

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

[PATCH] D85474: Add -fbinutils-version= to gate ELF features on the specified binutils version

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

This looks reasonable to me, but I don't know the surrounding code well enough 
to be comfortable LGTM-ing it. Sorry!




Comment at: llvm/include/llvm/MC/MCAsmInfo.h:387
 
+  // Generated object files can only use features support by GNU ld of this
+  // binutils version.

This sentence implies this specific version, but I think it's meant to be a 
minimum. If so, I'd add "or later" to the end of the sentence.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85474

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

[PATCH] D85474: Add -fbinutils-version= to gate ELF features on the specified binutils version

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

Looks like there's only an `llc` test, but the option is in clang too? Should 
we have a test there?

Is there any documentation for clang this option needs adding to? Also, this 
likely deserves a release note, I feel.




Comment at: clang/include/clang/Driver/Options.td:3307-3310
+def fbinutils_version_EQ : Joined<["-"], "fbinutils-version=">, Group,
+  HelpText<"Produced object files can use ELF features only supported by ld 
newer than the specified version. If"
+  "-fno-integrated-as is specified, produced assembly can use newer ELF 
features. "
+  "'future' means that features not implemented by any known release can be 
used">;

It might be helpful to indicate what the default is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85474

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

[PATCH] D85337: [AMDGPU] gfx1031 target

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:1844
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX1030),
+  LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX1031),
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_XNACK),

rampitec wrote:
> jhenderson wrote:
> > Where's the dedicated llvm-readobj test for this? Please add one. If there 
> > exists one for the other AMDGPU flags, extend that one, otherwise, it might 
> > be best to write one for the whole set at once.
> It is in the lvm/test/CodeGen/AMDGPU/elf-header-flags-mach.ll above along 
> with all other targets.
I emphasise "dedicated". llvm/test/CodeGen... is not the place for tests for 
llvm-readobj code. Such tests belong in llvm/test/tools/llvm-readobj.

To me, the CodeGen test looks like a test of the llc behaviour of generating 
the right value. It uses llvm-readobj, and therefore only incidentally tests 
the dumping behaviour. Imagine a situation where we decided to add support for 
this to llvm-objdump, and then somebody decides to switch the test over to 
using llvm-objdump to match (maybe it "looks better" that way). That would 
result in this code in llvm-readobj being untested.

My opinion, and one I think is shared with others who maintain llvm-readobj 
(amongst other things) is that you need direct testing for llvm-readobj 
behaviour within llvm-readobj tests to avoid such situations. This would 
therefore mean that this change needs two tests:

1) A test in llvm/test/tools/llvm-readobj/ELF which uses a process (e.g. 
yaml2obj) to create the ELF header with the required flag, to ensure the 
dumping behaviour is correct.
2) A test in llvm/test/CodeGen which tests that llc produces the right output 
value in the ELF header flags field (which of course would use llvm-readobj 
currently, but could use anything to verify).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85337

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

[PATCH] D85337: [AMDGPU] gfx1031 target

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:1844
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX1030),
+  LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX1031),
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_XNACK),

Where's the dedicated llvm-readobj test for this? Please add one. If there 
exists one for the other AMDGPU flags, extend that one, otherwise, it might be 
best to write one for the whole set at once.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85337

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

[PATCH] D84473: Dump Accumulator

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

By the way, has the design of this been discussed on llvm-dev? It seems like 
something should have an RFC if it hasn't already.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84473

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

[PATCH] D84473: Dump Accumulator

[James Henderson via Phabricator via cfe-commits]

jhenderson requested changes to this revision.
jhenderson added a comment.
This revision now requires changes to proceed.

I'm assuming you need llvm-readobj to test your changes? In which case, the 
llvm-readobj change needs to come first, and then you can write tests using it 
to test this change (at the moment, the testing for this seems insufficient to 
me). Do you have a patch yet for the llvm-readobj side?

Could you add some documentation somewhere, either in code comments or 
somewhere else, explaining the output format you actually want this to be? From 
my casual reading (I'm not the most knowledgeable of people in this area), it 
looks like you're creating an ELF SHT_NOTE section, but don't use the SHT_NOTE 
ELF format (see 
https://www.sco.com/developers/gabi/latest/ch5.pheader.html#note_section for 
details). I would actually expect this to emit a section with a new section 
type, so that the consumer (llvm-readobj) can more easily identify it. 
Alternatively, it could just be a basic SHT_NOTE section, with a specific note 
type. Under no circumstances should consumers need to check the section header 
name to identify the section (string comparisons are expensive, and should be 
avoided where possible - the general intent for ELF is for different kinds of 
sections to be distinguished by their types and flags).




Comment at: llvm/include/llvm/Analysis/DumpAccumulator.h:56
+
+#include 
+

Spurious include?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84473

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

[PATCH] D84473: Dump Accumulator

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

Could the llvm-readobj bit be spun off into a separate review, please. As 
things stand it doesn't have any of its own testing, which should be added at 
the same time as adding support in the tool itself. The new option is also 
lacking any documentation in the llvm-readobj and llvm-readelf docs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84473

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

[PATCH] D83912: [llvm-readobj] Update tests because of changes at llvm-readobj behavior

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

Hi @Elvina,

Just to let you know that I had to fix up three clang tests that were using the 
old behaviour too, prior to committing. I also noticed that you've got the 
stack the wrong way around - you needed to fix the tests before changing the 
behaviour in this case to avoid breaking the codebase, even if it was only 
briefly.

Anyway, these are now committed. I'll update the bug.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83912



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

[PATCH] D81672: [Driver] When forcing a crash print the bug report message

[James Henderson via Phabricator via cfe-commits]

jhenderson accepted this revision.
jhenderson added a comment.

Latest version LGTM too.


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

https://reviews.llvm.org/D81672



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

[PATCH] D81672: [Driver] When forcing a crash print the bug report message

[James Henderson via Phabricator via cfe-commits]

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

LGTM, but wait for others too, please.


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

https://reviews.llvm.org/D81672



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

[PATCH] D81672: [Driver] When forcing a crash call abort to get the correct diagnostic

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

In D81672#2101513 , @aganea wrote:

> But that won't work when compiling & crashing with `-fno-integrated-cc1`, 
> would it? (or if building with `cmake ... -DCLANG_SPAWN_CC1=1`). In that 
> case, normal crashes (not -gen-reproducer) won't go through the 
> `CrashRecoveryContext`.
>  Try running:
>
>   $ CCC_OVERRIDE_OPTIONS=+-fno-integrated-cc1
>   $ py your_build_folder/bin/llvm-lit.py -vv -a 
> clang/test/Driver/crash-report.c
>
>
> See if the commands issued by the test display the "PLEASE submit a bug 
> report" message.


If I'm following it correctly (I could easily not be), the latest version of 
the patch will also no longer print the bug report submission message for 
crashes in other parts of LLVM, not just clang, which was rather the motivation 
of @gbreynoo's recent changes.


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

https://reviews.llvm.org/D81672



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

[PATCH] D81672: [Driver] When forcing a crash call abort to get the correct diagnostic

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

This seems sensible to me, but I have zero experience with the 
`CrashRecoveryContext` class. It might be best to get someone who has more 
knowledge of it to look at it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81672



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