[Lldb-commits] [PATCH] D58930: Add XCOFF triple object format type for AIX

[Hubert Tong via Phabricator via lldb-commits]

hubert.reinterpretcast added inline comments.



Comment at: llvm/lib/Support/Triple.cpp:537
   return StringSwitch(EnvironmentName)
+// FIXME: We have to put XCOFF before COFF;
+// perhaps an order-independent pattern matching is desired?

If the conclusion is that checking XCOFF before COFF is fine, then we should 
remove the FIXME and just leave a normal comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58930



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

[Lldb-commits] [PATCH] D58930: Add XCOFF triple object format type for AIX

[Hubert Tong via Phabricator via lldb-commits]

hubert.reinterpretcast added inline comments.



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:2079
+  if (log)
+log->Printf("sorry: unimplemented for XCOFF");
+  return false;

JDevlieghere wrote:
> jasonliu wrote:
> > JDevlieghere wrote:
> > > jasonliu wrote:
> > > > apaprocki wrote:
> > > > > No need to be `sorry:` :) This should probably just say `error: XCOFF 
> > > > > is unimplemented` to be more direct in case anything is expecting 
> > > > > "error:" in the output.
> > > > Sure. Will address in next revision.
> > > Just bundle this with the WASM case, the error message is correct for 
> > > both.
> > I think they are different. 
> > The error message for WASM seems to suggest that it will never ever get 
> > supported on WASM. 
> > But it is not the case for XCOFF, we want to indicate that it is not 
> > implemented yet.  
> I don't think the error message suggests that at all, and it's definitely not 
> true. At this point neither XCOFF nor WASM is supported, and that's exactly 
> what the log message says.
> 
I agree that the error message for WASM does not indicate that the lack of 
support is inherent or intended to be permanent; however, it is not indicative 
either of an intent to implement the support. I am not sure what the intent is 
for WASM, but I do know that the intent for XCOFF is to eventually implement 
the support. I do not see how using an ambiguous message in this commit (when 
we know what the intent is) is superior to the alternative of having an 
unambiguous message.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58930



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

[Lldb-commits] [PATCH] D58930: Add XCOFF triple object format type for AIX

[Hubert Tong via Phabricator via lldb-commits]

hubert.reinterpretcast added inline comments.



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:2079
+  if (log)
+log->Printf("sorry: unimplemented for XCOFF");
+  return false;

davide wrote:
> hubert.reinterpretcast wrote:
> > JDevlieghere wrote:
> > > jasonliu wrote:
> > > > JDevlieghere wrote:
> > > > > jasonliu wrote:
> > > > > > apaprocki wrote:
> > > > > > > No need to be `sorry:` :) This should probably just say `error: 
> > > > > > > XCOFF is unimplemented` to be more direct in case anything is 
> > > > > > > expecting "error:" in the output.
> > > > > > Sure. Will address in next revision.
> > > > > Just bundle this with the WASM case, the error message is correct for 
> > > > > both.
> > > > I think they are different. 
> > > > The error message for WASM seems to suggest that it will never ever get 
> > > > supported on WASM. 
> > > > But it is not the case for XCOFF, we want to indicate that it is not 
> > > > implemented yet.  
> > > I don't think the error message suggests that at all, and it's definitely 
> > > not true. At this point neither XCOFF nor WASM is supported, and that's 
> > > exactly what the log message says.
> > > 
> > I agree that the error message for WASM does not indicate that the lack of 
> > support is inherent or intended to be permanent; however, it is not 
> > indicative either of an intent to implement the support. I am not sure what 
> > the intent is for WASM, but I do know that the intent for XCOFF is to 
> > eventually implement the support. I do not see how using an ambiguous 
> > message in this commit (when we know what the intent is) is superior to the 
> > alternative of having an unambiguous message.
> I think we should keep this consistent with the other target so my vote is 
> for grouping XCOFF with WASM. After all, if it's going to be implemented 
> soon, the message will go away :)
Well, I don't know about "soon"...
Using the WASM message for XCOFF is not actually wrong; so, I can be okay with 
it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58930



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

[Lldb-commits] [PATCH] D58930: Add XCOFF triple object format type for AIX

[Hubert Tong via Phabricator via lldb-commits]

hubert.reinterpretcast added a comment.

@jasonliu, you have had a number of patches committed into the project already 
(D22698 , D22702 
, D34649 ). 
Please go ahead with requesting commit access, and commit this patch with the 
additional fix when you are ready.




Comment at: llvm/lib/MC/MCObjectFileInfo.cpp:806
+Env = IsXCOFF;
+// TODO: Initialize MCObjectFileInfo for XCOFF format when MCSectionXCOFF 
is ready.
+break;

This line is over 80 characters.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58930



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

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

[Hubert Tong via Phabricator via lldb-commits]

hubert.reinterpretcast added inline comments.



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

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151567

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

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

[Hubert Tong via Phabricator via lldb-commits]

hubert.reinterpretcast added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151567

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

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

[Hubert Tong via Phabricator via lldb-commits]

hubert.reinterpretcast added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151567

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

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

[Hubert Tong via Phabricator via lldb-commits]

hubert.reinterpretcast added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151567

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

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

[Hubert Tong via Phabricator via lldb-commits]

hubert.reinterpretcast added a comment.

In D151567#4458232 , 
@hubert.reinterpretcast wrote:

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

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

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151567

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

[Lldb-commits] [PATCH] D76168: CPlusPlusNameParser does not handles templated operator< properly

[Hubert Tong via Phabricator via lldb-commits]

hubert.reinterpretcast added a comment.

I am not sure what the usage scenario is that this is meant to support. Is it 
user input that tries to name a specialization of a template `operator<` 
without separation to prevent tokenization as `operator<<`? I think that case 
should qualify as user error.

Both C++17 subclause 17.2 [temp.names] and N4849 subclause 13.3 [temp.names] 
imbue the "angle-bracket" treatment of `<` to after identification of a name 
(not to the formation of the name itself) and does involve special treatment of 
`<<`. Special treatment in tokenization (C++17 subclause 5.4 [lex.pptoken]; 
N4849 subclause 5.4 [lex.pptoken]) for angle brackets extends to `<:` and not 
to `<<`.

All of Clang, GCC, ICC, and MSVC do not compile the following:

  enum E { E0 };
  template  void operator<(E, T);
  
  void g() {
operator<(E0, 0);  // does not compile
  }

Compiler Explorer (godbolt.org) >>link<< 
.

In other words, where is the input coming from? Should the producer of the 
input be corrected instead?


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

https://reviews.llvm.org/D76168



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

[Lldb-commits] [PATCH] D64251: Don't depend on psutil on AIX

[Hubert Tong via Phabricator via lldb-commits]

hubert.reinterpretcast added inline comments.



Comment at: llvm/utils/lit/lit/LitConfig.py:98
+self.fatal("Setting a timeout per test requires the"
+" Python psutil module but it could not be"
+" found. Try installing it via pip or via"

Minor nit: The quote indentation no longer lines up with the previous line.



Comment at: llvm/utils/lit/tests/lit.cfg:62
-lit_config.note('Found python psutil module')
-config.available_features.add("python-psutil")
 except ImportError:

Removing `python-psutil` as a feature entirely may be a bit aggressive. It has 
the potential of quietly disabling "out-of-tree" tests. I'm not sure that a 
Phabricator patch about AIX has the right level of visibility for making such a 
change. Can you send an RFC about the cleanup to the mailing list?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64251



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