nit changes

nit

nit

Added a test using APIs, moved getHeaderSize from protected to public. Aligns with other get APIs that are already public.

In D99698#2669581, @dblaikie wrote:

In D99698#2669200, @ayermolo wrote:

In D99698#2665550, @dblaikie wrote:

Test case?

I am not sure how to test it.

How about a unit test? We have some for the DWARF parser already.

In D99698#2665550, @dblaikie wrote:

Test case?

Thank You!

Oh right. Windows...
@dblaikie if you are busy I can put up a diff.
Doing should work I think.

1. CHECK: 0x11
2. CHECK-NEXT: f2()
3. CHECK-NEXT: splitdwarf-single-issue.cpp:7:3
4. CHECK-NEXT: main
5. CHECK-NEXT: splitdwarf-single-issue.cpp:13:3

Thanks for committing, and refactoring test. Didn't know about --defsym.

That would be great. I don't have commit access yet.

Renamed .s to same name as test.

@dblaikie You were right, it's possible. Just need to use non-relocated address and remove some sections. If all good, will commit.

Changed to use assembly.

Trying through ARC, removed modified file that snuck in.

Trying through arc.

In D96827#2623393, @dblaikie wrote:

Looks good - just a couple of minor fixes to make before committing.

(I assume by your patch that this is only reproducible with a linked binary? please confirm that (in a comment here) before committing)

Per discussion with @dblaikie going to go with this approach. Need to add test + see if we can avoid redundant checks.

Thanks!

Gotcha, thanks. Yeah I abandoned it on my end.

@MaskRay saw you marking this as "Needs revision", is that just a general way of not accepting diff?

From what I remember the discussion went back and forth with no real conclusion. I might be miss remembering, so please correct me if I am wrong.
This patch can be a short/medium term bridge to a more comprehensive solution. If I am understanding it correctly it also deals with a problem of sections like .debug_loc.

In D94141#2491925, @MaskRay wrote:

In D94141#2491909, @ayermolo wrote:

In D94141#2488729, @MaskRay wrote:

In D94141#2487960, @ayermolo wrote:

In D94141#2483728, @jhenderson wrote:

Could you give a step-by-step example of how you might use the information you want to emit? That will help us ensure that any proposals satisfy the requirements (or possibly to suggest an alternative).

Sure. From memory, as I was doing it last year.
I was looking in to relocation R_X86_64_DTPOFF32 out of range issue in the unknown code base in an older version of llvm. I don't quite remember all the details at this point, but it It was useful to see what the layout was produced at a glance, what each section address, and size was. Similar to what llvm-readelf -sections outputs. Specifically as it related to .tdata/.tbss. The original theory was that maybe one of them was growing too large or linker script was messing with layout. The output, redirected to file, of print-map for that build is around 2GB. The size of each section can be just searched. Figuring out how everything laid out would require a script. I thought that having lld output summary would benefit others also, so they don't have to write a script to parse out that from output of print-map.

You can use ld.lld ... -M | sed -En '/\w+ +\w+ +\w+ \S/p'

I have seen #include "\.\./ occasionally within a top-level llvm-project directory, but I don't think cross top-level directory should be allowed.

Thank you for sed command.
I tried it on output and it partially cleaned up output, there is still quite a bit of "typeinfo name for".
Probably command can be modified, but I think fundamentally this comes back to the same thing. Other users of lld need to know how to use sed, and if they are say on windows find alternative or write a parsing script.
This is an opt-in option that provides a better usage experience so that others don't need to re-invent the wheel.
Just a thought.

We accept new features very cautiously. Adding an option has maintenance costs and recognition cost if some options have overlapping features. You can dig up the history for previous options. They need to show values to the community. For this one, I don't think it provides any additional benefit and can just confuse users as it duplicates --print-map/-M. If you want the output, add --noinhibit-exec and inspect the output with readelf -S.

In D94141#2488729, @MaskRay wrote:

In D94141#2487960, @ayermolo wrote:

In D94141#2483728, @jhenderson wrote:

Could you give a step-by-step example of how you might use the information you want to emit? That will help us ensure that any proposals satisfy the requirements (or possibly to suggest an alternative).

Sure. From memory, as I was doing it last year.
I was looking in to relocation R_X86_64_DTPOFF32 out of range issue in the unknown code base in an older version of llvm. I don't quite remember all the details at this point, but it It was useful to see what the layout was produced at a glance, what each section address, and size was. Similar to what llvm-readelf -sections outputs. Specifically as it related to .tdata/.tbss. The original theory was that maybe one of them was growing too large or linker script was messing with layout. The output, redirected to file, of print-map for that build is around 2GB. The size of each section can be just searched. Figuring out how everything laid out would require a script. I thought that having lld output summary would benefit others also, so they don't have to write a script to parse out that from output of print-map.

You can use ld.lld ... -M | sed -En '/\w+ +\w+ +\w+ \S/p'

I have seen #include "\.\./ occasionally within a top-level llvm-project directory, but I don't think cross top-level directory should be allowed.

In D94141#2483728, @jhenderson wrote:

Could you give a step-by-step example of how you might use the information you want to emit? That will help us ensure that any proposals satisfy the requirements (or possibly to suggest an alternative).

rebased on latest.

Well print-map does have size and address information scattered throughout what is potentially quite a large output. I think it will be useful to have a summary that can be quickly referenced with all the information in one place.
Maybe split the difference and add it as part of -print-map as a summary that gets printed before?

@dblaikie @MaskRay Anything else do I need to change, all good?

To reproduce this config should do the trick:

This diff seems to be causing issue building llvm with clang-tools-extra;openmp projects enabled. Basically in self hoisting mode.

@tejohnson I was wondering where does this @anon.{{.*}} gets set, or in general where can I find more information about it.

In D93055#2461829, @JonChesterfield wrote:

Ah, yes. That makes sense. Building llvmsupport without PIC is a reasonable thing to do, and will cause problems when linked into libomptarget. One solution is to link libomptarget against a dynamically built llvmsupport, which might mean building llvmsupport twice. I don't know whether the existing cmake is wired up to do that.

I'd prefer we default to profiling disabled (and the llvm libs not linked in) for now.

(actually my preferred solution is to link libomptarget statically, but that feels like a longer term goal)

@ggeorgakoudis In our local build environment I am now observing error below:

From: David Blaikie via Phabricator <reviews@reviews.llvm.org>
Sent: Monday, December 7, 2020 11:08 AM
To: Alexander Yermolovich <ayermolo@fb.com>; llvm-commits@lists.llvm.org <llvm-commits@lists.llvm.org>
Cc: Hongtao Yu <hoy@fb.com>; jan_svoboda@apple.com <jan_svoboda@apple.com>; stevenwu@apple.com <stevenwu@apple.com>; dany.grumberg@gmail.com <dany.grumberg@gmail.com>; Wenlei He <wenlei@fb.com>; dexonsmith@apple.com <dexonsmith@apple.com>; cfe-commits@lists.llvm.org <cfe-commits@lists.llvm.org>; maskray@google.com <maskray@google.com>; ikudrin@accesssoftek.com <ikudrin@accesssoftek.com>; bhuvanendra.kumarn@amd.com <bhuvanendra.kumarn@amd.com>; mlekena@skidmore.edu <mlekena@skidmore.edu>; blitzrakete@gmail.com <blitzrakete@gmail.com>; shenhan@google.com <shenhan@google.com>; orlando.hyams@sony.com <orlando.hyams@sony.com>
Subject: [PATCH] D90507: [Driver] Add DWARF64 flag: -gdwarf64

In D90507#2398654, @ikudrin wrote:

It looks like lld/test/COFF/lto-new-pass-manager.ll.obj was added to the patch by accident and should be removed.

In D90507#2394109, @wenlei wrote:

This change covers non-LTO cases. For LTO, I think we would need to pass it from driver to LTO. Something like this: tools::addLTOOptions -> lld -> lto::Config (Config->TargetOptions->MCTargetOptions) ->LTO Backend.

In D90507#2393434, @jansvoboda11 wrote:

Hi @ayermolo, do you think this might be triggered by D82756? (my only upstream patch ATM)

In D91404#2394228, @MaskRay wrote:

In D91404#2393750, @avl wrote:

Well, this is a bit different from my original idea but is an overall good heuristic for many of the debug sections. It works for .debug_info, which is one of the biggest sections; it does not work for .debug_line, though, which is not that big as .debug_info, but potentially might become problematic in the (distant) future; it also does not work for .debug_abbrev, .debug_addr, .debug_ranges, and some others, which are usually not that big. However, the patch should be extended to support .debug_str, which can be even larger than .debug_info.

Would it be useful if decision done for the debug_info section would also be applied to all other debug sections from the same object file?

In D91404#2393718, @ikudrin wrote:

Well, this is a bit different from my original idea but is an overall good heuristic for many of the debug sections. It works for .debug_info, which is one of the biggest sections; it does not work for .debug_line, though, which is not that big as .debug_info, but potentially might become problematic in the (distant) future; it also does not work for .debug_abbrev, .debug_addr, .debug_ranges, and some others, which are usually not that big. However, the patch should be extended to support .debug_str, which can be even larger than .debug_info.

We either mark InputFile or InputSectionBase referenced by a 64-bit absolute relocation. I prefer InputBaseBase which is more direct (we have spare bits in SectionBase after keepUnique).

DWARF v4 .debug_str is referenced by .debug_info's non-first relocation. The InputSectionBase based referenced bit approach does not work. So maybe we have to use InputFile.

@jansvoboda11 Can you take a look? The failing test is unrelated it seems. Fails locally even without my change.

The failure seems unrelated to my changes. Trying locally it fails with latest upstream even without my changes.

In D90507#2378015, @ikudrin wrote:

1. The patch needs tests to check the added functionality.
2. DWARF64 can be generated only for a limited number of targets. There should be diagnostics for invalid switch combinations to prevent misuse. @MaskRay mentioned that in the patch for llc, D87011#2254749, but that makes a lot more sense for user-level tools.

In D87011#2379609, @dblaikie wrote:

In D87011#2379603, @ayermolo wrote:

In D87011#2378533, @ikudrin wrote:

In D87011#2378347, @jhenderson wrote:

At least in LLD, it's not quite as simple as being added after the user's code: if a library appears on the link line it will be included in the output order as soon as it is determined it is needed. Thus if you have have three modules 1.o, 2.o, and 3.o, with 3.o in an archive 3.a and 1.o requiring 3.o, you end up with an output order of 1.o 3.o 2.o if the input order was 1.o 3.a 2.o or 3.a 1.o 2.o or an output order of 1.o 2.o 3.o if the input order was 1.o 2.o 3.a. In fact, with use of the --undefined linker switch, you can even force 3.o to appear first.

I accept using --undefined or rearranging the command-line order is less than ideal, but I'm really not convinced LLD should have any place in parsing the DWARF to determine output order. Furthermore, it's not even a reliable solution - if the objects built with DWARF32 (potentially all of which might have come from libraries) are large enough, no amount of reordering will fix the behaviour. I think users who need DWARF64 in their libraries are just going to have to request DWARF64 versions of the libraries, if the --undefined and reordering command line options are insufficient.

I'd guess that for a large-scale project the recommendation to use -u would be unrealistic. We are talking about projects where debugging information in a single section can easily go beyond the 4GiB limit; it is impossible for the developer to adjust the command line manually.

By the way, from a semantic point of view, I don't think it matters if the DWARF is in a different order to the data it represents - I'm just concerned about the maintenance and performance burden of having to parse the DWARF to achieve this reordering.

There is no need to parse the debug info sections. Reading only the first 4 bytes of .debug_info is enough to assess the format (there might be input files with format intermixing, but we can ignore them in the sack of simplicity). And we do not need any automatic sorting if the size of an output section is less than 4GiB.

Exactly. Not to mention, I think for users that actually worry about 4Gig limit they have pretty complex build system that will need to be modified to get build order right. Probably doable, but looking at overall compilation pipeline, is it really the best approach? Within lld we don't have to parse entire debug section, just read few bytes in each CU to determine if it's 32 or 64 bit.
Yes theoretically it is possible that there are just so many third party libraries that they will over flow 4gig by themselves, but I think common case is they will be under 4 gigs.

FWIW, this is probably a big enough discussion to deserve it's own review, probably even it's own llvm-dev thread. My personal take would be: Unless there's a specific user who needs this, probably not worth building it. If you personally have a need or support users who need it, that swings the discussion a fair bit into "what's the best way we can help these users".

In D87011#2378533, @ikudrin wrote:

In D87011#2378347, @jhenderson wrote:

At least in LLD, it's not quite as simple as being added after the user's code: if a library appears on the link line it will be included in the output order as soon as it is determined it is needed. Thus if you have have three modules 1.o, 2.o, and 3.o, with 3.o in an archive 3.a and 1.o requiring 3.o, you end up with an output order of 1.o 3.o 2.o if the input order was 1.o 3.a 2.o or 3.a 1.o 2.o or an output order of 1.o 2.o 3.o if the input order was 1.o 2.o 3.a. In fact, with use of the --undefined linker switch, you can even force 3.o to appear first.

I accept using --undefined or rearranging the command-line order is less than ideal, but I'm really not convinced LLD should have any place in parsing the DWARF to determine output order. Furthermore, it's not even a reliable solution - if the objects built with DWARF32 (potentially all of which might have come from libraries) are large enough, no amount of reordering will fix the behaviour. I think users who need DWARF64 in their libraries are just going to have to request DWARF64 versions of the libraries, if the --undefined and reordering command line options are insufficient.

I'd guess that for a large-scale project the recommendation to use -u would be unrealistic. We are talking about projects where debugging information in a single section can easily go beyond the 4GiB limit; it is impossible for the developer to adjust the command line manually.

By the way, from a semantic point of view, I don't think it matters if the DWARF is in a different order to the data it represents - I'm just concerned about the maintenance and performance burden of having to parse the DWARF to achieve this reordering.

There is no need to parse the debug info sections. Reading only the first 4 bytes of .debug_info is enough to assess the format (there might be input files with format intermixing, but we can ignore them in the sack of simplicity). And we do not need any automatic sorting if the size of an output section is less than 4GiB.

@ikudrin Can you elaborate on LLD changes. I recently started to look in to it. Reason I am interested in this, internally we are a looking in to using DWARF64, so I have bandwidth, and incentive, to help with it implementation and adoption.

@ikudrin What else is left on clang side? I added a diff for passing a flag from clang to be, need to see what's up with failures, anything else that needs to be done?
Testing it locally I was able to generate binary with DWARF64, that llvm-dwarf is able to parse it. Looks like gdb and lldb support varies.

Awesome, thanks.
I was trying to pass in -dwarf64 through our build system, but was still seeing 32 bit relocations. Will dig further on my end.
Thanks for working on this.

@ikudrin To clarify this will emit R_X86_64_64 bit relocations for .debug_info on 64 bit platform, correct?

Thank you @craig.topper @wenlei

@craig.topper Any more questions/concerns? Can you approve?

@avl Would it be possible to rebase this on latest, or is this diff abandoned?

We are linking against Intel Composer XE in to which svml libraries were added. I do believe they are from icc.

@wenlei Thanks!

Hi, can someone take a look at this please?