[PATCH] D42758: Support `#pragma comment(lib, "name")` in the frontend for ELF

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

In https://reviews.llvm.org/D42758#997880, @erichkeane wrote:

> I'd like to see @probinson s PS4 discussion bottom out, but I don't see any 
> reason to hold this up otherwise.


The PS4 compiler uses its linker options mechanism to communicate three 
different things to the linker, namely `#pragma comment(lib, ...)` directives, 
as discussed here, symbols marked with `__declspec(dllexport)`, and symbols 
marked with `__declspec(dllimport)` (the latter two requesting the linker 
explicitly to export/import the decorated symbols). I think if all three of 
these are converted to linker commands, we should be able to adopt the new 
implementation.


Repository:
  rC Clang

https://reviews.llvm.org/D42758



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

[PATCH] D59746: [LibTooling] Fix '-h' option

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

Just chiming in from a LLVM binutils developer perspective. Some of the 
binutils are missing -h as an alias, when they really should have it for 
compatibility (specifically I checked llvm-nm and llvm-symbolizer). As a 
result, a solution that auto-adds -h as an alias would be good, as long as we 
have the ability to override it somehow. I wouldn't mind having logic in 
llvm-objdump/llvm-readobj to explicitly override the short alias if it is 
required.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59746



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

[PATCH] D59746: [CommandLineParser] Add DefaultOption flag

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

I've not looked at the implementation in depth, but cl::DefaultOption seems 
like a nice way of handling this. Please make sure that there is testing in 
llvm-readobj and llvm-objdump that test their own short alias interpretation 
somewhere though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59746



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

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] D77683: [Docs] Make code review policy clearer about requested pre-commit reviews

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

My impression is that you shouldn't be making changes in code you are not 
already familiar with without a review, even if the fix seems fairly obvious 
(it's surprising how often an "obvious" fix isn't actually the right thing to 
do). I'd have assumed this was covered by "any uncertainty" already. Presumably 
to get familiarity with said area of code, you'd already have prior art in 
getting/giving reviews of a given area, so would know the local culture?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77683



___
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

[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 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 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] D74813: [RFC] Add hash of block contents to function block names

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: llvm/test/tools/llvm-cxxfilt/invalid.test:1
-RUN: llvm-cxxfilt -n _Z1fi __Z1fi f ___ZSt1ff_block_invoke | FileCheck %s
+RUN: llvm-cxxfilt -n _Z1fi __Z1fi f ___ZSt1ff_block_invoke 
___Z7my_mainv_block_invoke_ZTSi_ZTSj_ZTSPVK3sss | FileCheck %s
 

I'm not sure this test needs updating with an additional case?

This test is just to show what llvm-cxxfilt prints when an input is not a valid 
mangled name (I think). I think the existing block_invoke example is there 
merely to show triple underscores can be handled, but I actually am doubtful 
that it really belongs here, as that sounds more like belonging in the 
demangler testing.

NB. I haven't actually looked at the history of this test or the source code to 
verify anything I just said.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74813



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

[PATCH] D79276: [FileCheck] Support comment directives

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

I hesitate to suggest this, but is it worth using `REM` as a comment prefix? 
It's the comment marker for Windows batch files, so has some precedence.




Comment at: llvm/docs/CommandGuide/FileCheck.rst:57
+ By default, FileCheck ignores any occurrence in ``match-filename`` of any 
check
+ prefix if it is preceded on the same line by "``COM:``" or "``RUN:``".  See 
the
+ section `The "COM:" directive`_ for usage details.

Nit: the rest of this document uses single spaces after full stops. Please do 
that in the new text too.



Comment at: llvm/lib/Support/FileCheck.cpp:1930
 
+bool FileCheck::ValidateCheckPrefixes() {
+  StringSet<> PrefixSet;

It looks to me like the changes related to this function could be done 
separately (perhaps first). Is that the case, and if so, could you do so? It's 
a little hard to follow what's just refactoring of the existing stuff and what 
is new, with the extra comment stuff thrown in.



Comment at: llvm/test/FileCheck/comment.txt:1
+// Comment directives successfully comment out check directives.
+//

You don't need to prefix the lines with '//' or anything else for that matter, 
since this isn't being used as an assembly/yaml/C/etc input.

If you do want to have the characters (I don't care either way), I'd prefer '#' 
for RUN/CHECK directive lines and '##' for comments (the latter so they stand 
out better from directive lines). It's fewer characters, whilst still as 
explicit ('#' is used widely in some parts of the testsuite at least).



Comment at: llvm/test/FileCheck/comment.txt:2
+// Comment directives successfully comment out check directives.
+//
+// Check all default comment prefixes.

Don't prefix empty lines.



Comment at: llvm/test/FileCheck/comment.txt:4
+// Check all default comment prefixes.
+// Check that a comment directive at the begin/end of the file is handled.
+// Check that the preceding/following line's check directive is not affected.

begin -> beginning



Comment at: llvm/test/FileCheck/comment.txt:9
+// RUN: echo 'CHECK: foo'>> %t.chk
+// RUN: echo 'RUN:echo "CHECK: baz"' >> %t.chk
+// RUN: %ProtectFileCheckOutput FileCheck -vv %t.chk < %t.in 2>&1 \

Is the lack of space after 'RUN:' deliberate?



Comment at: llvm/test/FileCheck/comment.txt:10-11
+// RUN: echo 'RUN:echo "CHECK: baz"' >> %t.chk
+// RUN: %ProtectFileCheckOutput FileCheck -vv %t.chk < %t.in 2>&1 \
+// RUN: | FileCheck -check-prefix=SUPPRESSES-CHECKS -DCHECK_LINE=2 %s
+//

I have a personal preference for multi-line commands like this to be formatted 
like this:

```
// RUN:   ... | \
// RUN: ...
```

with the `|` and `\` on the same line, to show that the next line is the start 
of a new command (as opposed to just being more arguments for that line), and 
the extra two space indentation showing that the line is a continuation of the 
previous.



Comment at: llvm/test/FileCheck/comment.txt:15
+// Check that a comment directive not at the beginning of a line is handled.
+// RUN: echo 'foo'  >  %t.in
+// RUN: echo 'CHECK: foo'   >  %t.chk

Not a big deal, but it might be easier for debugging the test in the future if 
the %t.in (and %t.chk) of each case is named differently, e.g. %t2.in, %t2.chk 
etc. That way, the later ones won't overwrite the earlier ones.



Comment at: llvm/test/FileCheck/comment.txt:38-43
+// RUN: echo 'foo COM: bar'> %t.in
+// RUN: echo 'CHECK: foo COM: bar' > %t.chk
+// RUN: %ProtectFileCheckOutput FileCheck -vv %t.chk < %t.in 2>&1 \
+// RUN: | FileCheck -check-prefix=WITHIN-CHECKS %s
+//
+// WITHIN-CHECKS: .chk:1:8: remark: CHECK: expected string found in input

I'm struggling a little with this case. Firstly, why the '8' in the column 
count in the remark? Secondly, if COM: was being treated as a genuine comment 
here, then the check directive would become `CHECK: foo` which would still be a 
match of the input, if I'm not mistaken? I guess the two might somehow be 
related, but I don't know how if so.



Comment at: llvm/test/FileCheck/comment.txt:127
+// RUN: %ProtectFileCheckOutput not FileCheck /dev/null < /dev/null 2>&1 \
+// RUN:  -comment-prefixes= \
+// RUN: | FileCheck -check-prefix=PREFIX-EMPTY %s

Maybe worth cases like "-comment-prefixes=,FOO", "-comment-prefixes=FOO," and 
"-comment-prefixes=FOO,,BAR".



Comment at: llvm/test/FileCheck/comment.txt:133
+// RUN: %ProtectFileCheckOutput not FileCheck /dev/null < /dev/null 2>&1 \
+// RUN:   -comment-prefixes=. \
+// RUN: | FileCheck -check-prefix=PREFIX-BAD-CHAR %s
-

[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] 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] 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] D79276: [FileCheck] Support comment directives

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: llvm/docs/CommandGuide/FileCheck.rst:57
+ By default, FileCheck ignores any occurrence in ``match-filename`` of any 
check
+ prefix if it is preceded on the same line by "``COM:``" or "``RUN:``".  See 
the
+ section `The "COM:" directive`_ for usage details.

jdenny wrote:
> jhenderson wrote:
> > Nit: the rest of this document uses single spaces after full stops. Please 
> > do that in the new text too.
> It doesn't matter to me which we use, especially given that it doesn't seem 
> to affect how the HTML renders.  However, the majority of periods (ignoring 
> `..` marking blocks) in this document that are followed by at least one space 
> are followed by two spaces. Does that change your suggestion?
Ah, it looks like it depends on who wrote the respective section. I know 
@probinson prefers double-spaces, and he wrote a lot of this stuff originally. 
I personally prefer single spaces (why waste extra characters etc etc). As the 
document is already inconsistent, I don't mind. When I made my original 
comment, I obviously didn't look at the right bits of the docs.



Comment at: llvm/lib/Support/FileCheck.cpp:1920
+  errs() << "error: supplied " << Kind << " prefix must be unique among "
+ << "check prefixes and comment prefixes: '" << Prefix << "'\n";
   return false;

This can probably be "check and comment prefixes"



Comment at: llvm/lib/Support/FileCheck.cpp:1928
 static const char *DefaultCheckPrefixes[] = {"CHECK"};
+static const char *DefaultCommentPrefixes[] = {"COM", "RUN"};
 

Similar to what I mentioned in D79375, perhaps it would make more sense for COM 
and RUN to be defined by the tool itself rather than the library to allow users 
to customise their respective front-ends as they wish.



Comment at: llvm/lib/Support/FileCheck.cpp:1970
+  for (StringRef Prefix : Req.CommentPrefixes) {
+PrefixRegexStr.push_back('|');
+PrefixRegexStr.append(Prefix);

This is incredibly nit-picky, but perhaps this should have a check for 
`PrefixRegexStr.empty()`. It's such an edge case that it probably doesn't 
matter, but assuming you adopt my comment about the default prefixes being 
defined by the tool, you could imagine a situation where a different front-end 
defined no check prefixes, and the user then didn't specify any either.



Comment at: llvm/test/FileCheck/comment.txt:1
+// Comment directives successfully comment out check directives.
+//

jdenny wrote:
> jhenderson wrote:
> > You don't need to prefix the lines with '//' or anything else for that 
> > matter, since this isn't being used as an assembly/yaml/C/etc input.
> > 
> > If you do want to have the characters (I don't care either way), I'd prefer 
> > '#' for RUN/CHECK directive lines and '##' for comments (the latter so they 
> > stand out better from directive lines). It's fewer characters, whilst still 
> > as explicit ('#' is used widely in some parts of the testsuite at least).
> I don't much care either. The FileCheck test suite is not consistent in this 
> regard.
> 
> What about the following style, making use of `COM:` (or `REM:` or whatever 
> we choose) for comments as that's it's purpose:
> 
> ```
> COM: -
> COM: Comment directives successfully comment out check directives.
> COM: -
> 
> COM: Check all default comment prefixes.
> COM: Check that a comment directive at the begin/end of the file is handled.
> COM: Check that the preceding/following line's check directive is not 
> affected.
> RUN: echo 'foo'   >  %t.in
> RUN: echo 'COM: CHECK: bar'   >  %t.chk
> RUN: echo 'CHECK: foo'>> %t.chk
> RUN: echo 'RUN:echo "CHECK: baz"' >> %t.chk
> RUN: %ProtectFileCheckOutput FileCheck -vv %t.chk < %t.in 2>&1 \
> RUN: | FileCheck -check-prefix=SUPPRESSES-CHECKS -DCHECK_LINE=2 %s
> 
> COM: Check the case of one user-specified comment prefix.
> COM: Check that a comment directive not at the beginning of a line is handled.
> RUN: echo 'foo'  >  %t.in
> RUN: echo 'CHECK: foo'   >  %t.chk
> RUN: echo 'letters then space MY-PREFIX: CHECK: bar' >> %t.chk
> RUN: %ProtectFileCheckOutput \
> RUN: FileCheck -vv %t.chk -comment-prefixes=MY-PREFIX < %t.in 2>&1 \
> RUN: | FileCheck -check-prefix=SUPPRESSES-CHECKS -DCHECK_LINE=1 %s
> 
> COM: Check the case of multiple user-specified comment prefixes.
> RUN: echo 'foo'   >  %t.in
> RUN: echo 'Bar_2: CHECK: Bar' >  %t.chk
> RUN: echo 'CHECK: foo'>> %t.chk
> RUN: echo 'Foo_1: CHECK: Foo' >> %t.chk
> RUN: echo 'Baz_3: CHECK: Baz' >> %t.chk
> RUN: %ProtectFileCheckOutput \
> RUN: FileCheck -vv %t.chk -comment-prefixes=F

[PATCH] D79276: [FileCheck] Support comment directives

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: llvm/lib/Support/FileCheck.cpp:1928
 static const char *DefaultCheckPrefixes[] = {"CHECK"};
+static const char *DefaultCommentPrefixes[] = {"COM", "RUN"};
 

jdenny wrote:
> jhenderson wrote:
> > Similar to what I mentioned in D79375, perhaps it would make more sense for 
> > COM and RUN to be defined by the tool itself rather than the library to 
> > allow users to customise their respective front-ends as they wish.
> I suggested there that we handle the check prefix side of that change in a 
> separate patch.  Perhaps the same patch could handle the comment prefix side 
> of it.
> 
> Do you see a reason to handle this before committing the current patches?
I don't, as there's no current use-case for it. Let's leave it as-is for now.



Comment at: llvm/test/FileCheck/comment/bad-comment-prefix.txt:36
+RUN: %ProtectFileCheckOutput not FileCheck /dev/null < /dev/null 2>&1 \
+RUN:  -check-prefixes=FOO,COM | \
+RUN:   FileCheck -check-prefix=CHECK-PREFIX-DUP-COMMENT %s

Perhaps worth checking against RUN as well as COM?



Comment at: llvm/test/FileCheck/comment/blank-comments.txt:9
+
+CHECK: .chk:2:8: remark: CHECK: expected string found in input

I'm assuming that FileCheck treats the entire line after the first `CHECK:` 
here as part of the CHECK, and doesn't try to start a second CHECK directive?



Comment at: llvm/test/FileCheck/comment/suffixes.txt:1-2
+# Comment prefixes plus check directive suffixes are not comment directives
+# and are treated as plain text.
+

I don't think you should change anything here, but if I'm following this right, 
this leads to the amusing limitation that you can "comment out" (via 
--comment-prefixes) any CHECK lines that use a suffix (CHECK-NEXT, CHECK-NOT 
etc) but not those that don't without changing --check-prefixes value!

By the way, do we need to have a test-case for that? I.e. that 
--comment-prefixes=CHECK-NEXT disables the CHECK-NEXT lines (assuming it does 
of course)?



Comment at: llvm/test/FileCheck/comment/suppresses-checks.txt:1
+Comment directives successfully comment out check directives.
+

Missing '#'


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

https://reviews.llvm.org/D79276



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

[PATCH] D79276: [FileCheck] Support comment directives

[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 might not hurt to have someone else looking at it.




Comment at: llvm/test/FileCheck/comment/bad-comment-prefix.txt:40
+RUN:  -check-prefixes=RUN,FOO | \
+RUN:   FileCheck -check-prefix=CHECK-PREFIX-DUP-RUN_ %s
+CHECK-PREFIX-DUP-COM: error: supplied check prefix must be unique among check 
and comment prefixes: 'COM'

I'm guessing the underscore is to circumvent lit trying to run things? If so, I 
think we need to fix lit to only run lines where the RUN: is at the start of 
the line (ignoring whitespace and non alpha-numerics). I don't think anything 
else makes sense.

Not related to this patch of course, so nothing to do here, unless my guess is 
incorrect.



Comment at: llvm/test/FileCheck/comment/suffixes.txt:1-2
+# Comment prefixes plus check directive suffixes are not comment directives
+# and are treated as plain text.
+

jdenny wrote:
> jhenderson wrote:
> > I don't think you should change anything here, but if I'm following this 
> > right, this leads to the amusing limitation that you can "comment out" (via 
> > --comment-prefixes) any CHECK lines that use a suffix (CHECK-NEXT, 
> > CHECK-NOT etc) but not those that don't without changing --check-prefixes 
> > value!
> > 
> > By the way, do we need to have a test-case for that? I.e. that 
> > --comment-prefixes=CHECK-NEXT disables the CHECK-NEXT lines (assuming it 
> > does of course)?
> > I don't think you should change anything here, but if I'm following this 
> > right, this leads to the amusing limitation that you can "comment out" (via 
> > --comment-prefixes) any CHECK lines that use a suffix (CHECK-NEXT, 
> > CHECK-NOT etc) but not those that don't without changing --check-prefixes 
> > value!
> 
> That's right, but check prefixes have this problem too.  That is, you can do 
> things like `-check-prefixes=FOO,FOO-NOT` so that `FOO-NOT` is not negative.
> 
> `ValidatePrefixes` should be extended to catch such cases, I think.  But in a 
> separate patch.
> 
> > By the way, do we need to have a test-case for that? I.e. that 
> > --comment-prefixes=CHECK-NEXT disables the CHECK-NEXT lines (assuming it 
> > does of course)?
> 
> Hmm.  I think it's behavior we don't want to support.  Maybe the test case 
> should be added when extending `ValidatePrefixes` as I described above?
> 
I agree it's separate work. FWIW, I just came up with a genuinely useful 
use-case for it with CHECK directives, but it might just be unnecessary. 
Imagine the case where you want a test where some specific output is printed 
under one condition and not another condition. You'd want something like:

```
# RUN: ... | FileCheck %s --check-prefix=WITH
# RUN: ... | FileCheck %s --check-prefix=WITHOUT

# WITH: some text that should be matched
# WITHOUT-NOT: some text that should be matched
```

A careleses developer could change the text of "WITH" to match some new 
behaviour without changing "WITHOUT-NOT", thus breaking the second case. You 
could instead do:

```
# RUN: ... | FileCheck %s --check-prefix=CHECK-NOT
# RUN: ... | FileCheck %s

# CHECK-NOT: some text that should be matched
```
Then, if the output changed, you'd update both the regular and NOT match. I 
might have used this pattern in a few tests in the past had it occurred to me.

Anyway, I think there are other ways of solving that problem, although not 
without work on FileCheck (I've been prototyping a method with only limited 
success so far), and I agree it's otherwise mostly horrible, so I'm not 
seriously opposing the suggestion.


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

https://reviews.llvm.org/D79276



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

[PATCH] D79276: [FileCheck] Support comment directives

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: llvm/test/FileCheck/comment/suffixes.txt:1-2
+# Comment prefixes plus check directive suffixes are not comment directives
+# and are treated as plain text.
+

jdenny wrote:
> jhenderson wrote:
> > jdenny wrote:
> > > jhenderson wrote:
> > > > I don't think you should change anything here, but if I'm following 
> > > > this right, this leads to the amusing limitation that you can "comment 
> > > > out" (via --comment-prefixes) any CHECK lines that use a suffix 
> > > > (CHECK-NEXT, CHECK-NOT etc) but not those that don't without changing 
> > > > --check-prefixes value!
> > > > 
> > > > By the way, do we need to have a test-case for that? I.e. that 
> > > > --comment-prefixes=CHECK-NEXT disables the CHECK-NEXT lines (assuming 
> > > > it does of course)?
> > > > I don't think you should change anything here, but if I'm following 
> > > > this right, this leads to the amusing limitation that you can "comment 
> > > > out" (via --comment-prefixes) any CHECK lines that use a suffix 
> > > > (CHECK-NEXT, CHECK-NOT etc) but not those that don't without changing 
> > > > --check-prefixes value!
> > > 
> > > That's right, but check prefixes have this problem too.  That is, you can 
> > > do things like `-check-prefixes=FOO,FOO-NOT` so that `FOO-NOT` is not 
> > > negative.
> > > 
> > > `ValidatePrefixes` should be extended to catch such cases, I think.  But 
> > > in a separate patch.
> > > 
> > > > By the way, do we need to have a test-case for that? I.e. that 
> > > > --comment-prefixes=CHECK-NEXT disables the CHECK-NEXT lines (assuming 
> > > > it does of course)?
> > > 
> > > Hmm.  I think it's behavior we don't want to support.  Maybe the test 
> > > case should be added when extending `ValidatePrefixes` as I described 
> > > above?
> > > 
> > I agree it's separate work. FWIW, I just came up with a genuinely useful 
> > use-case for it with CHECK directives, but it might just be unnecessary. 
> > Imagine the case where you want a test where some specific output is 
> > printed under one condition and not another condition. You'd want something 
> > like:
> > 
> > ```
> > # RUN: ... | FileCheck %s --check-prefix=WITH
> > # RUN: ... | FileCheck %s --check-prefix=WITHOUT
> > 
> > # WITH: some text that should be matched
> > # WITHOUT-NOT: some text that should be matched
> > ```
> > 
> > A careleses developer could change the text of "WITH" to match some new 
> > behaviour without changing "WITHOUT-NOT", thus breaking the second case. 
> > You could instead do:
> > 
> > ```
> > # RUN: ... | FileCheck %s --check-prefix=CHECK-NOT
> > # RUN: ... | FileCheck %s
> > 
> > # CHECK-NOT: some text that should be matched
> > ```
> > Then, if the output changed, you'd update both the regular and NOT match. I 
> > might have used this pattern in a few tests in the past had it occurred to 
> > me.
> > 
> > Anyway, I think there are other ways of solving that problem, although not 
> > without work on FileCheck (I've been prototyping a method with only limited 
> > success so far), and I agree it's otherwise mostly horrible, so I'm not 
> > seriously opposing the suggestion.
> I agree the use case is important, and I also agree there must be a better 
> solution.
> 
> The underlying issue is that we want to reuse a pattern.  Perhaps there 
> should be some way to define a FileCheck variable once and reuse it among 
> multiple FileCheck commands. For example:
> 
> ```
> # RUN: ... | FileCheck %s --check-prefix=WITH,ALL
> # RUN: ... | FileCheck %s --check-prefix=WITHOUT,ALL
> 
> # ALL-DEF: [[VAR:some text that should be matched]]
> # WITH: [[VAR]]
> # WITHOUT-NOT: [[VAR]]
> ```
> 
> It should probably be possible to use regexes in such a variable.  I'm not 
> sure if that's possible now.  It might require a special variable type.  We 
> currently have `#` to indicate numeric variables.  Perhaps `~` would indicate 
> pattern variables.
That's almost exactly what I've been prototyping on-and-off over the past few 
months, but I've been running into various ordering issues, which I haven't yet 
solved to my satisfaction. My original thread that inspired the idea is 
http://lists.llvm.org/pipermail/llvm-dev/2020-January/138822.html.


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

https://reviews.llvm.org/D79276



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

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

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 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: [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] 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] 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 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] 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] 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#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] 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.

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

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

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] 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] 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] 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] 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 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 "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] 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] 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] 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] 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] 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 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 envir

[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 variable

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

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 OldAr

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

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 &EI) {

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] D76572: Replace `T(x)` with `reinterpret_cast(x)` everywhere it means reinterpret_cast. No functional change

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

@Quuxplusone - did you see that there are several clang-format warnings 
reported against these changes? Please could you fix them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76572



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

[PATCH] D76572: Replace `T(x)` with `reinterpret_cast(x)` everywhere it means reinterpret_cast. No functional change

[James Henderson via Phabricator via cfe-commits]

jhenderson accepted this revision.
jhenderson added a comment.

LGTM, with nit, with regards to the llvm-readobj and libObject parts. I haven't 
looked at the other bits though.




Comment at: llvm/lib/CodeGen/AsmPrinter/OcamlGCPrinter.cpp:150
+ ">= 65536.\n"
+ "(" +
+ Twine(reinterpret_cast(&FI)) + ")");

Nit: I think you can fold these two string literals together. Not sure why they 
were separate before.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76572



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

[PATCH] D76572: Replace `T(x)` with `reinterpret_cast(x)` everywhere it means reinterpret_cast. No functional change

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

In D76572#1942024 , @Quuxplusone wrote:

> Btw, @jhenderson and @rsmith, if you wanted to land your own files' pieces of 
> this patch while waiting on whoever-it-is to approve the pieces in other 
> files, that'd be cool. As I mentioned initially, I don't have the access 
> necessary to land any part of it myself.


From my point of view, I don't see any particular rush to land this, so let's 
just keep it all as one, and wait for the other bits to be reviewed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76572



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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

Are there any instances where we DON'T want to get the real file system? If 
not, could the `*llvm::vfs::getRealFileSystem()` call be put inside 
`cl::ExpandResponseFiles`?




Comment at: llvm/include/llvm/Support/CommandLine.h:32
 #include "llvm/Support/ManagedStatic.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"

Can llvm::vfs::FileSystem be forward declared and this moved into the .cpp?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769



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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

In D70769#1761428 , @lh123 wrote:

> In D70769#1761394 , @jhenderson 
> wrote:
>
> > Are there any instances where we DON'T want to get the real file system? If 
> > not, could the `*llvm::vfs::getRealFileSystem()` call be put inside 
> > `cl::ExpandResponseFiles`?
>
>
> This patch is part of D70222 .
>
>   This is for using InMemoryFileSystem in unit tests.


Okay, that makes sense. Perhaps we should introduce a new overload of 
`ExpandResponseFiles` that allows specifying a different file system then, and 
having the current version call into that one, specifying the getRealFileSystem 
call? I'm not overly keen on having yet another apparently boiler-plate piece 
of functionality required in every single call of `ExpandResponseFiles`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769



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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

In D70769#1761517 , @sammccall wrote:

> It's not just unit-tests, in D70222  we will 
> ultimately use FSes other than getRealFilesystem() in clangd.


I see, thank you.

> Having non-VFS-clean versions of functions around that silently use the real 
> FS is a bit of a scourge for users that need to be VFS-clean, honestly. 
> Parsing argv is exactly the sort of place where FS access isn't obvious and 
> exposing a function that doesn't accept a VFS encourages callers to miss this 
> aspect. At the same time this is mostly called from a bunch of drivers that 
> (presumably) don't need VFS support. Maybe allowing nullptr = real FS, or a 
> default argument, would be an OK compromise?

A default argument would be my preference there. It looks like we have some 
already.




Comment at: llvm/include/llvm/Support/CommandLine.h:1963
 /// \param [in] Tokenizer Tokenization strategy. Typically Unix or Windows.
+/// \param [in] FS VFS used for all file accesses when running the tool.
 /// \param [in,out] Argv Command line into which to expand response files.

Does `VFS` make sense here, given that it could be a real filesystem?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769



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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

Aside from one nit, this looks okay to me. I'd like someone else to look at it 
though too. I'm not at all familiar with the file system stuff to be confident 
to say everything there is good.




Comment at: llvm/lib/Support/CommandLine.cpp:1148
+  SmallVector RHSPath;
+  if (FS.getRealPath(FName, LHSPath) || FS.getRealPath(RFile.File, 
RHSPath)) {
+return false;

Has this been clang-formatted?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769



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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: llvm/lib/Support/CommandLine.cpp:1148
+  llvm::ErrorOr RHS = FS.status(RFile.File);
+  if (!LHS || !RHS) {
+return false;

kadircet wrote:
> again you need to `consumeError`s before returning. I would first get `LHS`, 
> consume and bail out if it was an error, then do the same for `RHS` and only 
> after that return `equivalent`.
Could you add a TODO comment here and at the other `consumeError()` call to say 
that the error should be propagated up the stack, please?



Comment at: llvm/lib/Support/CommandLine.cpp:1054
+  if (!CurrDirOrErr) {
+llvm::consumeError(llvm::errorCodeToError(CurrDirOrErr.getError()));
 return false;

I'd ultimately like to see the public interface return 
llvm::Expected/llvm::Error so that a potentially useful error with information 
isn't just thrown away. However, I agree that that's probably a separate 
change. On the other hand, it should be simple to return it here, since this is 
only used locally, for consumption in `ExpandResponseFiles` below. Could you 
make that change, please?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769



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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: llvm/lib/Support/CommandLine.cpp:1071
+  return llvm::make_error(
+  "Could not convert UTF16 To UTF8", llvm::inconvertibleErrorCode());
 Str = StringRef(UTF8Buf);

Not sure, but you might want to use `illegal_byte_sequence` here instead of 
`llvm::inconvertibleErrorCode()`.



Comment at: llvm/lib/Support/CommandLine.cpp:1178
+
+if (llvm::Error Err = ExpandResponseFile(FName, Saver, Tokenizer, 
ExpandedArgv, MarkEOLs,
+RelativeNames, FS)) {

clang-format please


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769



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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: llvm/lib/Support/CommandLine.cpp:1069
+  return llvm::createStringError(
+  std::make_error_code(std::errc::illegal_byte_sequence),
+  "Could not convert UTF16 To UTF8");

`std::make_error_code(std::errc::illegal_byte_sequence)` -> 
`errc::illegal_byte_sequence`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769



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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: llvm/lib/Support/CommandLine.cpp:1069
+  return llvm::createStringError(
+  std::make_error_code(std::errc::illegal_byte_sequence),
+  "Could not convert UTF16 To UTF8");

jhenderson wrote:
> `std::make_error_code(std::errc::illegal_byte_sequence)` -> 
> `errc::illegal_byte_sequence`
LLVM has its own errc error_code set. Please use that, i.e. delete the `std::` 
(note that I didn't add `std::` in my previous comment).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769



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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: llvm/lib/Support/CommandLine.cpp:1069
+  return llvm::createStringError(
+  std::make_error_code(std::errc::illegal_byte_sequence),
+  "Could not convert UTF16 To UTF8");

lh123 wrote:
> jhenderson wrote:
> > jhenderson wrote:
> > > `std::make_error_code(std::errc::illegal_byte_sequence)` -> 
> > > `errc::illegal_byte_sequence`
> > LLVM has its own errc error_code set. Please use that, i.e. delete the 
> > `std::` (note that I didn't add `std::` in my previous comment).
> I think we should use `std::errc::illegal_byte_sequence`, because it's 
> sigature is:
>  ```
> template 
> inline Error createStringError(std::errc EC, char const *Fmt, const Ts &... 
> Vals)
> ```
I'm not sure if that's a mistake in the function interface or not, and it looks 
like our usage is very inconsistent, but the comments in Errc.h imply that we 
really should use llvm::errc values and not std::errc, or there may be problems.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769



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