[Lldb-commits] [PATCH] D127048: [lldb] Set COFF and PDB module env from default target triple

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

> This changes the PE/COFF and PDB plugins to set the module triple according 
> to the default target triple used to build LLDB.

I'm missing some context here but using triple used when *building* lldb seems 
to conflict with the idea that the same lldb (client at least) can be used to 
debug many architectures.

Or is this not a concern because remote debugging is not a thing for Windows? 
For instance, is it possible that I'm on Linux with some AArch64 linux targeted 
lldb trying to remote debug a windows system. If the default triple is the best 
signal you've got then fair enough I guess just wanted to check there wasn't 
some more dynamic way to get it.

And I assume PE/COFF have no embedded flag in them to indicate this ABI?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127048

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

[Lldb-commits] [PATCH] D127048: [lldb] Set COFF and PDB module env from default target triple

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added a comment.

In D127048#3559866 , @DavidSpickett 
wrote:

>> This changes the PE/COFF and PDB plugins to set the module triple according 
>> to the default target triple used to build LLDB.
>
> I'm missing some context here but using triple used when *building* lldb 
> seems to conflict with the idea that the same lldb (client at least) can be 
> used to debug many architectures.

Yes, that's true. The idea here is that when doing local debugging in either 
the mingw or msvc ecosystems, the flavour of the lldb build itself could be a 
decent hint - but it's not indeed not a correct and faultproof fix.

> Or is this not a concern because remote debugging is not a thing for Windows? 
> For instance, is it possible that I'm on Linux with some AArch64 linux 
> targeted lldb trying to remote debug a windows system. If the default triple 
> is the best signal you've got then fair enough I guess just wanted to check 
> there wasn't some more dynamic way to get it.
>
> And I assume PE/COFF have no embedded flag in them to indicate this ABI?

No, there's no flag for that. Mingw and MSVC environment binaries are quite the 
same.

(Qt Creator tries to do some similar heuristics to guess which ABI a binary 
uses, by looking at e.g. linker version numbers in the binary, to deduce 
whether a binary has been linked with GNU ld.bfd or MS link.exe - but when it 
comes to LLD, that logic falls apart. A more complex heuristic that can be used 
is e.g. looking at the list of DLLs that a binary links against, but that's 
also much more complex and also not entirely faultproof.)

@alvinhochun has got another patch in progress - 
https://reviews.llvm.org/D127053 (which isn't yet submitted for review as it 
lacks tests), which tries to decude the same based on whether a binary contains 
dwarf debug info.

That's more fault proof, but is also not an entirely complete fix - while 
finding dwarf debug sections mostly can imply using the itanium C++ ABI, you 
can also use PDB files with itanium C++ ABI (i.e. in mingw setups). It's less 
common but still a viable scenario. And reversely, I think I've heard about 
special cases where people use dwarf debug info with MSVC-ecosystem builds 
too...

So in short, what would be needed is a good enough heuristic that works for the 
vast majority of cases, and an option that lets users specify it for the cases 
that can't easily be autodetected.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127048

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

[Lldb-commits] [PATCH] D127048: [lldb] Set COFF and PDB module env from default target triple

[Alvin Wong via Phabricator via lldb-commits]

alvinhochun added a comment.

In D127048#3559918 , @mstorsjo wrote:

> In D127048#3559866 , @DavidSpickett 
> wrote:
>
>>> This changes the PE/COFF and PDB plugins to set the module triple according 
>>> to the default target triple used to build LLDB.
>>
>> I'm missing some context here but using triple used when *building* lldb 
>> seems to conflict with the idea that the same lldb (client at least) can be 
>> used to debug many architectures.
>
> Yes, that's true. The idea here is that when doing local debugging in either 
> the mingw or msvc ecosystems, the flavour of the lldb build itself could be a 
> decent hint - but it's not indeed not a correct and faultproof fix.

Yes, the aim is to at least have a default that at least works natively for the 
default target in local debugging. Before this change, C++ debugging for mingw 
target just doesn't work at all from what I understand so far.

> @alvinhochun has got another patch in progress - 
> https://reviews.llvm.org/D127053 (which isn't yet submitted for review as it 
> lacks tests), which tries to decude the same based on whether a binary 
> contains dwarf debug info.
>
> That's more fault proof, but is also not an entirely complete fix - while 
> finding dwarf debug sections mostly can imply using the itanium C++ ABI, you 
> can also use PDB files with itanium C++ ABI (i.e. in mingw setups). It's less 
> common but still a viable scenario. And reversely, I think I've heard about 
> special cases where people use dwarf debug info with MSVC-ecosystem builds 
> too...
>
> So in short, what would be needed is a good enough heuristic that works for 
> the vast majority of cases, and an option that lets users specify it for the 
> cases that can't easily be autodetected.

I'm feeling wishy-washy on that patch because of the reason you stated. It may 
still be a good heuristic, but I think having an option to specify the ABI is a 
requirement before that patch can be landed.

I still don't have a clue how I can make it an option. It seems impossible to 
pass the option into `ObjectFilePECOFF::GetModuleSpecifications` cleanly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127048

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

[Lldb-commits] [PATCH] D127048: [lldb] Set COFF and PDB module env from default target triple

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

There is an arch setting to `target create` but I doubt passing the whole 
triple there will make a difference (truncated to arch only at best).

Platforms do have settings so you could try adding something to 
`remote-windows` or presumably `host` when run on Windows. How you plumb that 
into where you need it I don't know.

  (lldb) settings show platform.plugin.qemu-user.architecture
  platform.plugin.qemu-user.architecture (string) =


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127048

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

[Lldb-commits] [PATCH] D126367: [lldb] Add gnu-debuglink support for Windows PE/COFF

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a reviewer: omjavaid.
DavidSpickett added a comment.

Can you include some links to documentation of the debuglink feature in the 
commit message? I assume 
https://sourceware.org/gdb/onlinedocs/gdb/Separate-Debug-Files.html is what 
you'd want.




Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:882
+  lldb::offset_t gnu_debuglink_offset = 0;
+  std::string gnu_debuglink_file = data.GetCStr(&gnu_debuglink_offset);
+  gnu_debuglink_offset = llvm::alignTo(gnu_debuglink_offset, 4);

Is "leading directory components removed" significant here?
```
A filename, with any leading directory components removed, followed by a zero 
byte,
```
I think that means for `C:\a\b\c\foo` you get `foo`, and here we expect to just 
get the file name not a path?



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:883
+  std::string gnu_debuglink_file = data.GetCStr(&gnu_debuglink_offset);
+  gnu_debuglink_offset = llvm::alignTo(gnu_debuglink_offset, 4);
+  data.GetU32(&gnu_debuglink_offset, &gnu_debuglink_crc, 1);

Is this aligning up or down, can you add a comment explaining?

I think it is something like read a filename, return the offset of the end of 
it. Align that up to a 4 byte boundary then read the crc from there?



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:884
+  gnu_debuglink_offset = llvm::alignTo(gnu_debuglink_offset, 4);
+  data.GetU32(&gnu_debuglink_offset, &gnu_debuglink_crc, 1);
+  return FileSpec(gnu_debuglink_file);

You read the crc but it is unused.

If you just want to make sure that the section actually has enough data to have 
a crc in it, then check somehow that it was read or better, check the size 
explicitly.

As for calculating the crc I guess you'd have to read the whole linked file so 
the most you could do here is stash the crc value somewhere for later when you 
have opportunity to open it. Most of the time you won't hit an issue but I 
guess it's here for a reason. Perhaps if you were to debug a file that linked 
to something that had been rebuilt since?



Comment at: lldb/source/Plugins/SymbolVendor/PECOFF/SymbolVendorPECOFF.cpp:73
+  FileSpec fspec = module_sp->GetSymbolFileFileSpec();
+  // Otherwise, try gnu_debuglink, if one exists.
+  if (!fspec)

Is this preference defined by some standard or are you assuming you won't see 
both?

FWIW it does make sense to me to use the filename in the usual place first, 
then the debuglink, just as you've got it here.



Comment at: lldb/source/Symbol/LocateSymbolFile.cpp:368
+if (num_specs == 2) {
+  // Special case to handle both i386 and i686 from ObjectFilePECOFF
+  ModuleSpec mspec2;

What produces this special case?



Comment at: lldb/test/Shell/ObjectFile/PECOFF/dwarf-gnu-debuglink-i686.yaml:2
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objcopy --strip-all --add-gnu-debuglink=%t %t %t.stripped
+# RUN: lldb-test object-file %t.stripped | FileCheck %s

For these tests you are making a file, then giving it a debug link to itself 
then checking that lldb finds it? Seems like if the parsing of the debuglink 
failed then it could just find the original file and look like it passed.

Except you strip that original file of everything *but* the debug link. So if 
lldb tried to use the orignal file, it wouldn't have any debug info.

Assuming I got that right could you add comments to this and the other test 
stating that intent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126367

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

[Lldb-commits] [PATCH] D127048: [lldb] Set COFF and PDB module env from default target triple

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a reviewer: omjavaid.
DavidSpickett added a comment.

I won't have time to comment much more but Omair might be able to help.

Also you might want to look at how the apple platforms select their ABI if you 
haven't already. If the answer is "with a flag in the object file" then oh 
well, but perhaps they have an override in there you could crib from.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127048

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

[Lldb-commits] [PATCH] D126367: [lldb] Add gnu-debuglink support for Windows PE/COFF

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added a comment.

FWIW, most of the logic for how to deal with the gnu debuglink contents here is 
copied from the corresponding preexisting methods for ELF.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126367

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

[Lldb-commits] [PATCH] D126367: [lldb] Add gnu-debuglink support for Windows PE/COFF

[Alvin Wong via Phabricator via lldb-commits]

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

In D126367#3560233 , @DavidSpickett 
wrote:

> Can you include some links to documentation of the debuglink feature in the 
> commit message? I assume 
> https://sourceware.org/gdb/onlinedocs/gdb/Separate-Debug-Files.html is what 
> you'd want.

I suppose I can add that link.




Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:882
+  lldb::offset_t gnu_debuglink_offset = 0;
+  std::string gnu_debuglink_file = data.GetCStr(&gnu_debuglink_offset);
+  gnu_debuglink_offset = llvm::alignTo(gnu_debuglink_offset, 4);

DavidSpickett wrote:
> Is "leading directory components removed" significant here?
> ```
> A filename, with any leading directory components removed, followed by a zero 
> byte,
> ```
> I think that means for `C:\a\b\c\foo` you get `foo`, and here we expect to 
> just get the file name not a path?
I think that's the expected result. (Also see the reply below.)



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:883
+  std::string gnu_debuglink_file = data.GetCStr(&gnu_debuglink_offset);
+  gnu_debuglink_offset = llvm::alignTo(gnu_debuglink_offset, 4);
+  data.GetU32(&gnu_debuglink_offset, &gnu_debuglink_crc, 1);

DavidSpickett wrote:
> Is this aligning up or down, can you add a comment explaining?
> 
> I think it is something like read a filename, return the offset of the end of 
> it. Align that up to a 4 byte boundary then read the crc from there?
I don't know really. I just copied this logic from 
https://github.com/llvm/llvm-project/blob/99a83b1286748501e0ccf199a582dc3ec5451ef5/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp#L1533,
 which was originally implemented 9 years ago with 
https://github.com/LLVM/llvm-project/commit/a7499c98301e847d2e525921801e1edcc44e34da.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:884
+  gnu_debuglink_offset = llvm::alignTo(gnu_debuglink_offset, 4);
+  data.GetU32(&gnu_debuglink_offset, &gnu_debuglink_crc, 1);
+  return FileSpec(gnu_debuglink_file);

DavidSpickett wrote:
> You read the crc but it is unused.
> 
> If you just want to make sure that the section actually has enough data to 
> have a crc in it, then check somehow that it was read or better, check the 
> size explicitly.
> 
> As for calculating the crc I guess you'd have to read the whole linked file 
> so the most you could do here is stash the crc value somewhere for later when 
> you have opportunity to open it. Most of the time you won't hit an issue but 
> I guess it's here for a reason. Perhaps if you were to debug a file that 
> linked to something that had been rebuilt since?
I didn't think much about it at first. I thought ObjectFileELF only used this 
CRC as one of the few ways of generating an UUID for the object file. But just 
now re-reading the code I realize that `Symbols::LocateExecutableSymbolFile` 
actually uses this UUID to check the validity of the debug file, so it now 
makes more sense.

I think I may have to do the same for ObjectFilePECOFF. The tricky bit is that 
it already has code to calculate an UUID using the PDB info if it exists. What 
should happen if both PDB and gnu-debuglink exists for the COFF object file?



Comment at: lldb/source/Plugins/SymbolVendor/PECOFF/SymbolVendorPECOFF.cpp:73
+  FileSpec fspec = module_sp->GetSymbolFileFileSpec();
+  // Otherwise, try gnu_debuglink, if one exists.
+  if (!fspec)

DavidSpickett wrote:
> Is this preference defined by some standard or are you assuming you won't see 
> both?
> 
> FWIW it does make sense to me to use the filename in the usual place first, 
> then the debuglink, just as you've got it here.
I think that may actually be the option set by the user (`target symbols add` 
perhaps?) but I am not sure. (This was just copied from `SymbolVendorELF`.)



Comment at: lldb/source/Symbol/LocateSymbolFile.cpp:368
+if (num_specs == 2) {
+  // Special case to handle both i386 and i686 from ObjectFilePECOFF
+  ModuleSpec mspec2;

DavidSpickett wrote:
> What produces this special case?
`ObjectFilePECOFF::GetModuleSpecifications` returns two specs for `MachineX86` 
-- `"i386-pc-windows"` and `"i686-pc-windows"` (with `-msvc` after being 
normalized implicitly). I do not know why it does this and I am scared of 
touching it.



Comment at: lldb/test/Shell/ObjectFile/PECOFF/dwarf-gnu-debuglink-i686.yaml:2
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objcopy --strip-all --add-gnu-debuglink=%t %t %t.stripped
+# RUN: lldb-test object-file %t.stripped | FileCheck %s

DavidSpickett wrote:
> For these tests you are making a file, then giving it a debug link to itself 
> then checking that lldb finds it? See

[Lldb-commits] [PATCH] D126367: [lldb] Add gnu-debuglink support for Windows PE/COFF

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:883
+  std::string gnu_debuglink_file = data.GetCStr(&gnu_debuglink_offset);
+  gnu_debuglink_offset = llvm::alignTo(gnu_debuglink_offset, 4);
+  data.GetU32(&gnu_debuglink_offset, &gnu_debuglink_crc, 1);

alvinhochun wrote:
> DavidSpickett wrote:
> > Is this aligning up or down, can you add a comment explaining?
> > 
> > I think it is something like read a filename, return the offset of the end 
> > of it. Align that up to a 4 byte boundary then read the crc from there?
> I don't know really. I just copied this logic from 
> https://github.com/llvm/llvm-project/blob/99a83b1286748501e0ccf199a582dc3ec5451ef5/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp#L1533,
>  which was originally implemented 9 years ago with 
> https://github.com/LLVM/llvm-project/commit/a7499c98301e847d2e525921801e1edcc44e34da.
Please confirm it and add a comment stating the logic.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:884
+  gnu_debuglink_offset = llvm::alignTo(gnu_debuglink_offset, 4);
+  data.GetU32(&gnu_debuglink_offset, &gnu_debuglink_crc, 1);
+  return FileSpec(gnu_debuglink_file);

alvinhochun wrote:
> DavidSpickett wrote:
> > You read the crc but it is unused.
> > 
> > If you just want to make sure that the section actually has enough data to 
> > have a crc in it, then check somehow that it was read or better, check the 
> > size explicitly.
> > 
> > As for calculating the crc I guess you'd have to read the whole linked file 
> > so the most you could do here is stash the crc value somewhere for later 
> > when you have opportunity to open it. Most of the time you won't hit an 
> > issue but I guess it's here for a reason. Perhaps if you were to debug a 
> > file that linked to something that had been rebuilt since?
> I didn't think much about it at first. I thought ObjectFileELF only used this 
> CRC as one of the few ways of generating an UUID for the object file. But 
> just now re-reading the code I realize that 
> `Symbols::LocateExecutableSymbolFile` actually uses this UUID to check the 
> validity of the debug file, so it now makes more sense.
> 
> I think I may have to do the same for ObjectFilePECOFF. The tricky bit is 
> that it already has code to calculate an UUID using the PDB info if it 
> exists. What should happen if both PDB and gnu-debuglink exists for the COFF 
> object file?
You'd have to work out if one can create such a file without "hacking" some how 
like adding a debuglink with objcopy. And even if lldb could load both, would 
they mostly be copies of each other. Seems like an odd scenario.

If it's not something a default toolchain use is going to produce, I'd prefer 
the more "normal" one (the PDB, just a guess) and if people get confused 
because it chose the PDB  then they've somewhat opted into paying that 
investigation cost by choosing this unusual workflow.

You can always log these sort of things too. I believe a lot of the Apple code 
will emit things like ok I'm loading this file here, but actually substituted 
it with this one etc. 99% of people never need to read them but it's easy to 
say to someone who has an issue "enable this log and tell me what you see".



Comment at: lldb/source/Plugins/SymbolVendor/PECOFF/SymbolVendorPECOFF.cpp:73
+  FileSpec fspec = module_sp->GetSymbolFileFileSpec();
+  // Otherwise, try gnu_debuglink, if one exists.
+  if (!fspec)

alvinhochun wrote:
> DavidSpickett wrote:
> > Is this preference defined by some standard or are you assuming you won't 
> > see both?
> > 
> > FWIW it does make sense to me to use the filename in the usual place first, 
> > then the debuglink, just as you've got it here.
> I think that may actually be the option set by the user (`target symbols add` 
> perhaps?) but I am not sure. (This was just copied from `SymbolVendorELF`.)
Ok so we support debuglink from ELF already? Cool.



Comment at: lldb/test/Shell/ObjectFile/PECOFF/dwarf-gnu-debuglink-i686.yaml:2
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objcopy --strip-all --add-gnu-debuglink=%t %t %t.stripped
+# RUN: lldb-test object-file %t.stripped | FileCheck %s

alvinhochun wrote:
> DavidSpickett wrote:
> > For these tests you are making a file, then giving it a debug link to 
> > itself then checking that lldb finds it? Seems like if the parsing of the 
> > debuglink failed then it could just find the original file and look like it 
> > passed.
> > 
> > Except you strip that original file of everything *but* the debug link. So 
> > if lldb tried to use the orignal file, it wouldn't have any debug info.
> > 
> > Assuming I got that right could you add comments to this and the other test 
> > stating that intent.
> No, I think the gnu-debuglink is only added to the output file `%t.stripp

[Lldb-commits] [PATCH] D126367: [lldb] Add gnu-debuglink support for Windows PE/COFF

[Alvin Wong via Phabricator via lldb-commits]

alvinhochun added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:883
+  std::string gnu_debuglink_file = data.GetCStr(&gnu_debuglink_offset);
+  gnu_debuglink_offset = llvm::alignTo(gnu_debuglink_offset, 4);
+  data.GetU32(&gnu_debuglink_offset, &gnu_debuglink_crc, 1);

DavidSpickett wrote:
> alvinhochun wrote:
> > DavidSpickett wrote:
> > > Is this aligning up or down, can you add a comment explaining?
> > > 
> > > I think it is something like read a filename, return the offset of the 
> > > end of it. Align that up to a 4 byte boundary then read the crc from 
> > > there?
> > I don't know really. I just copied this logic from 
> > https://github.com/llvm/llvm-project/blob/99a83b1286748501e0ccf199a582dc3ec5451ef5/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp#L1533,
> >  which was originally implemented 9 years ago with 
> > https://github.com/LLVM/llvm-project/commit/a7499c98301e847d2e525921801e1edcc44e34da.
> Please confirm it and add a comment stating the logic.
Ah yes, the linked spec states

> zero to three bytes of padding, as needed to reach the next four-byte 
> boundary within the section

before the crc checksum.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:884
+  gnu_debuglink_offset = llvm::alignTo(gnu_debuglink_offset, 4);
+  data.GetU32(&gnu_debuglink_offset, &gnu_debuglink_crc, 1);
+  return FileSpec(gnu_debuglink_file);

DavidSpickett wrote:
> alvinhochun wrote:
> > DavidSpickett wrote:
> > > You read the crc but it is unused.
> > > 
> > > If you just want to make sure that the section actually has enough data 
> > > to have a crc in it, then check somehow that it was read or better, check 
> > > the size explicitly.
> > > 
> > > As for calculating the crc I guess you'd have to read the whole linked 
> > > file so the most you could do here is stash the crc value somewhere for 
> > > later when you have opportunity to open it. Most of the time you won't 
> > > hit an issue but I guess it's here for a reason. Perhaps if you were to 
> > > debug a file that linked to something that had been rebuilt since?
> > I didn't think much about it at first. I thought ObjectFileELF only used 
> > this CRC as one of the few ways of generating an UUID for the object file. 
> > But just now re-reading the code I realize that 
> > `Symbols::LocateExecutableSymbolFile` actually uses this UUID to check the 
> > validity of the debug file, so it now makes more sense.
> > 
> > I think I may have to do the same for ObjectFilePECOFF. The tricky bit is 
> > that it already has code to calculate an UUID using the PDB info if it 
> > exists. What should happen if both PDB and gnu-debuglink exists for the 
> > COFF object file?
> You'd have to work out if one can create such a file without "hacking" some 
> how like adding a debuglink with objcopy. And even if lldb could load both, 
> would they mostly be copies of each other. Seems like an odd scenario.
> 
> If it's not something a default toolchain use is going to produce, I'd prefer 
> the more "normal" one (the PDB, just a guess) and if people get confused 
> because it chose the PDB  then they've somewhat opted into paying that 
> investigation cost by choosing this unusual workflow.
> 
> You can always log these sort of things too. I believe a lot of the Apple 
> code will emit things like ok I'm loading this file here, but actually 
> substituted it with this one etc. 99% of people never need to read them but 
> it's easy to say to someone who has an issue "enable this log and tell me 
> what you see".
It is doable, Clang can be asked to produce both PDB and DWARF debug info by 
passing both `-gdwarf -gcodeview`, and link with `-Wl,-pdb=`.

It turns out LLD always writes a synthetic "build id" for mingw even if no PDB 
is generated 
(https://github.com/llvm/llvm-project/blob/0498415f1d6ac0bfbda72486505c11a7b3d464c4/lld/COFF/Writer.cpp#L1926).
 Seems that this mechanism is already working to check that the executable and 
the debug file are from the same build at least for LLD-linked objects, because 
this "build id" does not get stripped. I have verified that lldb does generate 
the UUID for such objects.

As a fallback for objects linked by the GNU linker, I can use the CRC to 
generate the UUID like how ObjectFileELF does it.

I will add tests for these cases too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126367

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

[Lldb-commits] [PATCH] D116136: [lldb] Add UTF-8 char basic type

[Luís Ferreira via Phabricator via lldb-commits]

ljmf00 updated this revision to Diff 434543.
Herald added a project: All.

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

https://reviews.llvm.org/D116136

Files:
  lldb/bindings/python/python-extensions.swig
  lldb/docs/python_api_enums.rst


Index: lldb/docs/python_api_enums.rst
===
--- lldb/docs/python_api_enums.rst
+++ lldb/docs/python_api_enums.rst
@@ -1017,9 +1017,9 @@
 .. py:data:: eBasicTypeWChar
 .. py:data:: eBasicTypeSignedWChar
 .. py:data:: eBasicTypeUnsignedWChar
-.. py:data:: eBasicTypeChar8
 .. py:data:: eBasicTypeChar16
 .. py:data:: eBasicTypeChar32
+.. py:data:: eBasicTypeChar8
 .. py:data:: eBasicTypeShort
 .. py:data:: eBasicTypeUnsignedShort
 .. py:data:: eBasicTypeInt
Index: lldb/bindings/python/python-extensions.swig
===
--- lldb/bindings/python/python-extensions.swig
+++ lldb/bindings/python/python-extensions.swig
@@ -565,6 +565,7 @@
 if basic_type == eBasicTypeUnsignedWChar: return (True,False)
 if basic_type == eBasicTypeChar16: return (True,False)
 if basic_type == eBasicTypeChar32: return (True,False)
+if basic_type == eBasicTypeChar8: return (True,False)
 if basic_type == eBasicTypeShort: return (True,True)
 if basic_type == eBasicTypeUnsignedShort: return (True,False)
 if basic_type == eBasicTypeInt: return (True,True)


Index: lldb/docs/python_api_enums.rst
===
--- lldb/docs/python_api_enums.rst
+++ lldb/docs/python_api_enums.rst
@@ -1017,9 +1017,9 @@
 .. py:data:: eBasicTypeWChar
 .. py:data:: eBasicTypeSignedWChar
 .. py:data:: eBasicTypeUnsignedWChar
-.. py:data:: eBasicTypeChar8
 .. py:data:: eBasicTypeChar16
 .. py:data:: eBasicTypeChar32
+.. py:data:: eBasicTypeChar8
 .. py:data:: eBasicTypeShort
 .. py:data:: eBasicTypeUnsignedShort
 .. py:data:: eBasicTypeInt
Index: lldb/bindings/python/python-extensions.swig
===
--- lldb/bindings/python/python-extensions.swig
+++ lldb/bindings/python/python-extensions.swig
@@ -565,6 +565,7 @@
 if basic_type == eBasicTypeUnsignedWChar: return (True,False)
 if basic_type == eBasicTypeChar16: return (True,False)
 if basic_type == eBasicTypeChar32: return (True,False)
+if basic_type == eBasicTypeChar8: return (True,False)
 if basic_type == eBasicTypeShort: return (True,True)
 if basic_type == eBasicTypeUnsignedShort: return (True,False)
 if basic_type == eBasicTypeInt: return (True,True)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D116136: [lldb] Add UTF-8 char basic type

[Luís Ferreira via Phabricator via lldb-commits]

ljmf00 added a comment.

@labath In the meantime, a lot of time passed and someone patched it in D120690 
, but the order on the documentation is wrong 
and the Python bindings need swig definitions. This patch introduces those, 
after rebasing with main.


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

https://reviews.llvm.org/D116136

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

[Lldb-commits] [PATCH] D126367: [lldb] Add gnu-debuglink support for Windows PE/COFF

[Alvin Wong via Phabricator via lldb-commits]

alvinhochun updated this revision to Diff 434568.
alvinhochun edited the summary of this revision.
alvinhochun added a comment.

Addressed comments, added CRC checking for debuglink (only applies if the 
object does not contain a PDB build id) and added tests for this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126367

Files:
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
  lldb/source/Plugins/SymbolVendor/CMakeLists.txt
  lldb/source/Plugins/SymbolVendor/PECOFF/CMakeLists.txt
  lldb/source/Plugins/SymbolVendor/PECOFF/SymbolVendorPECOFF.cpp
  lldb/source/Plugins/SymbolVendor/PECOFF/SymbolVendorPECOFF.h
  lldb/source/Symbol/LocateSymbolFile.cpp
  lldb/test/Shell/ObjectFile/PECOFF/dwarf-gnu-debuglink-i686.yaml
  lldb/test/Shell/ObjectFile/PECOFF/dwarf-gnu-debuglink-mismatched-crc.yaml
  lldb/test/Shell/ObjectFile/PECOFF/dwarf-gnu-debuglink-pdb-buildid.yaml
  lldb/test/Shell/ObjectFile/PECOFF/dwarf-gnu-debuglink.yaml

Index: lldb/test/Shell/ObjectFile/PECOFF/dwarf-gnu-debuglink.yaml
===
--- /dev/null
+++ lldb/test/Shell/ObjectFile/PECOFF/dwarf-gnu-debuglink.yaml
@@ -0,0 +1,50 @@
+# This test produces a stripped version of the object file and adds a
+# gnu-debuglink section to it linking to the unstripped version of the object
+# file. The debug info shall be loaded from the gnu-debuglink reference.
+
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objcopy --strip-all --add-gnu-debuglink=%t %t %t.stripped
+# RUN: lldb-test object-file %t.stripped | FileCheck %s
+
+# CHECK: Name: .debug_info
+# CHECK-NEXT: Type: dwarf-info
+
+--- !COFF
+OptionalHeader:
+  AddressOfEntryPoint: 5152
+  ImageBase:   5368709120
+  SectionAlignment: 4096
+  FileAlignment:   512
+  MajorOperatingSystemVersion: 6
+  MinorOperatingSystemVersion: 0
+  MajorImageVersion: 0
+  MinorImageVersion: 0
+  MajorSubsystemVersion: 6
+  MinorSubsystemVersion: 0
+  Subsystem:   IMAGE_SUBSYSTEM_WINDOWS_CUI
+  DLLCharacteristics: [ IMAGE_DLL_CHARACTERISTICS_HIGH_ENTROPY_VA, IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE, IMAGE_DLL_CHARACTERISTICS_NX_COMPAT, IMAGE_DLL_CHARACTERISTICS_TERMINAL_SERVER_AWARE ]
+  SizeOfStackReserve: 1048576
+  SizeOfStackCommit: 4096
+  SizeOfHeapReserve: 1048576
+  SizeOfHeapCommit: 4096
+header:
+  Machine: IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [ IMAGE_FILE_EXECUTABLE_IMAGE, IMAGE_FILE_LARGE_ADDRESS_AWARE ]
+sections:
+  - Name:.text
+Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+VirtualAddress:  4096
+VirtualSize: 64
+SectionData: DEADBEEFBAADF00D
+  - Name:.data
+Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ ]
+VirtualAddress:  8192
+VirtualSize: 64
+SectionData: DEADBEEFBAADF00D
+  - Name:.debug_info
+Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_DISCARDABLE, IMAGE_SCN_MEM_READ ]
+VirtualAddress:  16384
+VirtualSize: 64
+SectionData: DEADBEEFBAADF00D
+symbols: []
+...
Index: lldb/test/Shell/ObjectFile/PECOFF/dwarf-gnu-debuglink-pdb-buildid.yaml
===
--- /dev/null
+++ lldb/test/Shell/ObjectFile/PECOFF/dwarf-gnu-debuglink-pdb-buildid.yaml
@@ -0,0 +1,63 @@
+# This test produces a stripped version of the object file and adds a
+# gnu-debuglink section to it linking to the unstripped version of the object
+# file. Then the unstripped version is stripped to keep only debug info to
+# cause its crc to change. In this case the object files still have the
+# synthetic PDB build id that LLD adds even for the mingw target. Because this
+# build id still matches, the debug info shall still be loaded from the
+# gnu-debuglink reference.
+
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objcopy --strip-all --add-gnu-debuglink=%t %t %t.stripped
+# RUN: llvm-strip --only-keep-debug %t
+# RUN: lldb-test object-file %t.stripped | FileCheck %s
+
+# CHECK: Name: .debug_info
+# CHECK-NEXT: Type: dwarf-info
+
+--- !COFF
+OptionalHeader:
+  AddressOfEntryPoint: 5152
+  ImageBase:   5368709120
+  SectionAlignment: 4096
+  FileAlignment:   512
+  MajorOperatingSystemVersion: 6
+  MinorOperatingSystemVersion: 0
+  MajorImageVersion: 0
+  MinorImageVersion: 0
+  MajorSubsystemVersion: 6
+  MinorSubsystemVersion: 0
+  Subsystem:   IMAGE_SUBSYSTEM_WINDOWS_CUI
+  DLLCharacteristics: [ IMAGE_DLL_CHARACTERISTICS_HIGH_ENTROPY_VA, IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE, IMAGE_DLL_CHARACTERISTICS_NX_COMPAT, IMAGE_DLL_CHARACTERISTICS_TERMINAL_SERVER_AWARE ]
+  SizeOfStackReserve: 1048576
+  SizeOfStackCommit: 4096
+  SizeOfHeapReserve: 1048576
+  SizeOfHeapCommit: 4096
+  Debug:
+RelativeVirtualAddress: 16384
+Size:28
+header:
+  Machine: I

[Lldb-commits] [PATCH] D48865: [LLDB] CommandObjectThreadUntil::DoExecute() sets the wrong selected thread ID

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Jim Ingham should give any final OK since he is the code owner of the thread 
plans.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D48865

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

[Lldb-commits] [PATCH] D127164: [WebAssembly] Add WASM_SEC_LAST_KNOWN to BinaryFormat section types list [NFC]

[Derek Schuff via Phabricator via lldb-commits]

dschuff created this revision.
dschuff added reviewers: aheejin, sbc100.
Herald added subscribers: pmatos, asb, wingo, ecnelises, sunfish, hiraditya, 
jgravelle-google.
Herald added a reviewer: jhenderson.
Herald added a project: All.
dschuff requested review of this revision.
Herald added subscribers: llvm-commits, lldb-commits, MaskRay.
Herald added projects: LLDB, LLVM.

There are 3 places where we were using WASM_SEC_TAG as the "last" known
section type, which requires updating (or leaves a bug) when a new known
section type is added. Instead add a "last type" to the enum for this
purpose.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D127164

Files:
  lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp
  llvm/include/llvm/BinaryFormat/Wasm.h
  llvm/lib/ObjCopy/wasm/WasmReader.cpp
  llvm/lib/Object/WasmObjectFile.cpp


Index: llvm/lib/Object/WasmObjectFile.cpp
===
--- llvm/lib/Object/WasmObjectFile.cpp
+++ llvm/lib/Object/WasmObjectFile.cpp
@@ -1729,7 +1729,7 @@
   const WasmSection &S = Sections[Sec.d.a];
   if (S.Type == wasm::WASM_SEC_CUSTOM)
 return S.Name;
-  if (S.Type > wasm::WASM_SEC_TAG)
+  if (S.Type >= wasm::WASM_SEC_LAST_KNOWN)
 return createStringError(object_error::invalid_section_index, "");
   return wasm::sectionTypeToString(S.Type);
 }
Index: llvm/lib/ObjCopy/wasm/WasmReader.cpp
===
--- llvm/lib/ObjCopy/wasm/WasmReader.cpp
+++ llvm/lib/ObjCopy/wasm/WasmReader.cpp
@@ -27,12 +27,8 @@
 // Give known sections standard names to allow them to be selected.
 Section &ReaderSec = Obj->Sections.back();
 if (ReaderSec.SectionType > WASM_SEC_CUSTOM &&
-ReaderSec.SectionType <= WASM_SEC_TAG)
+ReaderSec.SectionType < WASM_SEC_LAST_KNOWN)
   ReaderSec.Name = sectionTypeToString(ReaderSec.SectionType);
-// If the section type is CUSTOM, it has a name already. If it's a new type
-// of section that we don't explicitly handle here, it will have an empty
-// name and objcopy won't be able to select it by name (e.g. for removal
-// or dumping) but it will still be valid and able to be copied.
   }
   return std::move(Obj);
 }
Index: llvm/include/llvm/BinaryFormat/Wasm.h
===
--- llvm/include/llvm/BinaryFormat/Wasm.h
+++ llvm/include/llvm/BinaryFormat/Wasm.h
@@ -252,7 +252,8 @@
   WASM_SEC_CODE = 10,  // Function bodies (code)
   WASM_SEC_DATA = 11,  // Data segments
   WASM_SEC_DATACOUNT = 12, // Data segment count
-  WASM_SEC_TAG = 13// Tag declarations
+  WASM_SEC_TAG = 13,   // Tag declarations
+  WASM_SEC_LAST_KNOWN = 14,// Update this when adding new sections
 };
 
 // Type immediate encodings used in various contexts.
Index: lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp
===
--- lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp
+++ lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp
@@ -192,7 +192,7 @@
 m_sect_infos.push_back(section_info{*offset_ptr + c.tell(), section_length,
 section_id, *sect_name});
 *offset_ptr += (c.tell() + section_length);
-  } else if (section_id <= llvm::wasm::WASM_SEC_TAG) {
+  } else if (section_id < llvm::wasm::WASM_SEC_LAST_KNOWN) {
 m_sect_infos.push_back(section_info{*offset_ptr + c.tell(),
 static_cast(payload_len),
 section_id, ConstString()});


Index: llvm/lib/Object/WasmObjectFile.cpp
===
--- llvm/lib/Object/WasmObjectFile.cpp
+++ llvm/lib/Object/WasmObjectFile.cpp
@@ -1729,7 +1729,7 @@
   const WasmSection &S = Sections[Sec.d.a];
   if (S.Type == wasm::WASM_SEC_CUSTOM)
 return S.Name;
-  if (S.Type > wasm::WASM_SEC_TAG)
+  if (S.Type >= wasm::WASM_SEC_LAST_KNOWN)
 return createStringError(object_error::invalid_section_index, "");
   return wasm::sectionTypeToString(S.Type);
 }
Index: llvm/lib/ObjCopy/wasm/WasmReader.cpp
===
--- llvm/lib/ObjCopy/wasm/WasmReader.cpp
+++ llvm/lib/ObjCopy/wasm/WasmReader.cpp
@@ -27,12 +27,8 @@
 // Give known sections standard names to allow them to be selected.
 Section &ReaderSec = Obj->Sections.back();
 if (ReaderSec.SectionType > WASM_SEC_CUSTOM &&
-ReaderSec.SectionType <= WASM_SEC_TAG)
+ReaderSec.SectionType < WASM_SEC_LAST_KNOWN)
   ReaderSec.Name = sectionTypeToString(ReaderSec.SectionType);
-// If the section type is CUSTOM, it has a name already. If it's a new type
-// of section that we don't explicitly handle here, it will have an empty
-// name and objcopy won't be able to select it by name (e.g. for removal
-// or dumping) but it will still be 

[Lldb-commits] [PATCH] D127164: [WebAssembly] Add WASM_SEC_LAST_KNOWN to BinaryFormat section types list [NFC]

[Heejin Ahn via Phabricator via lldb-commits]

aheejin added inline comments.



Comment at: llvm/include/llvm/BinaryFormat/Wasm.h:256
+  WASM_SEC_TAG = 13,   // Tag declarations
+  WASM_SEC_LAST_KNOWN = 14,// Update this when adding new sections
 };

- Does this pass clang-format? Because we don't have a space after `,`.
- 14 is not actually the last section known, because we don't have a section 
whose section code is 14. How about doing something like
```
WAS_SEC_LAST_KNOWN = WASM_SEC_TAG;
```
?



Comment at: llvm/lib/ObjCopy/wasm/WasmReader.cpp:32-35
-// If the section type is CUSTOM, it has a name already. If it's a new type
-// of section that we don't explicitly handle here, it will have an empty
-// name and objcopy won't be able to select it by name (e.g. for removal
-// or dumping) but it will still be valid and able to be copied.

Why was this removed? It doesn't look like it's related to the last section 
thing.. It's just explaning custom sections have names already.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127164

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

[Lldb-commits] [PATCH] D127164: [WebAssembly] Add WASM_SEC_LAST_KNOWN to BinaryFormat section types list [NFC]

[Derek Schuff via Phabricator via lldb-commits]

dschuff added inline comments.



Comment at: llvm/lib/ObjCopy/wasm/WasmReader.cpp:32-35
-// If the section type is CUSTOM, it has a name already. If it's a new type
-// of section that we don't explicitly handle here, it will have an empty
-// name and objcopy won't be able to select it by name (e.g. for removal
-// or dumping) but it will still be valid and able to be copied.

aheejin wrote:
> Why was this removed? It doesn't look like it's related to the last section 
> thing.. It's just explaning custom sections have names already.
Checking against `WASM_SEC_LAST_KNOWN` ensures that there can't be a new type 
of section that we don't explicitly handle here (because no types of known 
sections are explicitly handled or mentioned); any section known to 
`sectionTypeToString` will work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127164

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

[Lldb-commits] [PATCH] D127164: [WebAssembly] Add WASM_SEC_LAST_KNOWN to BinaryFormat section types list [NFC]

[Derek Schuff via Phabricator via lldb-commits]

dschuff updated this revision to Diff 434647.
dschuff added a comment.

- review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127164

Files:
  lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp
  llvm/include/llvm/BinaryFormat/Wasm.h
  llvm/lib/ObjCopy/wasm/WasmReader.cpp
  llvm/lib/Object/WasmObjectFile.cpp


Index: llvm/lib/Object/WasmObjectFile.cpp
===
--- llvm/lib/Object/WasmObjectFile.cpp
+++ llvm/lib/Object/WasmObjectFile.cpp
@@ -1729,7 +1729,7 @@
   const WasmSection &S = Sections[Sec.d.a];
   if (S.Type == wasm::WASM_SEC_CUSTOM)
 return S.Name;
-  if (S.Type > wasm::WASM_SEC_TAG)
+  if (S.Type > wasm::WASM_SEC_LAST_KNOWN)
 return createStringError(object_error::invalid_section_index, "");
   return wasm::sectionTypeToString(S.Type);
 }
Index: llvm/lib/ObjCopy/wasm/WasmReader.cpp
===
--- llvm/lib/ObjCopy/wasm/WasmReader.cpp
+++ llvm/lib/ObjCopy/wasm/WasmReader.cpp
@@ -27,12 +27,8 @@
 // Give known sections standard names to allow them to be selected.
 Section &ReaderSec = Obj->Sections.back();
 if (ReaderSec.SectionType > WASM_SEC_CUSTOM &&
-ReaderSec.SectionType <= WASM_SEC_TAG)
+ReaderSec.SectionType <= WASM_SEC_LAST_KNOWN)
   ReaderSec.Name = sectionTypeToString(ReaderSec.SectionType);
-// If the section type is CUSTOM, it has a name already. If it's a new type
-// of section that we don't explicitly handle here, it will have an empty
-// name and objcopy won't be able to select it by name (e.g. for removal
-// or dumping) but it will still be valid and able to be copied.
   }
   return std::move(Obj);
 }
Index: llvm/include/llvm/BinaryFormat/Wasm.h
===
--- llvm/include/llvm/BinaryFormat/Wasm.h
+++ llvm/include/llvm/BinaryFormat/Wasm.h
@@ -252,7 +252,8 @@
   WASM_SEC_CODE = 10,  // Function bodies (code)
   WASM_SEC_DATA = 11,  // Data segments
   WASM_SEC_DATACOUNT = 12, // Data segment count
-  WASM_SEC_TAG = 13// Tag declarations
+  WASM_SEC_TAG = 13,   // Tag declarations
+  WASM_SEC_LAST_KNOWN = WASM_SEC_TAG,
 };
 
 // Type immediate encodings used in various contexts.
Index: lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp
===
--- lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp
+++ lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp
@@ -192,7 +192,7 @@
 m_sect_infos.push_back(section_info{*offset_ptr + c.tell(), section_length,
 section_id, *sect_name});
 *offset_ptr += (c.tell() + section_length);
-  } else if (section_id <= llvm::wasm::WASM_SEC_TAG) {
+  } else if (section_id <= llvm::wasm::WASM_SEC_LAST_KNOWN) {
 m_sect_infos.push_back(section_info{*offset_ptr + c.tell(),
 static_cast(payload_len),
 section_id, ConstString()});


Index: llvm/lib/Object/WasmObjectFile.cpp
===
--- llvm/lib/Object/WasmObjectFile.cpp
+++ llvm/lib/Object/WasmObjectFile.cpp
@@ -1729,7 +1729,7 @@
   const WasmSection &S = Sections[Sec.d.a];
   if (S.Type == wasm::WASM_SEC_CUSTOM)
 return S.Name;
-  if (S.Type > wasm::WASM_SEC_TAG)
+  if (S.Type > wasm::WASM_SEC_LAST_KNOWN)
 return createStringError(object_error::invalid_section_index, "");
   return wasm::sectionTypeToString(S.Type);
 }
Index: llvm/lib/ObjCopy/wasm/WasmReader.cpp
===
--- llvm/lib/ObjCopy/wasm/WasmReader.cpp
+++ llvm/lib/ObjCopy/wasm/WasmReader.cpp
@@ -27,12 +27,8 @@
 // Give known sections standard names to allow them to be selected.
 Section &ReaderSec = Obj->Sections.back();
 if (ReaderSec.SectionType > WASM_SEC_CUSTOM &&
-ReaderSec.SectionType <= WASM_SEC_TAG)
+ReaderSec.SectionType <= WASM_SEC_LAST_KNOWN)
   ReaderSec.Name = sectionTypeToString(ReaderSec.SectionType);
-// If the section type is CUSTOM, it has a name already. If it's a new type
-// of section that we don't explicitly handle here, it will have an empty
-// name and objcopy won't be able to select it by name (e.g. for removal
-// or dumping) but it will still be valid and able to be copied.
   }
   return std::move(Obj);
 }
Index: llvm/include/llvm/BinaryFormat/Wasm.h
===
--- llvm/include/llvm/BinaryFormat/Wasm.h
+++ llvm/include/llvm/BinaryFormat/Wasm.h
@@ -252,7 +252,8 @@
   WASM_SEC_CODE = 10,  // Function bodies (code)
   WASM_SEC_DATA = 11,  // Data segments
   WASM_SEC_DATACOUNT = 12, // Data segment count
-  WASM_SEC_TAG = 13// Tag declarat

[Lldb-commits] [PATCH] D127164: [WebAssembly] Add WASM_SEC_LAST_KNOWN to BinaryFormat section types list [NFC]

[Derek Schuff via Phabricator via lldb-commits]

dschuff added inline comments.



Comment at: llvm/include/llvm/BinaryFormat/Wasm.h:256
+  WASM_SEC_TAG = 13,   // Tag declarations
+  WASM_SEC_LAST_KNOWN = 14,// Update this when adding new sections
 };

aheejin wrote:
> - Does this pass clang-format? Because we don't have a space after `,`.
> - 14 is not actually the last section known, because we don't have a section 
> whose section code is 14. How about doing something like
> ```
> WAS_SEC_LAST_KNOWN = WASM_SEC_TAG;
> ```
> ?
ah, no it doesnt pass clang-format.
Also, agreed that LAST_KNOWN should match TAG rather than the one past-the-end. 
Fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127164

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

[Lldb-commits] [PATCH] D127164: [WebAssembly] Add WASM_SEC_LAST_KNOWN to BinaryFormat section types list [NFC]

[Derek Schuff via Phabricator via lldb-commits]

dschuff updated this revision to Diff 434650.
dschuff added a comment.

- expand comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127164

Files:
  lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp
  llvm/include/llvm/BinaryFormat/Wasm.h
  llvm/lib/ObjCopy/wasm/WasmReader.cpp
  llvm/lib/Object/WasmObjectFile.cpp


Index: llvm/lib/Object/WasmObjectFile.cpp
===
--- llvm/lib/Object/WasmObjectFile.cpp
+++ llvm/lib/Object/WasmObjectFile.cpp
@@ -1729,7 +1729,7 @@
   const WasmSection &S = Sections[Sec.d.a];
   if (S.Type == wasm::WASM_SEC_CUSTOM)
 return S.Name;
-  if (S.Type > wasm::WASM_SEC_TAG)
+  if (S.Type > wasm::WASM_SEC_LAST_KNOWN)
 return createStringError(object_error::invalid_section_index, "");
   return wasm::sectionTypeToString(S.Type);
 }
Index: llvm/lib/ObjCopy/wasm/WasmReader.cpp
===
--- llvm/lib/ObjCopy/wasm/WasmReader.cpp
+++ llvm/lib/ObjCopy/wasm/WasmReader.cpp
@@ -24,15 +24,12 @@
 const WasmSection &WS = WasmObj.getWasmSection(Sec);
 Obj->Sections.push_back(
 {static_cast(WS.Type), WS.Name, WS.Content});
-// Give known sections standard names to allow them to be selected.
+// Give known sections standard names to allow them to be selected. (Custom
+// sections already have their names filled in by the parser).
 Section &ReaderSec = Obj->Sections.back();
 if (ReaderSec.SectionType > WASM_SEC_CUSTOM &&
-ReaderSec.SectionType <= WASM_SEC_TAG)
+ReaderSec.SectionType <= WASM_SEC_LAST_KNOWN)
   ReaderSec.Name = sectionTypeToString(ReaderSec.SectionType);
-// If the section type is CUSTOM, it has a name already. If it's a new type
-// of section that we don't explicitly handle here, it will have an empty
-// name and objcopy won't be able to select it by name (e.g. for removal
-// or dumping) but it will still be valid and able to be copied.
   }
   return std::move(Obj);
 }
Index: llvm/include/llvm/BinaryFormat/Wasm.h
===
--- llvm/include/llvm/BinaryFormat/Wasm.h
+++ llvm/include/llvm/BinaryFormat/Wasm.h
@@ -252,7 +252,8 @@
   WASM_SEC_CODE = 10,  // Function bodies (code)
   WASM_SEC_DATA = 11,  // Data segments
   WASM_SEC_DATACOUNT = 12, // Data segment count
-  WASM_SEC_TAG = 13// Tag declarations
+  WASM_SEC_TAG = 13,   // Tag declarations
+  WASM_SEC_LAST_KNOWN = WASM_SEC_TAG,
 };
 
 // Type immediate encodings used in various contexts.
Index: lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp
===
--- lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp
+++ lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp
@@ -192,7 +192,7 @@
 m_sect_infos.push_back(section_info{*offset_ptr + c.tell(), section_length,
 section_id, *sect_name});
 *offset_ptr += (c.tell() + section_length);
-  } else if (section_id <= llvm::wasm::WASM_SEC_TAG) {
+  } else if (section_id <= llvm::wasm::WASM_SEC_LAST_KNOWN) {
 m_sect_infos.push_back(section_info{*offset_ptr + c.tell(),
 static_cast(payload_len),
 section_id, ConstString()});


Index: llvm/lib/Object/WasmObjectFile.cpp
===
--- llvm/lib/Object/WasmObjectFile.cpp
+++ llvm/lib/Object/WasmObjectFile.cpp
@@ -1729,7 +1729,7 @@
   const WasmSection &S = Sections[Sec.d.a];
   if (S.Type == wasm::WASM_SEC_CUSTOM)
 return S.Name;
-  if (S.Type > wasm::WASM_SEC_TAG)
+  if (S.Type > wasm::WASM_SEC_LAST_KNOWN)
 return createStringError(object_error::invalid_section_index, "");
   return wasm::sectionTypeToString(S.Type);
 }
Index: llvm/lib/ObjCopy/wasm/WasmReader.cpp
===
--- llvm/lib/ObjCopy/wasm/WasmReader.cpp
+++ llvm/lib/ObjCopy/wasm/WasmReader.cpp
@@ -24,15 +24,12 @@
 const WasmSection &WS = WasmObj.getWasmSection(Sec);
 Obj->Sections.push_back(
 {static_cast(WS.Type), WS.Name, WS.Content});
-// Give known sections standard names to allow them to be selected.
+// Give known sections standard names to allow them to be selected. (Custom
+// sections already have their names filled in by the parser).
 Section &ReaderSec = Obj->Sections.back();
 if (ReaderSec.SectionType > WASM_SEC_CUSTOM &&
-ReaderSec.SectionType <= WASM_SEC_TAG)
+ReaderSec.SectionType <= WASM_SEC_LAST_KNOWN)
   ReaderSec.Name = sectionTypeToString(ReaderSec.SectionType);
-// If the section type is CUSTOM, it has a name already. If it's a new type
-// of section that we don't explicitly handle here, it will have an empty
-/

[Lldb-commits] [PATCH] D127164: [WebAssembly] Add WASM_SEC_LAST_KNOWN to BinaryFormat section types list [NFC]

[Heejin Ahn via Phabricator via lldb-commits]

aheejin added inline comments.



Comment at: llvm/lib/ObjCopy/wasm/WasmReader.cpp:32-35
-// If the section type is CUSTOM, it has a name already. If it's a new type
-// of section that we don't explicitly handle here, it will have an empty
-// name and objcopy won't be able to select it by name (e.g. for removal
-// or dumping) but it will still be valid and able to be copied.

dschuff wrote:
> aheejin wrote:
> > Why was this removed? It doesn't look like it's related to the last section 
> > thing.. It's just explaning custom sections have names already.
> Checking against `WASM_SEC_LAST_KNOWN` ensures that there can't be a new type 
> of section that we don't explicitly handle here (because no types of known 
> sections are explicitly handled or mentioned); any section known to 
> `sectionTypeToString` will work.
I'm not sure if I understand. Is this related to `WASM_SEC_TAG` -> 
`WASM_LAST_SEC_KNOWN` change in this PR? Or it's just an unrelated drive-by fix?

> Checking against `WASM_SEC_LAST_KNOWN` ensures that there can't be a new type 
> of section that we don't explicitly handle here (because no types of known 
> sections are explicitly handled or mentioned);

I'm not sure what this means..

> If the section type is CUSTOM, it has a name already.

Why is this deleted too? How is the custom section related to 
`WASM_SEC_LAST_KNOWN`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127164

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

[Lldb-commits] [PATCH] D127164: [WebAssembly] Add WASM_SEC_LAST_KNOWN to BinaryFormat section types list [NFC]

[Derek Schuff via Phabricator via lldb-commits]

dschuff added inline comments.



Comment at: llvm/lib/ObjCopy/wasm/WasmReader.cpp:32-35
-// If the section type is CUSTOM, it has a name already. If it's a new type
-// of section that we don't explicitly handle here, it will have an empty
-// name and objcopy won't be able to select it by name (e.g. for removal
-// or dumping) but it will still be valid and able to be copied.

aheejin wrote:
> dschuff wrote:
> > aheejin wrote:
> > > Why was this removed? It doesn't look like it's related to the last 
> > > section thing.. It's just explaning custom sections have names already.
> > Checking against `WASM_SEC_LAST_KNOWN` ensures that there can't be a new 
> > type of section that we don't explicitly handle here (because no types of 
> > known sections are explicitly handled or mentioned); any section known to 
> > `sectionTypeToString` will work.
> I'm not sure if I understand. Is this related to `WASM_SEC_TAG` -> 
> `WASM_LAST_SEC_KNOWN` change in this PR? Or it's just an unrelated drive-by 
> fix?
> 
> > Checking against `WASM_SEC_LAST_KNOWN` ensures that there can't be a new 
> > type of section that we don't explicitly handle here (because no types of 
> > known sections are explicitly handled or mentioned);
> 
> I'm not sure what this means..
> 
> > If the section type is CUSTOM, it has a name already.
> 
> Why is this deleted too? How is the custom section related to 
> `WASM_SEC_LAST_KNOWN`?
It is indirectly related.
The purpose of this block of code is to fill in the `Name` field of the 
`WasmSection` object for known sections (as the comment on line 27 explains). 
Suppose we add a new type of known section, WASM_SEC_NEW, and we update 
`sectionTypeToString` but forget to update this file.  Before this CL, the 
behavior would be as described in the comment I'm deleting; this code would 
silently fail to give that section a name, which would mean it couldn't be 
selected by llvm-objdump. After this CL, everything would work, without needing 
any updates to this file (and if we forgot to update `sectionTypeToString` it 
would assert when encountering an unknown section type). So this comment was 
just to describe the somewhat surprising failure mode, and isn't necessary 
anymore.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127164

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

[Lldb-commits] [PATCH] D127164: [WebAssembly] Add WASM_SEC_LAST_KNOWN to BinaryFormat section types list [NFC]

[Heejin Ahn via Phabricator via lldb-commits]

aheejin accepted this revision.
aheejin added inline comments.



Comment at: llvm/lib/ObjCopy/wasm/WasmReader.cpp:32-35
-// If the section type is CUSTOM, it has a name already. If it's a new type
-// of section that we don't explicitly handle here, it will have an empty
-// name and objcopy won't be able to select it by name (e.g. for removal
-// or dumping) but it will still be valid and able to be copied.

dschuff wrote:
> aheejin wrote:
> > dschuff wrote:
> > > aheejin wrote:
> > > > Why was this removed? It doesn't look like it's related to the last 
> > > > section thing.. It's just explaning custom sections have names already.
> > > Checking against `WASM_SEC_LAST_KNOWN` ensures that there can't be a new 
> > > type of section that we don't explicitly handle here (because no types of 
> > > known sections are explicitly handled or mentioned); any section known to 
> > > `sectionTypeToString` will work.
> > I'm not sure if I understand. Is this related to `WASM_SEC_TAG` -> 
> > `WASM_LAST_SEC_KNOWN` change in this PR? Or it's just an unrelated drive-by 
> > fix?
> > 
> > > Checking against `WASM_SEC_LAST_KNOWN` ensures that there can't be a new 
> > > type of section that we don't explicitly handle here (because no types of 
> > > known sections are explicitly handled or mentioned);
> > 
> > I'm not sure what this means..
> > 
> > > If the section type is CUSTOM, it has a name already.
> > 
> > Why is this deleted too? How is the custom section related to 
> > `WASM_SEC_LAST_KNOWN`?
> It is indirectly related.
> The purpose of this block of code is to fill in the `Name` field of the 
> `WasmSection` object for known sections (as the comment on line 27 explains). 
> Suppose we add a new type of known section, WASM_SEC_NEW, and we update 
> `sectionTypeToString` but forget to update this file.  Before this CL, the 
> behavior would be as described in the comment I'm deleting; this code would 
> silently fail to give that section a name, which would mean it couldn't be 
> selected by llvm-objdump. After this CL, everything would work, without 
> needing any updates to this file (and if we forgot to update 
> `sectionTypeToString` it would assert when encountering an unknown section 
> type). So this comment was just to describe the somewhat surprising failure 
> mode, and isn't necessary anymore.
I see.  My original question was why
> If the section type is CUSTOM, it has a name already. 
is related to this CL, but I see you moved that comment to elsewhere, so it 
hasn't really changed..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127164

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