Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp | ||
|---|---|---|
| 29 | Note that DenseMap doesn't guarantee deterministic iteration order, which is something to watch out for in general (you need std::map or MapVector to get determinism). Over here though, you sort the slices afterwards, so it's fine. | |
| 139–140 | Not this diff, but it's not ideal to be recomputing this for every single input file. Since you have to do this computation in createStaticLibrary as well now, perhaps you could make ArchCPUType and ArchCPUSubtype part of your Config, and then pass that into this function? | |
| 381 | Nit: newline above this. | |
| llvm/test/tools/llvm-libtool-darwin/universal-output.test | ||
|---|---|---|
| 5–6 | Rather than having two near-identical YAML blobs, use the -D option to pull out the different bits: # RUN: yaml2obj %s --docnum=1 -o %t.armv6 -DSUBTYPE=0x6 ... # RUN: yaml2obj %s --docnum=2 -o %t.armv7 -DSUBTYPE=0x9 ... There's a lot of extra noise in the two, which I suspect you can get rid of with a bit of effort. You don't need to have the YAML directly represent a real object you've made, as long as it's sufficient to trigger the code paths you need. For example, do you actually need the segment and section contents? | |
| llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp | ||
| 121 | Looks to me like this function is only partially tested? What about the other three CPUs and the default case? | |
| 357 | You should probably avoid auto here. It's not obvious from the immediate context what the type is. | |
| llvm/test/tools/llvm-libtool-darwin/universal-output.test | ||
|---|---|---|
| 5–6 | Thanks, I was able to get rid of section contents as well. However, I still need the segment command or it throws an "invalid object file" error (perhaps because of the string table below?). Updated it now. | |
| llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp | ||
| 121 | Ok also added test for CPU_TYPE_ARM64, CPU_TYPE_X86_64 and the default case. CPU_TYPE_ARM64_32 does not have a subtype so I didn't add a test for it. | |
| llvm/test/tools/llvm-libtool-darwin/universal-output.test | ||
|---|---|---|
| 55–67 | for CPU_TYPE_ARM64, I cannot test whether the architecture is present with llvm-lipo as it says "unknown(...)" for arm64e. So I did this check using llvm-objdump (whether the constituent members are present inside the fat file or not). Is this ok? To allow lipo to recognize arm64e I would have to make a modification under llvm/lib/Object/MachOObjectFile.cpp : case MachO::CPU_TYPE_ARM64:
switch (CPUSubType & ~MachO::CPU_SUBTYPE_MASK) {
case MachO::CPU_SUBTYPE_ARM64_ALL:
if (McpuDefault)
*McpuDefault = "cyclone";
if (ArchFlag)
*ArchFlag = "arm64";
return Triple("arm64-apple-darwin");
default:
return Triple();
}Not sure what would go under *McpuDefault = ... and return Triple("..."); for the case of arm64e? | |
| llvm/test/tools/llvm-libtool-darwin/universal-output.test | ||
|---|---|---|
| 55–67 | Apple hasn't fully upstreamed arm64e to LLVM yet. You can see their implementation of this at https://github.com/llvm/llvm-project-staging/blob/f0fd4f3af0acda5068acc5bbe10b3792c41bdfc4/llvm/lib/Object/MachOObjectFile.cpp#L2695-L2700. I think it's fine to use objdump for this for now; just add a comment about why. | |
| llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp | ||
| 121 | Can you have two files with a CPUType that falls under the default case but different CPUSubtypes, and confirm that they get put in the same slice of the output file? | |
| 357 | I agree that it's not obvious, but the expanded type for this (and for iterators in general) is also extremely verbose :/ I thought the general guideline was to use auto for range based for loops for that reason. Perhaps we could stick with the auto in the loop and assign the members of the pair to variables in the loop? (It looks like we're only using the second member anyway.) | |
| 358–359 | You want uint64_t, and you don't need the std::allocator member in the vector (that's the default). | |
| llvm/test/tools/llvm-libtool-darwin/universal-output.test | ||
|---|---|---|
| 5–6 | Thanks. I don't know enough about Mach-O to be able to tell you why the object reading code won't handle it without segments. For now, I think this is fine. | |
| llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp | ||
| 357 |
I wasn't aware that it was the guidance for all range based for loops? https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable seems to indicate auto where the context makes the type obvious, or the type is already abstracted away (usually applies for iterators). The trouble is here that I have no idea that NewMembers is even a DenseMap, let alone a DenseMap of integers to vectors of ArchiveMembers. Maybe assigning the member to a local variable could help. Are you able to just use std::pair too, or does that not work? You can get rid of the llvm:: prefixes everywhere here too. There's some concern elsewhere about using DenseMap with integer keys, because -1/-2 are reserved values that result in assertions if used. Probably not that important, but maybe you would be better off using unordered_map? | |
| llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp | ||
|---|---|---|
| 121 | Besides the 4 non-default cases below, the file llvm/lib/Object/MachOObjectFile.cpp supports the following CPU types (which will fall under default case):
However, for all these default CPU types, there is just a single subtype:
Thus, I am unable to have two files having the same CPU type that falls under default case but have different Subtypes :( | |
| 357 | ok, updated it to use unordered_map? How does this look now? | |
| llvm/test/tools/llvm-libtool-darwin/universal-output.test | ||
|---|---|---|
| 77 | Ah yes, you are right. Thanks! | |
| llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp | ||
| 357 | I guess not. If I do that, it doesn't compile and throws the following error: /home/sameerarora101/local/llvm-project/llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:354:10: required from here
/usr/include/c++/8/bits/stl_construct.h:75:7: error: use of deleted function ‘llvm::NewArchiveMember::NewArchiveMember(const llvm::NewArchiveMember&)’
{ ::new(static_cast<void*>(__p)) _T1(std::forward<_Args>(__args)...); } | |
LGTM
| llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp | ||
|---|---|---|
| 121 | Got it, yeah, I don't think there's anything we can do then. | |
Hi @sameerarora101,
sorry, but LLVM::universal-output.test test gets failed on the armv7 cross toolchain buider.
Here is a direct link to the failed test log
http://lab.llvm.org:8011/builders/llvm-clang-win-x-armv7l/builds/802
the first failed build itself
http://lab.llvm.org:8011/builders/llvm-clang-win-x-armv7l/builds/802
Would you take care of it?
Thank you.
That didn't work, but I'm able to repro locally using the CMake cache file used by the builder, so I'm digging into it.
Rather than having two near-identical YAML blobs, use the -D option to pull out the different bits:
There's a lot of extra noise in the two, which I suspect you can get rid of with a bit of effort. You don't need to have the YAML directly represent a real object you've made, as long as it's sufficient to trigger the code paths you need. For example, do you actually need the segment and section contents?