| Summary: | New build warnings | ||
|---|---|---|---|
| Product: | xorg | Reporter: | Xavier Bachelot <xavier> |
| Component: | Driver/openchrome | Assignee: | Kevin Brace <kevinbrace> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | medium | CC: | bensberg, kevinbrace, openchrome-devel |
| Version: | unspecified | ||
| Hardware: | Other | ||
| OS: | All | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Attachments: | |||
Created attachment 124357 [details] F23 chroot build log > I have seen more build warnings when building in a Fedora 23 chroot with the > Fedora enforced compile options, which enables more warnings than the plain > build. I'll report on them later, once I've taken a deeper look. Or I might as well attach the build log right now... Created attachment 124511 [details] [review] viaProbePinStrapping switch statement warning fix There was a compilation warning with a switch statement inside via_outputs.c. (In reply to Xavier Bachelot from comment #0) Hi Xavier, > Several build warnings have appeared in the previous months. Here are the > relevant extracts of a build log. This is a plain build of the master branch > on a fully updated Fedora 23. > > via_outputs.c: In function ‘viaProbePinStrapping’: > via_outputs.c:1202:17: warning: switch condition has boolean value > [-Wswitch-bool] > switch (sr13 && 0xC0) { > ^ > I hope the patch I uploaded fixes the compilation warning. I hope it fixes the warning. > via_ums.c: In function ‘umsCreate’: > via_ums.c:711:11: warning: implicit declaration of function > ‘xf86InitFBManager’ [-Wimplicit-function-declaration] > ret = xf86InitFBManager(pScreen, &AvailFBArea); > ^ > via_ums.c:722:9: warning: implicit declaration of function > ‘xf86InitFBManagerLinear’ [-Wimplicit-function-declaration] > xf86InitFBManagerLinear(pScreen, offset, size); > ^ > > I have seen more build warnings when building in a Fedora 23 chroot with the > Fedora enforced compile options, which enables more warnings than the plain > build. I'll report on them later, once I've taken a deeper look. That one I think comes from the code that is used to support XAA. When going through EXA code path, OpenChrome does not encounter the above API (or ABI or KPI). That being said, I do not know if it still has to be there for those compiling OpenChrome without the use of EXA or XAA (i.e., xBSD). Hi Kevin, (In reply to Kevin Brace from comment #3) > (In reply to Xavier Bachelot from comment #0) > > Hi Xavier, > > > Several build warnings have appeared in the previous months. Here are the > > relevant extracts of a build log. This is a plain build of the master branch > > on a fully updated Fedora 23. > > > > via_outputs.c: In function ‘viaProbePinStrapping’: > > via_outputs.c:1202:17: warning: switch condition has boolean value > > [-Wswitch-bool] > > switch (sr13 && 0xC0) { > > ^ > > > > I hope the patch I uploaded fixes the compilation warning. > I hope it fixes the warning. > Yes, the build warning (and bug) is gone. > > > via_ums.c: In function ‘umsCreate’: > > via_ums.c:711:11: warning: implicit declaration of function > > ‘xf86InitFBManager’ [-Wimplicit-function-declaration] > > ret = xf86InitFBManager(pScreen, &AvailFBArea); > > ^ > > via_ums.c:722:9: warning: implicit declaration of function > > ‘xf86InitFBManagerLinear’ [-Wimplicit-function-declaration] > > xf86InitFBManagerLinear(pScreen, offset, size); > > ^ > > > > I have seen more build warnings when building in a Fedora 23 chroot with the > > Fedora enforced compile options, which enables more warnings than the plain > > build. I'll report on them later, once I've taken a deeper look. > > That one I think comes from the code that is used to support XAA. > When going through EXA code path, OpenChrome does not encounter the above > API (or ABI or KPI). > That being said, I do not know if it still has to be there for those > compiling OpenChrome without the use of EXA or XAA (i.e., xBSD). I though XAA has been removed from the X server ? I know precious little about the BSDs, so I can't comment. Hi Xavier, For xf86InitFBManagerLinear issue, I feel like it is better to keep it as an item in the "TODO list" for OpenChrome Version 0.6. It will take some time to remedy the issue, and at this point, I will rather release a new version since it fixes several bugs and adds VT1632A DVI support. (In reply to Xavier Bachelot from comment #4) > Hi Kevin, > > (In reply to Kevin Brace from comment #3) > > I hope the patch I uploaded fixes the compilation warning. > > I hope it fixes the warning. > > > Yes, the build warning (and bug) is gone. > I will commit the change shortly (I will push it within 24 hours.) > > > I though XAA has been removed from the X server ? > I know precious little about the BSDs, so I can't comment. I have seen a few remnants of XAA in the code. https://cgit.freedesktop.org/openchrome/xf86-video-openchrome/commit/?id=25675f816326056eb9cd617f09d8dad23f1ae94a This stuff from the latest code. _____________________________________________________________ Bool umsCreate(ScrnInfoPtr pScrn) { ScreenPtr pScreen = pScrn->pScreen; VIAPtr pVia = VIAPTR(pScrn); unsigned long offset; BoxRec AvailFBArea; Bool ret = TRUE; long size; int maxY; #ifdef HAVE_DRI if (pVia->directRenderingType == DRI_1) { pVia->driSize = (pVia->FBFreeEnd - pVia->FBFreeStart) >> 2; if ((pVia->driSize > (pVia->maxDriSize * 1024)) && pVia->maxDriSize > 0) pVia->driSize = pVia->maxDriSize * 1024; /* In the case of DRI we handle all VRAM by the DRI ioctls */ if (pVia->useEXA) return TRUE; /* XAA has to use FBManager so we have to split the space with DRI */ maxY = pScrn->virtualY + (pVia->driSize / pVia->Bpl); } else #endif maxY = pVia->FBFreeEnd / pVia->Bpl; /* FBManager can't handle more than 32767 scan lines */ if (maxY > 32767) maxY = 32767; AvailFBArea.x1 = 0; AvailFBArea.y1 = 0; AvailFBArea.x2 = pScrn->displayWidth; AvailFBArea.y2 = maxY; pVia->FBFreeStart = (AvailFBArea.y2 + 1) * pVia->Bpl; /* * Initialization of the XFree86 framebuffer manager is done via * Bool xf86InitFBManager(ScreenPtr pScreen, BoxPtr FullBox) * FullBox represents the area of the framebuffer that the manager * is allowed to manage. This is typically a box with a width * of pScrn->displayWidth and a height of as many lines as can be fit * within the total video memory */ ret = xf86InitFBManager(pScreen, &AvailFBArea); if (ret != TRUE) xf86DrvMsg(pScrn->scrnIndex, X_ERROR, "xf86InitFBManager init failed\n"); DEBUG(xf86DrvMsg(pScrn->scrnIndex, X_INFO, "Frame Buffer From (%d,%d) To (%d,%d)\n", AvailFBArea.x1, AvailFBArea.y1, AvailFBArea.x2, AvailFBArea.y2)); offset = (pVia->FBFreeStart + pVia->Bpp - 1) / pVia->Bpp; size = pVia->FBFreeEnd / pVia->Bpp - offset; if (size > 0) xf86InitFBManagerLinear(pScreen, offset, size); DEBUG(xf86DrvMsg(pScrn->scrnIndex, X_INFO, "Using %d lines for offscreen memory.\n", AvailFBArea.y2 - pScrn->virtualY)); return TRUE; } _____________________________________________________________ Since EXA is almost always used in Linux, it will not call xf86InitFBManager. We can fix this "flaw" later. The switch statement warning issue was fixed by commit 91b0c7d. https://cgit.freedesktop.org/openchrome/xf86-video-openchrome/commit/?id=91b0c7d7d5632eb29f0f72f99c675bf1120c5146 Hi Kevin, The switch statement warning was just the beginning, I uncovered a bunch of identical typos in the viaProbePinStrapping function. I have posted a patch to the mailing list fixing all of them, I assume you haven't noticed it and I should have commented here after sending it. Anyway, here's the link : https://lists.freedesktop.org/archives/openchrome-devel/2016-June/002706.html Please review and apply. Regards, Xavier Just noticed the patch won't apply anymore as the switch statement fix was committed already. I'll rebase the patch and apply it myself once you have reviewed it. *** Bug 96574 has been marked as a duplicate of this bug. *** Created attachment 124595 [details] [review] Proposed patch to fix xf86InitFBManager implicit declaration warning Commit e73fa19 removed including xf86fbman.h when compiling via_ums.c, and as a result, if the code was compiled with a stricter warning compilation option, it will not be able successfully compile the code due to not being able to resolve the lack of xf86InitFBManager declaration. Including xf86fbman.h with via_driver.h resolves this problem. Reported-by: Benno Schulenberg <"Benno's e-mail address"> Reported-by: Xavier Bachelot <"Xavier's e-mail address"> Signed-off-by: Kevin Brace <"Kevin's e-mail address"> Hi Xavier, This commit between Version 0.3.3 and 0.4.0 caused the xf86InitFBManager compilation warning regression. https://cgit.freedesktop.org/openchrome/xf86-video-openchrome/commit/?id=e73fa19fec23d6ec5be836f698d323f89ae48611 The above commit was to fix a compilation error when compiling OpenChrome with Ubuntu 8.04 LTS originally, but it introduced a regression. I found a good solution to this, and it is to include xf86fbman.h with via_driver.h. The fix is confirmed to work when compiling the code for both Ubuntu 8.04 LTS and Lubuntu 12.04. However, for Ubuntu 8.04 LTS, there is another small compilation related regression that got introduced recently that mainly affects older OSes that do not have libpciaccess, and I will commit that fix after this fix is committed. Hi Kevin, The patch in comment 11 fixes the 2 implicit-function-declaration warnings. Tested on Fedora 23. I'm currently looking at the other warnings, mostly unused-variable or unused-but-set-variable warnings, but also unused-functions, etc... and will attach patches asap. Cheers, Xavier Created attachment 124608 [details] [review] Fix for 19 unused-variable warnings Created attachment 124609 [details] [review] Fix unused-variable and unused-but-set-variable warnings in via_display.c Created attachment 124611 [details] [review] Fix unused-but-set-variable warning in via_driver.c Created attachment 124612 [details] [review] Fix 3 unused-variable warnings in via_lvds.c (In reply to Xavier Bachelot from comment #13) Hi Xavier, > Hi Kevin, > > The patch in comment 11 fixes the 2 implicit-function-declaration warnings. > Tested on Fedora 23. > > I'm currently looking at the other warnings, mostly unused-variable or > unused-but-set-variable warnings, but also unused-functions, etc... and will > attach patches asap. > > Cheers, > Xavier Okay, I am glad the fix worked. The reality is that I was barely able to figure out things back in March 2016, so that is probably why I accidentally removed the declaration of xf86fbman.h from via_ums.c to fix the Ubuntu 8.04 LTS compilation error. The fix worked at that point (i.e., was able to compile the Git repository code without modifications after the fix), but it created problems for those without EXA. Regarding the other compilation warnings, I am not too inclined to fix them for the next release (Version 0.5). Unused variable warning is not a particularly severe problem, but it will be in the release after the next release (Version 0.6). Comment on attachment 124608 [details] [review] Fix for 19 unused-variable warnings This patch is wrong. (In reply to Kevin Brace from comment #18) > Regarding the other compilation warnings, I am not too inclined to fix > them for the next release (Version 0.5). > Unused variable warning is not a particularly severe problem, but it will be > in the release after the next release (Version 0.6). Agreed, unused variables are not really a problem, but there are so many builds warning at the moment that the build logs gets barely readable and more important stuff might fall through the cracks. Fixing the obvious warnings first will ease to fix the others later on. Btw, you did not comment on the viaProbePinStrapping fixes I sent. I'm waiting for this patch to land before looping over all my boards to sent the pin strapping info for them. The xf86InitFBManager implicit declaration warning issue was fixed by commit ae62ee7. https://cgit.freedesktop.org/openchrome/xf86-video-openchrome/commit/?id=ae62ee78bf6aa7b8f3ba642fc8d5ff55947b5741 (In reply to Xavier Bachelot from comment #20) Hi Xavier, > Agreed, unused variables are not really a problem, but there are so many > builds warning at the moment that the build logs gets barely readable and > more important stuff might fall through the cracks. Fixing the obvious > warnings first will ease to fix the others later on. > The fix will have to be during OpenChrome Version 0.6 development phase (i.e., Version 0.5.zzz) I released Version 0.5 RC2, and while it is possible that there might have to be 0.5 RC3, the code cleanup work can happen later. That being said, if there are no major objections, OpenChrome Version 0.5 will be officially released in a few days, not weeks. > Btw, you did not comment on the viaProbePinStrapping fixes I sent. I'm > waiting for this patch to land before looping over all my boards to sent the > pin strapping info for them. I finally replied to your e-mail, and I will be rejecting your patch. This link explains the reason. https://en.wikipedia.org/wiki/Bitwise_operations_in_C#Logical_equivalents Hi Xavier, One more thing. You probably should have not added even more compilation warnings to this bug report since both of the original complaints have been resolved. You probably should have started a new bug report, but since you added more, I will keep it open until all of the warnings you have identified are taken cared at some point. The patches to fix various build warnings attached to this bug are all obsoletes, more or less. Instead, I've pushed all patches to my fdo tree in the fix_warnings branch. git://people.freedesktop.org/~xavierb/xf86-video-openchrome fix_warnings Comment on attachment 124511 [details] [review] viaProbePinStrapping switch statement warning fix Committed. Comment on attachment 124595 [details] [review] Proposed patch to fix xf86InitFBManager implicit declaration warning Committed. Hi Xavier, I have committed the fixes while back. https://cgit.freedesktop.org/openchrome/xf86-video-openchrome/commit/?id=ae62ee78bf6aa7b8f3ba642fc8d5ff55947b5741 https://cgit.freedesktop.org/openchrome/xf86-video-openchrome/commit/?id=91b0c7d7d5632eb29f0f72f99c675bf1120c5146 Hi Xavier, I sharply reduced the compilation warnings I have caused, so I am closing this bug. If you want further compilation warnings reduction, please open a new bug. |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.
Several build warnings have appeared in the previous months. Here are the relevant extracts of a build log. This is a plain build of the master branch on a fully updated Fedora 23. via_outputs.c: In function ‘viaProbePinStrapping’: via_outputs.c:1202:17: warning: switch condition has boolean value [-Wswitch-bool] switch (sr13 && 0xC0) { ^ via_ums.c: In function ‘umsCreate’: via_ums.c:711:11: warning: implicit declaration of function ‘xf86InitFBManager’ [-Wimplicit-function-declaration] ret = xf86InitFBManager(pScreen, &AvailFBArea); ^ via_ums.c:722:9: warning: implicit declaration of function ‘xf86InitFBManagerLinear’ [-Wimplicit-function-declaration] xf86InitFBManagerLinear(pScreen, offset, size); ^ I have seen more build warnings when building in a Fedora 23 chroot with the Fedora enforced compile options, which enables more warnings than the plain build. I'll report on them later, once I've taken a deeper look.