[DebugInfo] Support for `-gpubnames` option in `flang` #1033

SouraVX · 2021-04-16T07:15:33Z

This option is used to generate extra debug information.
i.e
DWARFv4
flang -gdwarf-4 -gpubnames test.c will generate .debug_pubnames and
.debug_pubtypes in the executable.
DWARFv5
flang -gdwarf-5 -gpubnames test.c will generate .debug_names in
the executable.

Prior to this change these sections were getting generated by default.

These sections were extra and may or maynot be used by the debugger. Hence
generation of these sections are bind to this -gpubnames option.
This behavior is analogous to clang, gcc, gfortran.

michalpasztamobica · 2021-04-16T07:41:05Z

Hi @SouraVX , I recently realized that our CI might not be exactly following the official building guidelines (or that the guidelines might have an error).
The flang build in the CI is using a native gcc/clang.
According to the official guidelines we should instead be using our own llvm/clang build, downloaded as an artifact from the previous step. Would you check if this makes a difference in your case?
@bryanpkc , @kiranchandramohan , would you be able to comment on this, please? I can provide a fix within minutes...

SouraVX · 2021-04-16T07:59:15Z

Thanks @michalpasztamobica for clarifying.

According to the official guidelines we should instead be using our own llvm/clang build, downloaded as an artifact from the previous step. Would you check if this makes a difference in your case?

Once driver CI completes the build, then I'll take those artifacts and use them to build/validate flang again locally. I hope this is what you meant ?

michalpasztamobica · 2021-04-16T08:10:13Z

Hi Sourabh,
Please see #1034 for what I mean - we might be using the wrong compiler in our CI.
No need to download the artifacts, would you please try to build and test locally but using the native compiler of your machine (whichever you previously used to build llvm and clang)?

SouraVX · 2021-04-16T08:15:12Z

Hi Sourabh,
Please see #1034 for what I mean - we might be using the wrong compiler in our CI.

Yea, Though not much of CI/yaml person, but I noticed the same :)

No need to download the artifacts, would you please try to build and test locally but using the native compiler of your machine (whichever you previously used to build llvm and clang)?

Thanks for clarifying again :) This is what we used locally for building/validating i.e, First build the Driver/CLANG once that completes used the artifact to build the flang then run make check-flang.

pawosm-arm · 2021-04-16T08:49:00Z

According to the official guidelines we should instead be using our own llvm/clang build, downloaded as an artifact from the previous step. Would you check if this makes a difference in your case? @bryanpkc , @kiranchandramohan , would you be able to comment on this, please? I can provide a fix within minutes...

@michalpasztamobica the only role of the guidelines is to provide you with a guidance, not a list of strict requirements. With this guidance some baseline was established. If someone was able to build flang its own way and it works for them, then fine. But when one runs into trouble (e.g. the resulting flang compiler is haunted by ICEs), then conformance to the guidelines is verified first. Also, these guidelines are not set in stone: if you observe that they result in a suboptimal or faulty product, they need to be revisited. They were not revisited for a very long time, apparently no one was in any immediate need for discussing them.

SouraVX · 2021-04-18T11:55:49Z

Once #1034 is merged, is there a way to re-trigger the CI builds ? That should make these CI bots green(Happy).
Re-triggering should happen in such a way that flang-compiler/classic-flang-llvm-project#32 should build first and rest things follow.

michalpasztamobica · 2021-04-19T06:26:56Z

@SouraVX , yes - it should be sufficient for you to rebase your branch on top of master, after #1034 is merged and then force-push the branch to github. This will trigger all the checks to run again with the CI fix in place.

michalpasztamobica · 2021-04-19T07:10:06Z

@SouraVX - now the CI is using the right compiler:

Run mkdir -p build && cd build
-- The C compiler identification is Clang 12.0.0
-- The CXX compiler identification is Clang 12.0.0

But the failure is still there... Do you see any difference between how the CI builds and runs tests and how you do it locally?

SouraVX · 2021-04-19T07:15:13Z

My test case is passing I think(Summary not listed in https://github.com/flang-compiler/flang/pull/1033/checks?check_run_id=2378342786) ?
This failure is different: At least NOT related to this change.

FAIL: Flang :: debug_info/debug_module_import.f90

SouraVX · 2021-04-19T07:32:03Z

@SouraVX - now the CI is using the right compiler:
Run mkdir -p build && cd build
-- The C compiler identification is Clang 12.0.0
-- The CXX compiler identification is Clang 12.0.0
But the failure is still there... Do you see any difference between how the CI builds and runs tests and how you do it locally?

Yes. but as a more intrusive check, how should one confirm the HEAD of the flang-driver is pointing at ? Which should be flang-compiler/classic-flang-llvm-project#32 - so that driver pass the appropriate bits to flag mapping to flang(flang1 & flang2)

michalpasztamobica · 2021-04-19T07:37:59Z

@SouraVX - you have a point, yes. What I can do is tweak my local fork to build your changes flang-compiler/classic-flang-llvm-project#32 and this PR, as I did previously and then I should be able to see that everything is passing. This will take a while, because the classic-flang-llvm-project on Github builds for more thatn 2 hours, but we should have the results available soon. If the failure is there - we can debug the CI configuration.

SouraVX · 2021-04-19T07:48:22Z

@michalpasztamobica, The process we follow internally, when such dependent changes are to be merged. We merge the parent change first (flang-driver). Once that is done we merge the child one. This may seems sequential but doesn't break things, since CI would automatically pick up the latest flang-driver.
flang-driver changes are less frequent compared to flang - hence above process seems reasonable to me :)

michalpasztamobica · 2021-04-19T07:53:03Z

Sure, we this would save me some hassle, indeed :) The failure looks like it could be caused by the missing changes from the dependent PR.

SouraVX · 2021-04-19T08:10:02Z

lowered strictness of a check in debug_module_import.f90, This should help.
Let's wait for, what CI bots has to say :)

SouraVX · 2021-04-19T08:20:03Z

Huh 💯 it worked, CI is in good shape. Thanks @michalpasztamobica :)

michalpasztamobica · 2021-04-19T08:31:55Z

lowered strictness of a check in debug_module_import.f90, This should help.

Is this a fix of the test or just a temporary workaround until we have the dependent PR available?

SouraVX · 2021-04-19T08:46:18Z

Is this a fix of the test or just a temporary workaround until we have the dependent PR available?

This PR is extending DICompileUnit metadata and the check for quite strict i.e

distinct !DICompileUnit({{.*}}, imports: ![[DBG_IMPORTS:[0-9]+]])

Notice last closing brace ), this is quite strict IMHO because this will cause this test to fail for every extension that may occur to DICompileUnit metadata.
After this PR:

distinct !DICompileUnit({{.*}}, imports: ![[DBG_IMPORTS:[0-9]+]], nameTableKind: None)

Usually we follow, not so strict style such that no test should be upgraded randomly since they failed due to there strict nature.
Argument supporting this proposition is that, most extensions LLVMM-IR/Metadata are incrementally version specific(i.e some metadata got introduced in LLVM 13). We should not be updating the older testcases just for the sake of it, because this may incur repercussions.
What do you guys think of it ? Please share your thoughts!

michalpasztamobica · 2021-04-19T11:02:24Z

EDIT: corrected testname.

@SouraVX , how come this PR (namely the new nametable.f90) is passing CI checks without the changes from flang-compiler/classic-flang-llvm-project#32?

I think this situation we are having here reveals another flaw in our CI setup worth discussing on the Wednesday call.
On every PR to classic-flang-llvm-project we run clang and llvm tests but we should perhaps also build and run latest flang master to check if the merge does not break flang builds? Bear in mind that flang's CI pulls the latest successful build from relevant classic-flang-llvm-project.

SouraVX · 2021-04-19T11:15:34Z

namely the new nametest.f90

You meant nametable.f90 ?

michalpasztamobica · 2021-04-19T11:46:22Z

@SouraVX , yes, my mistake sorry. I thought this test will only pass with the dependent changes?

SouraVX · 2021-04-19T15:35:12Z

It seems to me like, there might be some missing/mismatch/delta of pieces between upstream and downstream :(
Here's the log from our downstream flang
LLVM flow:
nameTableKind: None --> Do not generate the section.
NOTHING ---> Generate the section.

$ flang main.f90 -g -S -emit-llvm -o - | egrep "nameTableKind"
!4 = distinct !DICompileUnit(language: DW_LANG_Fortran90, file: !3, producer: " F90 Flang - 1.5 2017-05-01",
 isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !5, retainedTypes: !5,
 globals: !6, imports: !5, nameTableKind: None)

$ flang main.f90 -g -o -| llvm-readelf -S - | egrep "debug_"
  [26] .debug_loc        PROGBITS         0000000000000000  000010ce
  [27] .debug_abbrev     PROGBITS         0000000000000000  00001114
  [28] .debug_info       PROGBITS         0000000000000000  000011b5
  [29] .debug_str        PROGBITS         0000000000000000  00001287
  [30] .debug_line       PROGBITS         0000000000000000  00001318
$ flang main.f90 -g -gpubnames -S -emit-llvm -o - | egrep "nameTableKind"

$ flang main.f90 -g -gpubnames -o -| llvm-readelf -S - | egrep "debug_"
  [26] .debug_loc        PROGBITS         0000000000000000  000010ce
  [27] .debug_abbrev     PROGBITS         0000000000000000  00001114
  [28] .debug_info       PROGBITS         0000000000000000  000011b8
  [29] .debug_str        PROGBITS         0000000000000000  0000128a
  [30] .debug_pubnames   PROGBITS         0000000000000000  0000131b
  [31] .debug_pubtypes   PROGBITS         0000000000000000  0000133f
  [32] .debug_line       PROGBITS         0000000000000000  0000136b

Log for flang-fork: Section absent in both cases

$ trunk-flang/classic-flang-llvm-project/bin/flang main.f90 -g -S -emit-llvm -o - | egrep "nameTableKind"
!4 = distinct !DICompileUnit(language: DW_LANG_Fortran90, file: !3, producer: " F90 Flang - 1.5 2017-05-01",
 isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !5, retainedTypes: !5, globals: !6
, imports: !5, nameTableKind: None)

$ trunk-flang/classic-flang-llvm-project/bin/flang main.f90 -g && readelf -S a.out | egrep "debug_"
  [26] .debug_str        PROGBITS         0000000000000000  000061b1
  [27] .debug_loc        PROGBITS         0000000000000000  0000623c
  [28] .debug_abbrev     PROGBITS         0000000000000000  000062ab
  [29] .debug_info       PROGBITS         0000000000000000  00006333
  [30] .debug_line       PROGBITS         0000000000000000  000063f0

$ trunk-flang/classic-flang-llvm-project/bin/flang main.f90 -g -gpubnames -S -emit-llvm -o - | egrep "nameTableKind"
!4 = distinct !DICompileUnit(language: DW_LANG_Fortran90, file: !3, producer: " F90 Flang - 1.5 2017-05-01",
 isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !5, retainedTypes: !5, 
globals: !6, imports: !5, nameTableKind: None)
$ trunk-flang/classic-flang-llvm-project/bin/flang main.f90 -g -gpubnames && readelf -S a.out | egrep "debug_"

  [26] .debug_str        PROGBITS         0000000000000000  000061b1
  [27] .debug_loc        PROGBITS         0000000000000000  0000623c
  [28] .debug_abbrev     PROGBITS         0000000000000000  000062ab
  [29] .debug_info       PROGBITS         0000000000000000  00006333
  [30] .debug_line       PROGBITS         0000000000000000  000063f0

SouraVX · 2021-04-19T15:44:14Z

test/debug_info/nametable.f90

+!RUN: %flang %s -g -gpubnames -S -emit-llvm -o - | FileCheck %s --check-prefix=PUBNAMESECTION
+
+!Ensure that "nameTableKind: None" field is not present in DICompileUnit.
+!NAMESECTION-NOT: !DICompileUnit({{.*}}, nameTableKind: None


These checks should fail, catching if the nameTableKind: None is present when -gpubnames is specified. To my surprise this is still passing :(

I got the test to work correctly by not using -DAG checks.

michalpasztamobica · 2021-05-24T06:47:53Z

Hi @SouraVX,
In case this PR is still of interest for you...
I wonder if we could rebase flang-compiler/classic-flang-llvm-project#32 on top of flang-compiler/classic-flang-llvm-project@929e198. If it passes the Flang tests - merge it and then merge this PR. If it does not pass the Flang tests:

think of a way to make it pass with some temporary patch to flang in order not to break the CI and merge that flang patch first.
Merge [DebugInfo] Support of -gpubnames in Driver classic-flang-llvm-project#32
Merge this PR (which should be passing by then)

Does this make sense? I believe this suggestion would be in line with what @bryanpkc suggested in one of the Wednesday calls when we were discussing this topic.

SouraVX · 2021-05-24T09:10:47Z

Thanks for taking out time & revisiting this @michalpasztamobica :). I've pushed changes updated the testcase to be more specific.
Works great at my machine, let's see how CI Reacts.

Detailed log:

`flang` Built using `master` & Driver build using `release_11x` branch.
sourabh@llvm:~/work/dwarf/fortran$ ~/install/release/trunk-flang/bin/flang -gdwarf-5 nametable.f90 -c -o -| ~/install/release/trunk-flang/bin/llvm-readelf -S | egrep "debug_names"
sourabh@llvm:~/work/dwarf/fortran$  // **Section not present as expected, i.e without `-gpubnames` section should not be there.**

sourabh@llvm:~/work/dwarf/fortran$ ~/install/release/trunk-flang/bin/flang -gdwarf-5 -gpubnames nametable.f90 -c -o -| ~/install/release/trunk-flang/bin/llvm-readelf -S | egrep "debug_names"
  [16] .debug_names      PROGBITS        0000000000000000 0003d0 0000dc 00      0   0  4
  [17] .rela.debug_names RELA            0000000000000000 000b58 0000a8 18     25  16  8
// **Section present when `-gpubnames` is specified by the user.

SouraVX · 2021-05-24T09:13:03Z

@michalpasztamobica Is it possible for you to build the driver using PR: flang-compiler/classic-flang-llvm-project#32 and then build flang & trying this change locally ?

michalpasztamobica · 2021-05-24T09:25:57Z

@SouraVX , I can do it or I can try to hack the CI on my fork to do it for us :)

SouraVX · 2021-05-24T11:49:16Z

Rebased in hopes of CI pick up the parent Driver change first!

michalpasztamobica · 2021-05-24T11:50:19Z

I am running your commit on my fork: michalpasztamobica/classic-flang-llvm-project#4.
So far it is looking good.

SouraVX · 2021-05-24T11:52:40Z

I am running your commit on my fork: michalpasztamobica/classic-flang-llvm-project#4.
So far it is looking good.

Thank you @michalpasztamobica so much for taking out time & collaborating on this one!

SouraVX · 2021-05-24T13:57:41Z

CI all checks passes here in michalpasztamobica/classic-flang-llvm-project#4 thanks to @michalpasztamobica .
Now people can review the change. I've also provided manual verification of the patch log locally(See above comments), interested people can check the effect of the patch.

michalpasztamobica · 2021-05-24T18:38:49Z

I am running your commit on my fork: michalpasztamobica/classic-flang-llvm-project#4.
So far it is looking good.

Thank you @michalpasztamobica so much for taking out time & collaborating on this one!

No problem at all, it took me just a couple minutes to cherry-pick your changes and modify the script to run a different repo and branch. Feel free to use this trick in your branch to test such multi-repo builds :)

Thanks should go to github for letting us use their servers, which in fact did all the actual work 😄

SouraVX · 2021-06-08T16:42:43Z

PING!

kiranchandramohan · 2021-06-16T14:42:34Z

@SouraVX what do you need here to proceed? I guess the driver/llvm side changes have to be committed first?

SouraVX · 2021-06-16T14:45:39Z

@SouraVX what do you need here to proceed? I guess the driver/llvm side changes have to be committed first?

Yes, I think driver PR needed for other branches as well. I'll be doing this in a moment.

SouraVX · 2021-06-16T15:00:57Z

Once CI passes in all branches(i.e after merging of the Driver PR's) -- rebasing should be enough to trigger a CI build.

This option is used to generate extra debug information. i.e DWARFv4 `flang -gdwarf-4 -gpubnames test.c` will generate `.debug_pubnames` and `.debug_pubtypes` in the executable. DWARFv5 `flang -gdwarf-5 -gpubnames test.c` will generate `.debug_names` in the executable. Prior to this change these sections were getting generated by default. These sections were extra and may or maynot be used by the debugger. Hence generation of these sections are bind to this `-gpubnames` option. This behavior is analogous to `clang`, `gcc`, gfortran.

SouraVX · 2021-07-01T04:16:18Z

@kiranchandramohan @bryanpkc , All CI bots are green now :) Would you please take a look. Thank You!

bryanpkc

LGTM. Tests ran fine in our build.

SouraVX · 2021-07-06T05:35:15Z

@kiranchandramohan could you please have a look :)

kiranchandramohan

LGTM.

michalpasztamobica · 2021-07-07T07:37:37Z

I am wondering if we should advertise the merge of this PR somehow? It requires an update of the llvm-project build for make check-all to pass. I am afraid people will spend time looking at why some tests are failing, while they should just checkout and build newer llvm-project...

SouraVX · 2021-07-07T07:48:04Z

am wondering if we should advertise the merge of this PR somehow? It requires an update of the llvm-project build for make check-all to pass. I am afraid people will spend time looking at why some tests are failing, while they should just checkout and build newer llvm-project...

Thanks for the noting this, I'll put a note in Slack. Would that be Okay ? Otherwise folks subscribed/starred to this repo should also receiving this communication & from looking at PR they can easily figure out ?

michalpasztamobica · 2021-07-07T07:53:23Z

@SouraVX , I was thinking about the mailing list, but I think you are right that Slack channel is a better idea :) If people think otherwise they can ask for an e-mail to the mailing list :)

I think only people subscribed to this particular PR will receive updates from this PR. At least I am not getting notificiation from other PRs, just those that I contribute, comment or subscribe to.

SouraVX · 2021-07-07T08:00:42Z

@michalpasztamobica I think it's a matter of notification/starring the repo settings: I receive notifications for every event happening in this repo :)
Your settings might be the events you're participating/subscribed too :)

SouraVX mentioned this pull request Apr 16, 2021

[DebugInfo] Support of -gpubnames in Driver flang-compiler/classic-flang-llvm-project#32

Merged

SouraVX requested review from michalpasztamobica, alokkrsharma, bryanpkc, kiranchandramohan and pawosm-arm April 16, 2021 07:30

pawosm-arm approved these changes Apr 16, 2021

View reviewed changes

michalpasztamobica mentioned this pull request Apr 16, 2021

Github Action use the prebuilt clang to build flang #1034

Merged

SouraVX force-pushed the pubnames_flang branch from 5f5abbd to 949b6d0 Compare April 19, 2021 06:57

SouraVX force-pushed the pubnames_flang branch from 949b6d0 to a6240e7 Compare April 19, 2021 08:08

SouraVX commented Apr 19, 2021

View reviewed changes

SouraVX force-pushed the pubnames_flang branch from a6240e7 to 4f33077 Compare May 24, 2021 09:04

SouraVX force-pushed the pubnames_flang branch from 4f33077 to 3228672 Compare May 24, 2021 11:48

SouraVX requested a review from kiranchandramohan May 24, 2021 13:47

SouraVX mentioned this pull request Jun 2, 2021

Test pubnames rel11 michalpasztamobica/classic-flang-llvm-project#4

Open

SouraVX force-pushed the pubnames_flang branch from 3228672 to c85b7d5 Compare July 1, 2021 04:00

bryanpkc approved these changes Jul 1, 2021

View reviewed changes

kiranchandramohan approved these changes Jul 6, 2021

View reviewed changes

shivaramaarao merged commit 073966d into flang-compiler:master Jul 7, 2021

[DebugInfo] Support for -gpubnames option in flang #1033

[DebugInfo] Support for -gpubnames option in flang #1033

Conversation

SouraVX commented Apr 16, 2021

michalpasztamobica commented Apr 16, 2021

SouraVX commented Apr 16, 2021

michalpasztamobica commented Apr 16, 2021

SouraVX commented Apr 16, 2021

pawosm-arm commented Apr 16, 2021

SouraVX commented Apr 18, 2021

michalpasztamobica commented Apr 19, 2021

michalpasztamobica commented Apr 19, 2021

SouraVX commented Apr 19, 2021 • edited Loading

SouraVX commented Apr 19, 2021

michalpasztamobica commented Apr 19, 2021

SouraVX commented Apr 19, 2021

michalpasztamobica commented Apr 19, 2021

SouraVX commented Apr 19, 2021

SouraVX commented Apr 19, 2021

michalpasztamobica commented Apr 19, 2021

SouraVX commented Apr 19, 2021 • edited Loading

michalpasztamobica commented Apr 19, 2021 • edited Loading

SouraVX commented Apr 19, 2021

michalpasztamobica commented Apr 19, 2021

SouraVX commented Apr 19, 2021

SouraVX Apr 19, 2021

Choose a reason for hiding this comment

bryanpkc Apr 20, 2021

Choose a reason for hiding this comment

michalpasztamobica commented May 24, 2021 • edited Loading

SouraVX commented May 24, 2021

SouraVX commented May 24, 2021

michalpasztamobica commented May 24, 2021

SouraVX commented May 24, 2021

michalpasztamobica commented May 24, 2021

SouraVX commented May 24, 2021

SouraVX commented May 24, 2021 • edited Loading

michalpasztamobica commented May 24, 2021

SouraVX commented Jun 8, 2021

kiranchandramohan commented Jun 16, 2021

SouraVX commented Jun 16, 2021

SouraVX commented Jun 16, 2021

SouraVX commented Jul 1, 2021

bryanpkc left a comment

Choose a reason for hiding this comment

SouraVX commented Jul 6, 2021

kiranchandramohan left a comment

Choose a reason for hiding this comment

michalpasztamobica commented Jul 7, 2021

SouraVX commented Jul 7, 2021 • edited Loading

michalpasztamobica commented Jul 7, 2021

SouraVX commented Jul 7, 2021

[DebugInfo] Support for `-gpubnames` option in `flang` #1033

[DebugInfo] Support for `-gpubnames` option in `flang` #1033

SouraVX commented Apr 19, 2021 •

edited

Loading

SouraVX commented Apr 19, 2021 •

edited

Loading

michalpasztamobica commented Apr 19, 2021 •

edited

Loading

michalpasztamobica commented May 24, 2021 •

edited

Loading

SouraVX commented May 24, 2021 •

edited

Loading

SouraVX commented Jul 7, 2021 •

edited

Loading