1475562 - Investigate using PDB files in mingw clang builds

Status: RESOLVED FIXED

(Firefox Build System :: General: Unsupported Platforms, enhancement, P5)

Description

[Jacek Caban]

clang generally supports PDB files, but there are still some limitations, see:
https://github.com/mstorsjo/llvm-mingw/blob/master/README.md#pdb-support

I tried building it with Firefox. With following in mozconfig:
CFLAGS="-gcodeview"
CXXFLAGS="-gcodeview"
LDFLAGS="-Wl,-pdb,debug.pdb"

It generated .pdb files and built (although I had to disable module order checking in toolkit/library/libxul.mk), but the resulting binary was reported as broken by Windows. It starts fine on Wine. It needs more investigation.

Comment 1

[Jacek Caban]

Here is a try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f23c60bfc98b9081d1b8ad1d4f493176392c608&selectedJob=188059410

Comment 2

[Jacek Caban]

Note that it's just a hack, so generated .pdbs are not in artifacts, that would require proper build config integration. They are in build dirs that were used to link binaries.

Comment 3

[(Away)]

Looking at `dumpbin -headers firefox.exe`, one thing that stands out is some unknown flags in section headers, e.g.:

SECTION HEADER #1
   .text name
[...]
60500020 flags
         Code
         RESERVED - UNKNOWN
         RESERVED - UNKNOWN
         Execute Read

Compared to an official Nightly:

SECTION HEADER #1
   .text name
[...]
60000020 flags
         Code
         Execute Read

The same added 0x00500000 flags appear in other sections too: .rdata, .data, etc.

According to https://docs.microsoft.com/en-us/windows/desktop/api/winnt/ns-winnt-_image_section_header these don't make any sense for an exe:

0x00100000 IMAGE_SCN_ALIGN_1BYTES 
Align data on a 1-byte boundary. This is valid only for object files.
0x00400000 IMAGE_SCN_ALIGN_8BYTES 
Align data on a 8-byte boundary. This is valid only for object files.

Comment 4

[(not currently active) Ted Mielczarek]

(In reply to Jacek Caban from comment #2)
> Note that it's just a hack, so generated .pdbs are not in artifacts, that
> would require proper build config integration. They are in build dirs that
> were used to link binaries.

We've got two ways to do this now. The first is with the crashreporter, which is how we do it for our tier-1 builds. Building the crashreporter code might be a bit of a PITA for you since the bits we actually need to make this work are dump_syms, which uses the DIA SDK. The second is a bit of a hack we use for Address Sanitizer builds that build with --disable-crashreporter, they simply copy the .pdb files alongside the exe and dll files in dist/bin and package them up.

Comment 5

[(Away)]

Is this fixed by https://reviews.llvm.org/rL337598 ?

Comment 6

[Martin Storsjö]

(In reply to David Major [:dmajor] from comment #3)
> Looking at `dumpbin -headers firefox.exe`, one thing that stands out is some
> unknown flags in section headers, e.g.:
> 
> SECTION HEADER #1
>    .text name
> [...]
> 60500020 flags
>          Code
>          RESERVED - UNKNOWN
>          RESERVED - UNKNOWN
>          Execute Read

These usually are added by binutils objcopy/strip; were some strip run on the produced binaries without the recent fixes to LLD?

Comment 7

[Tom Ritter [:tjr]]

Attachment: add pdbs.patch

Alright, I have pdbs to test to see if they're working.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5cceea9fbffbf7b557c5510963df4ecfa23814a

I have a patch I'm asking for feedback on. It's not ready because I have to investigate this nsmodule thing; but aside from that let me know how I should change the build system bits.

Comment 8

[Jacek Caban]

I tested it locally with Tom's patch, clang and libunwind updated to make sure we use all Martin's fixes and using SEH on win64 (patches attached). xul.pdb could be successfully loaded by windbg and it produced useful backtraces.

Comment 9

[Jacek Caban]

Attachment: 0001-Use-pre-8-current-trunk-clang-version.diff

This should probably rename file and toolchain names.

Comment 10

[Jacek Caban]

Attachment: 0002-Use-SEH-for-exception-handling-on-x86_64.diff

Comment 11

[Jacek Caban]

Here is treeherder run (without PDB patches):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b582d6c2152608c32c4cca4167d2b5a86f44ab1

Comment 12

[(Away)]

Comment on attachment 9013006 [details] [diff] [review]
add pdbs.patch

I would call the variable MOZ_-something just so it doesn't get confused for a link.exe feature. In terms of the actual plumbing of the change, I don't know the surrounding code well enough, so I defer to ted.

Comment 13

[Jacek Caban]

Shouldn't CFLAGS/CXXFLAGS be set in configure script instead of mozconfig?

Comment 14

[Tom Ritter [:tjr]]

(In reply to Jacek Caban from comment #13)
> Shouldn't CFLAGS/CXXFLAGS be set in configure script instead of mozconfig?

Oh; so for local builds all you need is --enable-debug, and not setting CFLAGS in your mozconfig? 

Yes that makes sense. I will change that.

Comment 15

[Tom Ritter [:tjr]]

Attachment: make pdbs for mingw-clang

Comment 16

[Tom Ritter [:tjr]]

Neither x86_64-mingw-w64-nm nor llvm-nm will produce symbols when run on xul.dll after this patch. They both did before... is this expected? A bug? (Maybe something fixed in a later revision than the one we're using, which is clang 7 final?)

Comment 17

[Martin Storsjö]

(In reply to Tom Ritter [:tjr] from comment #16)
> Neither x86_64-mingw-w64-nm nor llvm-nm will produce symbols when run on
> xul.dll after this patch. They both did before... is this expected? A bug?
> (Maybe something fixed in a later revision than the one we're using, which
> is clang 7 final?)

I think this is expected; when using a PDB file, the symbol/source location information is stored there instead of in the PDB file. I guess I could see if I can extend LLD with a mode where it writes debug info into a PDB but still writes a normal symbol table into the final module (similar to the -S flag).

But for what purposes do you need nm to print symbols for a DLL? If you just want to check the list of exported symbols, e.g. llvm-readobj -coff-exports should achieve the same.

Comment 18

[Jacek Caban]

Agreed, llvm-readobj would be better. There is also winedump that does that and would be slightly easier to parse, but using a tool that is part of LLVM is better as a dependency.

Comment 19

[Tom Ritter [:tjr]]

Attachment: Bug 1475562 Produce pdbs for the mingw-clang build job

Okay, this patch is ready for review. I'll be filing a followup bug for improving the pdbs...

Comment 20

[Tom Ritter [:tjr]]

Attachment: Bug 1475562 Produce pdbs for the mingw-clang build job

Adding -g in addition to gcodeview, per Bug 1497645.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=844b8dfa48621e31e1d693479de1f45cddfab72f&selectedJob=204322449

Comment 21

[Nathan Froyd [:froydnj]]

Comment on attachment 9015696 [details] [diff] [review]
Bug 1475562 Produce pdbs for the mingw-clang build job

Review of attachment 9015696 [details] [diff] [review]:
-----------------------------------------------------------------

I have questions.

::: browser/config/mozconfigs/win32/mingwclang
@@ +28,5 @@
>  # MinGW Stuff
>  ac_add_options --target=i686-w64-mingw32
> +# This must be an environment variable, rather than
> +# --with-toolchain-prefix, so check_binary.py can pick it up
> +TOOLCHAIN_PREFIX="i686-w64-mingw32-"

This can't be right; the code in check_binary.py already picks TOOLCHAIN_PREFIX out from buildconfig.substs...

::: build/moz.configure/toolchain.configure
@@ +2002,5 @@
>              )
> +        elif compiler.type == 'clang':
> +            return namespace(
> +                mkshlib=['$(CXX)', '$(DSO_LDOPTS)', '-Wl,-pdb,$(LINK_PDBFILE)', '-o', '$@'],
> +                mkcshlib=['$(CC)', '$(DSO_LDOPTS)', '-Wl,-pdb,$(LINK_PDBFILE)', '-o', '$@'],

Is -pdb an lld only option?

::: toolkit/mozapps/installer/packager.py
@@ +311,5 @@
>                                 LibSignFile(os.path.join(args.destination,
>                                                          libname)))
>  
> +    # If a pdb file is present, include it. Run on all OSes to capture MinGW builds
> +    for p, f in copier:

Does this copy pdb files for *all* builds, including our normal Windows ones?  I don't think we want to do that...or is this bit of code dependent on MOZ_COPY_PDBS?

Comment 22

[Tom Ritter [:tjr]]

(In reply to Nathan Froyd [:froydnj] from comment #21)
> This can't be right; the code in check_binary.py already picks
> TOOLCHAIN_PREFIX out from buildconfig.substs...

I guess I got some runs confused; because you're right, we don't need to use the env var.

> Is -pdb an lld only option?

It is; but mingw-clang exclusively uses lld.

> Does this copy pdb files for *all* builds, including our normal Windows
> ones?  I don't think we want to do that...or is this bit of code dependent
> on MOZ_COPY_PDBS?

It is not dependent on MOZ_COPY_PDBS; however the pdbs don't get bundled because we generate them in a different location.
Tests on https://treeherder.mozilla.org/#/jobs?repo=try&revision=8be85246a96c68aa33d31b4ee704a3871c77c050&selectedJob=204382167

Comment 23

[Tom Ritter [:tjr]]

Attachment: Bug 1475562 Produce pdbs for the mingw-clang build job

Comment 24

[(not currently active) Ted Mielczarek]

Comment on attachment 9015945 [details] [diff] [review]
Bug 1475562 Produce pdbs for the mingw-clang build job

Review of attachment 9015945 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/config/mozconfigs/win32/mingwclang
@@ +50,5 @@
>  CXXFLAGS="-fms-extensions"
>  CPP="$TOOLTOOL_DIR/clang/bin/i686-w64-mingw32-clang -E"
>  AR=llvm-ar
>  RANLIB=llvm-ranlib
> +mk_add_options "export MOZ_COPY_PDBS=1"

I would prefer making this an option in moz.configure somewhere so you can simply put `MOZ_COPY_PDBS=1` in mozconfigs. It should be as simple as:
```
option(env='MOZ_COPY_PDBS', help='...')
set_config('MOZ_COPY_PDBS', depends_if('MOZ_COPY_PDBS')(lambda _: True))
```

::: build/moz.configure/toolchain.configure
@@ +1147,5 @@
>      # Debug info is ON by default.
>      if compiler_info.type in ('msvc', 'clang-cl'):
>          return '-Zi'
> +    elif target.kernel == 'WINNT' and compiler_info.type == 'clang':
> +        return '-g -gcodeview'

Does this really need to include both? It seems like -gcodeview would override -g?

::: config/rules.mk
@@ +164,5 @@
>  endif
>  
> +ifneq ($(HOST_OS_ARCH),WINNT)
> +ifeq ($(OS_ARCH),WINNT)
> +MOZ_PROGRAM_LDFLAGS += -Wl,-pdb,$(dir $@)/$(LINK_PDBFILE)

It sucks that we have a totally separate set of flags for linking dlls and exes. :-/

@@ +810,5 @@
>  define syms_template
>  syms:: $(2)
>  $(2): $(1)
>  ifdef MOZ_CRASHREPORTER
> +ifeq ($(HOST_OS_ARCH),WINNT)

This bit seems wrong. We want to continue to dump symbols on other platforms!

::: python/mozbuild/mozbuild/action/check_binary.py
@@ +194,5 @@
> +        # MinGW-Clang, when building pdbs, doesn't include the symbol table into
> +        # the final module. To get the NSModule info, we can look at the exported
> +        # symbols. (#1475562)
> +        if buildconfig.substs['OS_ARCH'] == 'WINNT' and \
> +           buildconfig.substs['HOST_OS_ARCH'] != 'WINNT':

I guess we don't really support cross-compiling with mingw-gcc now, so we don't have to worry about that?

::: toolkit/mozapps/installer/packager.py
@@ +310,5 @@
>                      copier.add(libbase + '.chk',
>                                 LibSignFile(os.path.join(args.destination,
>                                                          libname)))
>  
> +    # If a pdb file is present, include it. Run on all OSes to capture MinGW builds

If you make the `MOZ_COPY_PDBS` change I suggested then you could presumably use `buildconfig.substs['MOZ_COPY_PDBS']` here, but I don't think it matters that much since we won't have PDB files to package without that anyway.

Comment 25

[Martin Storsjö]

(In reply to Ted Mielczarek [:ted] [:ted.mielczarek] from comment #24)
> Comment on attachment 9015945 [details] [diff] [review]
> Bug 1475562 Produce pdbs for the mingw-clang build job
> 
> Review of attachment 9015945 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/config/mozconfigs/win32/mingwclang
> @@ +1147,5 @@
> >      # Debug info is ON by default.
> >      if compiler_info.type in ('msvc', 'clang-cl'):
> >          return '-Zi'
> > +    elif target.kernel == 'WINNT' and compiler_info.type == 'clang':
> > +        return '-g -gcodeview'
> 
> Does this really need to include both? It seems like -gcodeview would
> override -g?

Yes - just adding -gcodeview only sets the format, but doesn't enable generating most of the debug info, you need to add -g separately. I haven't traced all the clang/llvm internal options around this to say what the exact logic to this is (when you normally can do e.g. -ggdb which is enough as such) or if it can/should be changed in clang.

Comment 26

[Tom Ritter [:tjr]]

(In reply to Ted Mielczarek [:ted] [:ted.mielczarek] from comment #24)
> I would prefer making this an option in moz.configure somewhere so you can
> simply put `MOZ_COPY_PDBS=1` in mozconfigs. 

Done, thanks.

> @@ +810,5 @@
> >  define syms_template
> >  syms:: $(2)
> >  $(2): $(1)
> >  ifdef MOZ_CRASHREPORTER
> > +ifeq ($(HOST_OS_ARCH),WINNT)
> 
> This bit seems wrong. We want to continue to dump symbols on other platforms!

Yes. I put this in because running that broke things on mingwclang; but this won't get called at all if MOZ_COPY_PDBS is set, so I can just remove the condition entirely.

> ::: python/mozbuild/mozbuild/action/check_binary.py
> @@ +194,5 @@
> > +        # MinGW-Clang, when building pdbs, doesn't include the symbol table into
> > +        # the final module. To get the NSModule info, we can look at the exported
> > +        # symbols. (#1475562)
> > +        if buildconfig.substs['OS_ARCH'] == 'WINNT' and \
> > +           buildconfig.substs['HOST_OS_ARCH'] != 'WINNT':
> 
> I guess we don't really support cross-compiling with mingw-gcc now, so we
> don't have to worry about that?

Correct.

> ::: toolkit/mozapps/installer/packager.py
> @@ +310,5 @@
> >                      copier.add(libbase + '.chk',
> >                                 LibSignFile(os.path.join(args.destination,
> >                                                          libname)))
> >  
> > +    # If a pdb file is present, include it. Run on all OSes to capture MinGW builds
> 
> If you make the `MOZ_COPY_PDBS` change I suggested then you could presumably
> use `buildconfig.substs['MOZ_COPY_PDBS']` here, but I don't think it matters
> that much since we won't have PDB files to package without that anyway.

I like that change though, since it would prevent a future pdb-generation change from silently packaging pdbs into targets with no one noticing, and ballooning disk space. so I added that check.

Comment 27

[Tom Ritter [:tjr]]

Attachment: Bug 1475562 Produce pdbs for the mingw-clang build job

Comment 28

[(not currently active) Ted Mielczarek]

Comment on attachment 9016138 [details] [diff] [review]
Bug 1475562 Produce pdbs for the mingw-clang build job

Review of attachment 9016138 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/moz.configure/toolchain.configure
@@ +1147,5 @@
>      # Debug info is ON by default.
>      if compiler_info.type in ('msvc', 'clang-cl'):
>          return '-Zi'
> +    elif target.kernel == 'WINNT' and compiler_info.type == 'clang':
> +        return '-g -gcodeview'

This seems like it might want to become a list at some point, but it's not that big of a deal if it works right now.

::: config/rules.mk
@@ +163,5 @@
>  endif
>  endif
>  
> +ifneq ($(HOST_OS_ARCH),WINNT)
> +ifeq ($(OS_ARCH),WINNT)

Do we have a more specific test for mingw-clang we can use here?

::: moz.configure
@@ +40,5 @@
>          return False
>  
> +option(env='MOZ_COPY_PDBS',
> +    help='For builds that do not support symbols in the normal fashion, generate'
> +         ' and copy they into the resulting build archive.')

'they'?

Comment 29

[Tom Ritter [:tjr]]

(In reply to Ted Mielczarek [:ted] [:ted.mielczarek] from comment #28)
> Comment on attachment 9016138 [details] [diff] [review]
> Bug 1475562 Produce pdbs for the mingw-clang build job
> 
> Review of attachment 9016138 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: build/moz.configure/toolchain.configure
> @@ +1147,5 @@
> >      # Debug info is ON by default.
> >      if compiler_info.type in ('msvc', 'clang-cl'):
> >          return '-Zi'
> > +    elif target.kernel == 'WINNT' and compiler_info.type == 'clang':
> > +        return '-g -gcodeview'
> 
> This seems like it might want to become a list at some point, but it's not
> that big of a deal if it works right now.

Making it a list breaks stuff :)

> ::: config/rules.mk
> @@ +163,5 @@
> >  endif
> >  endif
> >  
> > +ifneq ($(HOST_OS_ARCH),WINNT)
> > +ifeq ($(OS_ARCH),WINNT)
> 
> Do we have a more specific test for mingw-clang we can use here?

I changed this to 
ifeq ($(OS_ARCH),WINNT)
ifeq ($(CC_TYPE),clang)


> ::: moz.configure
> @@ +40,5 @@
> >          return False
> >  
> > +option(env='MOZ_COPY_PDBS',
> > +    help='For builds that do not support symbols in the normal fashion, generate'
> > +         ' and copy they into the resulting build archive.')
> 
> 'they'?

fixed

Comment 30

[Tom Ritter [:tjr]]

Attachment: Bug 1475562 Produce pdbs for the mingw-clang build job

Comment 31

[(not currently active) Ted Mielczarek]

Comment on attachment 9016378 [details] [diff] [review]
Bug 1475562 Produce pdbs for the mingw-clang build job

Review of attachment 9016378 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for those changes, this looks good!

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/14243af871cc
Produce pdbs for the mingw-clang build job r=ted

Comment 33

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a81783cc2cb1
Backed out changeset 14243af871cc for causing build bustages. CLOSED TREE

Comment 34

[Cosmin Sabou [:CosminS]]

Backed out for causing build bustages.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed,busted&revision=8ce5fe12ae8d888639369cda659cb3c9130efc80&selectedJob=205115056

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=205115056&repo=mozilla-inbound&lineNumber=39701

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/a81783cc2cb164ef91f6d30ab638628ea920d245

Comment 35

[(not currently active) Ted Mielczarek]

Ah shoot, I missed that. You need `if buildconfig.substs.GET('MOZ_COPY_PDBS'):` to handle the unset cases :-(

Comment 36

[Tom Ritter [:tjr]]

Attachment: Bug 1475562 Produce pdbs for the mingw-clang build job

Fix with .get()

Comment 37

[Tom Ritter [:tjr]]

Attachment: Bug 1475562 Produce pdbs for the mingw-clang build job

Had to rebase, sorry =/

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c06d1f31c091
Produce pdbs for the mingw-clang build job r=ted

Comment 39

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/c06d1f31c091

Comment 40

[Tom Ritter [:tjr]]

Attachment: Bug 1475562 - Fix check_binary to examine mingw-clang modules r=ted (esr60)

Comment 41

[Tom Ritter [:tjr]]

Comment on attachment 9035859 [details] [diff] [review]
Bug 1475562 - Fix check_binary to examine mingw-clang modules r=ted (esr60)

This is one of series of patches I am requesting uplift to esr60. Please don't uplift any if the entire series won't go. The whole series will need to go in one push.

This try run (applied on tip-of-esr60 as of an hour ago; and beginning with 'Bug 1491901 - move MK*SHLIB to moz.configure') represents the patch series. It must be applied in that order: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0659d6e265f3b624ad6fbac0c9cd7ce246094596 (If this try run doesn't complete successfully, I will investigate and figure out why)

The uplift request form is the same for all of the patch series; see https://bugzilla.mozilla.org/show_bug.cgi?id=1491901#c10

Comment 42

[Julien Cristau [:jcristau]]

Comment on attachment 9035859 [details] [diff] [review]
Bug 1475562 - Fix check_binary to examine mingw-clang modules r=ted (esr60)

approved for 60.5esr

Comment 43

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/b5fd0e5701d7

Comment 44

[Tom Ritter [:tjr]]

Attachment: Bug 1475562 - Actually generate the pdbs for the mingw-clang build r=ted (esr60)

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: We would like to generate debug pdbs for the mingw-clang builds. For simplicitly, we previously uplifted one requirement for doing this; but did not actually uplift the pdb generation.

User impact if declined: the mingw-clang builds in esr60 will require a local patch to get debugging information for them. That will be kind of annoying and mean esr60 and central will be out of sync.

Fix Landed on Version: 64.0a1 / 20181018123730

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This is a build-time change to mingw-clang and Windows ASAN builds.

String or UUID changes made by this patch:

Comment 45

[Julien Cristau [:jcristau]]

Comment on attachment 9037402 [details] [diff] [review]
Bug 1475562 - Actually generate the pdbs for the mingw-clang build r=ted (esr60)

mingw-clang build system change, approved for 60.5esr

Comment 46

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/434bde49b9c8

Comment 47

[Martin Storsjö]

I'm considering either changing/extending the -pdb option to LLD and I'm interested in your opinions.

In discussions with VLC developers, we realized a limitation with the current -pdb option to LLD; in various build systems, it might be tricky to add the option properly (when building lots of different DLLs); one can't just run configure with "LDFLAGS=-Wl,-pdb,output.pdb" because that'll clobber the same file for all DLLs that are linked.

Instead it would be nice to have an option that indicates "output a PDB, with the default name matching the dll/exe output" without having to literally specify the filename. (This would be similar to what link.exe does if you just pass -debug to it, afaik.)

The problem with the current option in the form "-pdb mymodule.pdb" is that it's impossible to make it into a form without a value.

The ways forward are either to change the form of the option into "-pdb=mymodule.pdb", to allow for a plain "-pdb" with the default behaviour, or to add a completely new option. (Are there any suggestions for a sensible option name for the latter?)

As the mingw -pdb option to LLD is specific to LLD, I don't believe it is in very widespread use yet, so you might be one of the main users so far.

What do you think?

Comment 48

[(Away)]

I don't feel qualified to comment on whether breaking the existing syntax is acceptable. Maybe that should get some input from one of the LLVM lists. If it turns out to be OK, that would certainly be the simplest approach.

If a new option is needed, perhaps it could be called "-debug" to mimic link.exe, or (I don't know if the option parsing allows this) would it be possible use "-pdb=" in such a way that all of {"-pdb x.pdb", "-pdb=x.pdb", "-pdb="} are accepted?

Comment 49

[Martin Storsjö]

(In reply to David Major [:dmajor] from comment #48)

I don't feel qualified to comment on whether breaking the existing syntax is acceptable. Maybe that should get some input from one of the LLVM lists. If it turns out to be OK, that would certainly be the simplest approach.

Thanks for taking the time to comment on this!

If a new option is needed, perhaps it could be called "-debug" to mimic link.exe

I don't think that's generally appropriate, because debug info in general, when it comes to the mingw linkers, is dwarf.

or (I don't know if the option parsing allows this) would it be possible use "-pdb=" in such a way that all of {"-pdb x.pdb", "-pdb=x.pdb", "-pdb="} are accepted?

That's doable with the llvm options parser, and an alternative worth considering. Thanks!

Comment 50

[Tom Ritter [:tjr]]

(In reply to Martin Storsjö from comment #47)

I'm considering either changing/extending the -pdb option to LLD and I'm interested in your opinions.

In discussions with VLC developers, we realized a limitation with the current -pdb option to LLD; in various build systems, it might be tricky to add the option properly (when building lots of different DLLs); one can't just run configure with "LDFLAGS=-Wl,-pdb,output.pdb" because that'll clobber the same file for all DLLs that are linked.

Instead it would be nice to have an option that indicates "output a PDB, with the default name matching the dll/exe output" without having to literally specify the filename. (This would be similar to what link.exe does if you just pass -debug to it, afaik.)

This wwould be really nice. I had to contort to get our build system to work without clobber the output.

The problem with the current option in the form "-pdb mymodule.pdb" is that it's impossible to make it into a form without a value.

The ways forward are either to change the form of the option into "-pdb=mymodule.pdb", to allow for a plain "-pdb" with the default behaviour, or to add a completely new option. (Are there any suggestions for a sensible option name for the latter?)

As the mingw -pdb option to LLD is specific to LLD, I don't believe it is in very widespread use yet, so you might be one of the main users so far.

What do you think?

I have no preference for how you do it, but would be happy if you did =)

Comment 51

[Martin Storsjö]

(In reply to Tom Ritter [:tjr] (On Leave) from comment #50)

(In reply to Martin Storsjö from comment #47)

I'm considering either changing/extending the -pdb option to LLD and I'm interested in your opinions.

In discussions with VLC developers, we realized a limitation with the current -pdb option to LLD; in various build systems, it might be tricky to add the option properly (when building lots of different DLLs); one can't just run configure with "LDFLAGS=-Wl,-pdb,output.pdb" because that'll clobber the same file for all DLLs that are linked.

Instead it would be nice to have an option that indicates "output a PDB, with the default name matching the dll/exe output" without having to literally specify the filename. (This would be similar to what link.exe does if you just pass -debug to it, afaik.)

This wwould be really nice. I had to contort to get our build system to work without clobber the output.

There's a patch evolving for passing the right options when building VLC already though, so it might on the other hand not end up necessary.

One other aspect I thought of, though: When building with autotools (as VLC uses), the linked DLLs are stored in a subdirectory named ".libs". If the build system is unaware of the generated PDBs, it'll be a bit more awkward for a user to collect the generated PDBs from such a directory, compared to them being generated in the main directory (where they aren't stored next to the real DLL though). If the built DLLs are copied somewhere via a command like "make install", that step needs to account for the PDBs as well, and at that point, the convenience of the linker pdb option is probably only a minor detail.

In your case, I guess the build system already knew how to take care of generated PDB files though?

Comment 52

[Tom Ritter [:tjr]]

(In reply to Martin Storsjö from comment #51)

In your case, I guess the build system already knew how to take care of generated PDB files though?

Yes and No. Normally we build the pdbs... somewhere else. But with mingw-clang and ASAN we build them next to the binaries. We already had code for ASAN to deal with that, so I was able to repurpose that code for copying-to-output purposes; I just had to deal with the mingw-clang requirement of specifying the output dll name.