forgot to include a paren change in the previous diff

fix an assert and comment

Add assert

$(RM) a.o b.o

Remove more ARCHIVE_NAME

In D94890#2510627, @JDevlieghere wrote:

In D94890#2510446, @MaskRay wrote:

Is this good? :)

I think you forgot to update the patch?

Inline archive rule

In D94882#2509022, @jhenderson wrote:

Okay, I'm persuaded by the arguments. I don't really mind whether the tool will ignore an explicitly specified version or not, as long as a warning is emitted if it does ignore it. I think we want to avoid the potential confusion of "I asked for DWARFv3, but am getting DWARFv5 output", and a warning would at least help with that (assuming of course we don't just refuse to handle this case, which I think is also an acceptable, and possibly preferable solution).

Is this good? :)

In D94925#2505292, @sbc100 wrote:

BTW, do you know if we can remove the rules that add espindola (who has left the project IIUC) from all lld/ELF changes?

LGTM.

In D94890#2507856, @JDevlieghere wrote:

In D94890#2507241, @MaskRay wrote:

In D94890#2505988, @labath wrote:

Looks like a nice cleanup. The only part I am not sure of is the part about removing $(RM) $(ARCHIVE_OBJECTS). Is that necessary?
I'm not sure why is that line there, but if I had to guess, I would say it's to ensure that lldb (on macos) reads debug info from the archive file instead of the original .o files. If it's not required, it may be better to leave it in. Otherwise, someone from Apple should say whether that is ok (testing archives is only really interesting on fruity platforms).

I can add back it under the ifeq "$(OS)" "Darwin" guard if Apple folks think it is useful.

It looks like we have only one test on llvm.org (+ one additional test in the Swift fork) that's using this. My vote is to just remove this together with the ARCHIVE_C_SOURCES, ARCHIVE_CXX_SOURCES, ARCHIVE_OBJC_SOURCES and ARCHIVE_OBJCXX_SOURCES and inline it in that one test.

LGTM. If --allow-unused-prefixes=false tests are more than --allow-unused-prefixes=true tests, it is probably time to flip FileCheck default.

In D94882#2507660, @dblaikie wrote:

A few thoughts:

1. If gnu-as already has accepted such a patch, we probably want to do the same just for compatibility

In D94890#2505988, @labath wrote:

Looks like a nice cleanup. The only part I am not sure of is the part about removing $(RM) $(ARCHIVE_OBJECTS). Is that necessary?
I'm not sure why is that line there, but if I had to guess, I would say it's to ensure that lldb (on macos) reads debug info from the archive file instead of the original .o files. If it's not required, it may be better to leave it in. Otherwise, someone from Apple should say whether that is ok (testing archives is only really interesting on fruity platforms).

In D94391#2495056, @dblaikie wrote:

I'll leave this one to @aprantl

Delete if os.path.isdir("/usr/include/c++/v1"):

In D93190#2491770, @smeenai wrote:

@compnerd is already part of the libc++abi group, but I'm also adding him explicitly in case he has the time to look, since he has a lot of relevant experience.

In D94882#2506029, @jhenderson wrote:

In D94882#2505699, @dblaikie wrote:

In D94882#2504207, @jhenderson wrote:

Maybe a silly suggestion (and one that would need more discussion on the mailing lists), but why not just change the default version to 5 more widely? How often do people deliberately deviate from the default DWARF version in practice? To me, the proposed change feels like a bit of a hack for what is likely fairly unusual usage, and is only temporary, since likely most toolchains will want to change to DWARF v5 at some point going forward.

ie: you're suggesting users needing to use DWARFv4 line tables would access that functionality with a flag passed to the assembler (as they do today if they want to opt in to DWARFv5)?

Right, yes, that's what I'm suggesting.

I think part of the problem is that people aren't used to (ie: build systems and other infrastructure isn't designed for) having to pass a dwarf version to their assembler - the line table format hasn't changed since DWARFv2. So if you have run -gN -S, you can pass the resulting file to the compiler/assembler without any flags. So even if we switch the default - then people who aren't ready to switch to DWARFv4 are going to be in a possibly difficult situation of trying to teach their build system how to pass the appropriate flags to the assembler, etc.

I think maybe the way to think about it is that there wouldn't be a default DWARF version - it could always be determined by the presence/absence of a file 0.

I'm concerned about relying on what is essentially a minor detail of the specification, for a couple of reasons:

1. What if the .file 0 directive isn't present for future DWARF versions (e.g. DWARFv6), for whatever reason? You might have some code that is generated expecting DWARFv6, yet the assembler will end up generating (or at least trying to generate) DWARFv4 output. It seems like relying on a detail like this is fragile and not particularly future-proof.

In D94888#2505992, @labath wrote:

It looks like this is removing the ability to build libc++ tests with gcc (as it does not have the -stdlib option). While having that ability would be nice, I don't believe there's anyone currently using that configuration, so it shouldn't stand in the way of other things. But we should also update the python detection code then (in canRunLibcxxTests in packages/Python/lldbsuite/test/dotest.py -- I guess you just need to remove the if os.path.isdir("/usr/include/c++/v1"): blurb)

As for testing against the system libc++ with clang, I guess that should still work, as the extra rpath will be just ignored in that case...

Don't use double-colon. https://lists.gnu.org/archive/html/help-make/2021-01/msg00016.html

Theoretically the diagnostic can be retained: we need another variable beside MCContext::DwarfVersion or making MCContext::DwarfVersion Optional<int>; and carefully doing similar thing on Clang cc1as side.

comments

Thanks for looking into llvm-nm -D😊

You may add tests to test/ELF/linkerscript/output-too-large.s. You'll need at least another output section to demonstrate the effects.

In D94542#2501267, @reames wrote:

In D94542#2500145, @MaskRay wrote:

The value of -mbranches-within-32B-boundaries also dilutes over time (it mitigates issues for some Skylake architectures).
(Surprising to me, this feature has been available for one year but I don't see a lot of adoption).

The performance impact of the microcode fix were highly variable depending on the exact details of each workload. Mostly barely moved, some really suffered. I know of a couple of organizations using the mitigation, but I agree with your general point about this being something that decays in value with time. Hopefully, in a few years, we can stop talking about this.

I want to have a feeling how the aix assembly works, so I tried binutils-gdb:

Drop DW_AT_decl_file as well

https://bugs.llvm.org/show_bug.cgi?id=42138 was a BranchFolding bug which was probably found by an automating framework. @condy reopened it in #c13 because the symptom looks similar.
I closed 42138 in favor of the X86AsmBackend dedicated https://bugs.llvm.org//show_bug.cgi?id=48742 .

In D94735#2500129, @dblaikie wrote:

Ah, seems I was confused by the patch title - it looks like these values aren't used for FlagFwdDecl records - forward declarations bail out 3356, not using the file/line created on 3338/3339.

Seems like this change might lead to a situation of files but no line numbers, which seems a bit strange to me - well, at least for __va_list_tag, which isn't anymore associated with the file than with the line. Perhaps both should be addressed?

diff

-  [30] .debug_abbrev     PROGBITS        0000000000000000 ec7533c 616a22 00      0   0  1
-  [31] .debug_info       PROGBITS        0000000000000000 f28bd5e 50f9770b 00      0   0  1
-  [32] .debug_ranges     PROGBITS        0000000000000000 60223469 285e9b0 00      0   0  1
-  [33] .debug_str        PROGBITS        0000000000000000 62a81e19 ce211e0 01  MS  0   0  1
-  [34] .debug_line       PROGBITS        0000000000000000 6f8a2ff9 8a5ea2d 00      0   0  1
-  [35] .symtab           SYMTAB          0000000000000000 78301a28 156db60 18     37 821346  8
-  [36] .shstrtab         STRTAB          0000000000000000 7986f588 000168 00      0   0  1
-  [37] .strtab           STRTAB          0000000000000000 7986f6f0 69ab553 00      0   0  1
+  [30] .debug_abbrev     PROGBITS        0000000000000000 ec7533c 618ce4 00      0   0  1
+  [31] .debug_info       PROGBITS        0000000000000000 f28e020 50f97700 00      0   0  1
+  [32] .debug_ranges     PROGBITS        0000000000000000 60225720 285e9b0 00      0   0  1
+  [33] .debug_str        PROGBITS        0000000000000000 62a840d0 ce211e0 01  MS  0   0  1
+  [34] .debug_line       PROGBITS        0000000000000000 6f8a52b0 8a5ea2d 00      0   0  1
+  [35] .symtab           SYMTAB          0000000000000000 78303ce0 156db60 18     37 821346  8
+  [36] .shstrtab         STRTAB          0000000000000000 79871840 000168 00      0   0  1
+  [37] .strtab           STRTAB          0000000000000000 798719a8 69ab553 00      0   0  1

In D93881#2499664, @jcai19 wrote:

But I think we should do better: instead of: (1) create a temp file (2) open it by name again (3) check it is a regular file with one hard link & it is writable (S_IWUSR) (4) fstat (5) fchown,
we should just modify the destination in place (for objcopy file and strip file). If the file has been mapped readonly, on many platforms it may be unwritable. We have to accept this.

@MaskRay I took a look at the how ELF binaries are handled and from what I understand, the code directly copies the content of the input file into temporary file (https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-objcopy/ELF/Object.cpp#L1786), before replacing the output file with it. So unless we are willing to allocate memory space to save its content (which will be very costive for large binaries), it does not appear modifying the input file in place is easy. And if we ever decide to take that path, it looks like we would have to make a lot of changes as the code of interest is buried deeply in the call hierarchy, and in addition we would likely have to change how the other file formats handle such cases to be consistent. Please let me know if I misunderstood your proposal or overlooked anything, but so far it appears to me the best place to preserve the file ownership without major change to the code is in the proposed place.

When -mbranches-within-32B-boundaries (to mitigate microcode update for Intel JCC Erratum) is used, there are many alignment fragments. I think D75203 has advantage in that case.
In the absence of -mbranches-within-32B-boundaries, I don't think there is demonstrable improvement. I think the assembler relaxation does not pull its weight.
I'll wait another two days.

LGTM.

In D94647#2499153, @inglorion wrote:

Can we not just make -gsplit-dwarf set the DwarfFissionKind without also looking at other things? That way, we don't have to worry about detecting the right mix of -g, -g2, -fthinlto-index, -x ir, bitcode input, etc. If there is some reason why this doesn't work, I would also like it to be expressed in a comment in the code so that someone doesn't come along and simplify/break the code later on.

-fno-plt essentially inlines the PLT entry into the call site. For x86-64, there are benefits if many call sites are defined in different DSOs.

In D94655#2498669, @dblaikie wrote:

In D94655#2498548, @MaskRay wrote:

In D94655#2498504, @dblaikie wrote:

Is there any way to condition this on the type of the output, rather than the input? (or, more specifically, on whether machine code is being generated)

Or maybe we could always pass the split-dwarf-file down through LLVM and not need to conditionalize it at all? It'd be a no-op if there's no DWARF in the IR anyway?

I tried replacing if (IRInput || Args.hasArg(options::OPT_g_Group)) { with if (1), -gsplit-dwarf may produce .dwo for regular non-g .c compile.

Are you saying that if you make that change -gsplit-dwarf does cause .dwo files to be created for non-g .c compiles? Do the dwo files have anything in them? I had modified llvm to dynamically choose split or non-split based on whether there was enough data to be worth splitting into a .dwo file, but I guess that situation might still be producing an empty .dwo file which isn't ideal - I haven't tested that.

Stops dumping default values for Version, Flags, VersionNdx and Hash fields.

LSB 1.3 is superseded. The description can link to https://refspecs.linuxfoundation.org/LSB_5.0.0/LSB-Core-generic/LSB-Core-generic/symversion.html instead

LGTM.

In D94655#2498504, @dblaikie wrote:

Is there any way to condition this on the type of the output, rather than the input? (or, more specifically, on whether machine code is being generated)

Or maybe we could always pass the split-dwarf-file down through LLVM and not need to conditionalize it at all? It'd be a no-op if there's no DWARF in the IR anyway?

Thanks. The new name looks better! I mentioned the patch on discord. Hope one day or two is sufficient for interested folks to weigh in.

In D94647#2497170, @dblaikie wrote:

In implicit thinlto this seemed to work for me without any changes:

$ clang++-tot -flto=thin test.cpp -g -c
$ clang++-tot -flto=thin -fuse-ld=lld -gsplit-dwarf test.o && llvm-dwarfdump-tot a.out
a.out:  file format elf64-x86-64

.debug_info contents:
0x00000000: Compile Unit: length = 0x0000002c, format = DWARF32, version = 0x0004, abbr_offset = 0x0000, addr_size = 0x08 (next unit at 0x00000030)

0x0000000b: DW_TAG_compile_unit
              DW_AT_stmt_list   (0x00000000)
              DW_AT_comp_dir    ("/usr/local/google/home/blaikie/dev/scratch")
              DW_AT_GNU_dwo_name        ("a.out_dwo/1.dwo")
              DW_AT_GNU_dwo_id  (0xf29563d7c812deae)
              DW_AT_low_pc      (0x0000000000201730)
              DW_AT_high_pc     (0x0000000000201738)
              DW_AT_GNU_addr_base       (0x00000000)

Any idea why that worked without the need for a special case/changes? Might be nice if both these use cases (implicit and explicit thinlto) somehow managed to use more common code generation options. But I guess that's too different - probably some generic "take all the clang flags and pass them through to the linker to pass back to the compiler", which isn't really relevant to this... well, maybe (if they're being passed back to the compiler in implicit thin lto, how does implicit thin lto work correctly without the -g flag?)

improve code comment